Add mechanism to disable any or all plugins #191

ascrutae · 2017-05-05T10:37:10Z

Relate: #186

coveralls · 2017-05-05T11:00:33Z

Coverage increased (+0.2%) to 71.354% when pulling 56daa9ac1d4039af6f30393e5214da5386de482c on ascrutae:zhangxin/feature/186 into ecf7a2b on wu-sheng:master.

coveralls · 2017-05-05T11:05:37Z

Coverage increased (+0.07%) to 71.265% when pulling 56daa9ac1d4039af6f30393e5214da5386de482c on ascrutae:zhangxin/feature/186 into ecf7a2b on wu-sheng:master.

wu-sheng · 2017-05-05T12:04:52Z

apm-sniffer/apm-agent-core/src/main/java/org/skywalking/apm/agent/core/plugin/PluginCfg.java

+    private List<PluginDefine> pluginClassList = new ArrayList<PluginDefine>();
+    protected List<String> disabledPlugins = new ArrayList<String>();
+
+    PluginCfg() {


Using the contructor for initialization should very careful, if this enum is used before config initialization, the disable mechanism will fail.

So I prefer to make the initialization as a boot service, rather than this.

wu-sheng · 2017-05-05T12:07:51Z

apm-sniffer/apm-agent-core/src/main/java/org/skywalking/apm/agent/core/plugin/PluginCfg.java

@@ -12,15 +13,29 @@
 public enum PluginCfg {
    INSTANCE;

-    private List<String> pluginClassList = new ArrayList<String>();
+    private List<PluginDefine> pluginClassList = new ArrayList<PluginDefine>();
+    protected List<String> disabledPlugins = new ArrayList<String>();


DisableList is only used in plugin initialization, shouldn't set it as a class field.

wu-sheng · 2017-05-05T12:09:49Z

apm-sniffer/apm-agent-core/src/main/java/org/skywalking/apm/agent/core/plugin/PluginCfg.java

@@ -29,6 +44,55 @@ void load(InputStream input) throws IOException {
    }

    public List<String> getPluginClassList() {
-        return pluginClassList;
+        List<String> pluginClasses = new ArrayList<String>();


Why need this new list? Just a list copy?

wu-sheng · 2017-05-05T12:10:44Z

apm-sniffer/apm-agent-core/src/main/java/org/skywalking/apm/agent/core/plugin/PluginCfg.java

+    private static class PluginDefine {
+
+
+        private static final PluginDefine NOOP_PLUGIN = new PluginDefine() {


This is not a Noop. It always set a empty plugin define.

wu-sheng · 2017-05-05T12:11:48Z

apm-sniffer/apm-agent-core/src/main/java/org/skywalking/apm/agent/core/plugin/PluginCfg.java

+
+        public static PluginDefine build(String define) {
+            if (StringUtil.isEmpty(define)) {
+                return NOOP_PLUGIN;


Did not like the design of retuning noop plugin define. Hard to understand and make no sense.

wu-sheng · 2017-05-05T12:19:59Z

apm-sniffer/apm-agent-core/src/main/java/org/skywalking/apm/agent/core/conf/Config.java

+         * if you have multiple plugins need to disable.
+         *
+         * Here are the plugin names :
+         * TOMCAT, DUBBO, JEDIS, MOTAN, HTTPCLIENT, JDBC, MONGODB.


All existed config items are in lower case, why choose upper case this time?

Some plugin names should include version, I think

coveralls · 2017-05-07T14:47:26Z

Coverage increased (+0.008%) to 71.2% when pulling fa39a4c475c80645a871989140243ad46a46dfa6 on ascrutae:zhangxin/feature/186 into ecf7a2b on wu-sheng:master.

coveralls · 2017-05-07T14:47:34Z

Coverage increased (+0.1%) to 71.289% when pulling fa39a4c475c80645a871989140243ad46a46dfa6 on ascrutae:zhangxin/feature/186 into ecf7a2b on wu-sheng:master.

wu-sheng · 2017-05-08T00:16:06Z

apm-sniffer/apm-sdk-plugin/tomcat-7.x-8.x-plugin/src/main/resources/skywalking-plugin.def

@@ -1 +1 @@
-org.skywalking.apm.plugin.tomcat78x.define.TomcatInstrumentation
+tomcat-7.x=org.skywalking.apm.plugin.tomcat78x.define.TomcatInstrumentation


IMO, the key should be tomcat-7.x/8.x, if.f / is legal.

wu-sheng · 2017-05-08T00:22:16Z

apm-sniffer/apm-agent-core/src/main/java/org/skywalking/apm/agent/core/plugin/PluginCfg.java

+            String pluginDefine = null;
+            while ((pluginDefine = reader.readLine()) != null) {
+                PluginDefine plugin = PluginDefine.build(pluginDefine);
+                if (!plugin.disabled(disabledPlugins)) {


Are you sure this is going to work?

According to premain

public static void premain(String agentArgs, Instrumentation instrumentation) throws PluginException { SnifferConfigInitializer.initialize(); final PluginFinder pluginFinder = new PluginFinder(new PluginBootstrap().loadPlugins()); ServiceManager.INSTANCE.boot();

load(InputStream input) invokes before ServiceManager.boot. disabledPlugins(initialize in DisabledPluginInitService) must be empty every time, how does this pr work?

coveralls · 2017-05-08T04:11:19Z

Coverage decreased (-0.1%) to 71.068% when pulling 9c7053b on ascrutae:zhangxin/feature/186 into e3a42f7 on wu-sheng:master.

wu-sheng

This pr has evolved to a formal pr. I think you have a lot of works to do.

cc @pengys5

wu-sheng · 2017-05-08T12:36:00Z

apm-commons/apm-util/src/main/java/org/skywalking/apm/util/ConfigInitializer.java

@@ -46,6 +50,19 @@ else if (type.isEnum())
            parentDesc.removeLastDesc();
        }
    }
+
+    private static List convert2List(String value) {
+        List result = new ArrayList();


I think LinkedList is better choice for performance.

wu-sheng · 2017-05-08T12:36:27Z

apm-sniffer/apm-agent-core/src/main/java/org/skywalking/apm/agent/core/conf/Config.java

+         * Here are the plugin names :
+         * tomcat-7.x/8.x, dubbo, jedis-2.x, motan, httpclient-4.x, jdbc, mongodb-3.x.
+         */
+        public static List DISABLED_PLUGINS = new ArrayList();


Same performance concern. Prefer LinkedList.

wu-sheng · 2017-05-08T12:36:50Z

...iffer/apm-agent-core/src/main/java/org/skywalking/apm/agent/core/plugin/PluginBootstrap.java

                plugin.setClassTypePool(classTypePool);
                plugins.add(plugin);
            } catch (Throwable t) {
-                logger.error(t, "load plugin [{}] failure.", pluginClassName);
+                logger.error(t, "loade plugin [{}] failure.", pluginDefine.getDefineClass());


loade, typo.

wu-sheng · 2017-05-08T12:38:31Z

apm-sniffer/apm-agent-core/src/main/java/org/skywalking/apm/agent/core/plugin/PluginCfg.java

+            String pluginDefine = null;
+            while ((pluginDefine = reader.readLine()) != null) {
+                PluginDefine plugin = PluginDefine.build(pluginDefine);
+                if (!plugin.disabled(Config.Plugin.DISABLED_PLUGINS)) {


You are using a static field as a parameter, why? You can use is in disabled method, no parameter is needed.

And I prefer the method name should be boolean enable().

wu-sheng · 2017-05-08T12:42:46Z

apm-sniffer/apm-agent-core/src/main/java/org/skywalking/apm/agent/core/plugin/PluginDefine.java

+
+    public static PluginDefine build(String define) {
+        if (StringUtil.isEmpty(define)) {
+            return IllegalPluginDefine.INSTANCE;


Still don't understand this enum class, also strange name IllegalPluginDefine. If it is illegal, why don't throw exception?

wu-sheng · 2017-05-08T12:43:09Z

apm-sniffer/apm-agent-core/src/main/java/org/skywalking/apm/agent/core/plugin/PluginDefine.java

+        return defineClass;
+    }
+
+    static class IllegalPluginDefine extends PluginDefine {


Again and again, don't accept this design.

wu-sheng · 2017-05-08T12:44:04Z

apm-sniffer/apm-sdk-plugin/jedis-2.x-plugin/src/main/resources/skywalking-plugin.def

-org.skywalking.apm.plugin.jedis.v2.define.JedisClusterInstrumentation
-org.skywalking.apm.plugin.jedis.v2.define.JedisInstrumentation
+jedis=org.skywalking.apm.plugin.jedis.v2.define.JedisClusterInstrumentation
+jedis=org.skywalking.apm.plugin.jedis.v2.define.JedisInstrumentation


jedis-2.x, I think

wu-sheng · 2017-05-08T12:44:25Z

apm-sniffer/apm-sdk-plugin/motan-plugin/src/main/resources/skywalking-plugin.def

@@ -1,2 +1,2 @@
-org.skywalking.apm.plugin.motan.define.MotanConsumerInstrumentation
-org.skywalking.apm.plugin.motan.define.MotanProviderInstrumentation
+motan=org.skywalking.apm.plugin.motan.define.MotanConsumerInstrumentation


motan-0.x, I think.

…y-walking into zhangxin/feature/186

ascrutae · 2017-05-08T13:29:26Z

@wu-sheng yeah. I think so, I will test it.

wu-sheng

Still need some adjustments.

wu-sheng · 2017-05-08T13:31:19Z

apm-sniffer/apm-agent-core/src/main/java/org/skywalking/apm/agent/core/plugin/PluginCfg.java

+            while ((pluginDefine = reader.readLine()) != null) {
+                try {
+                    PluginDefine plugin = PluginDefine.build(pluginDefine);
+                    if (!plugin.disabled()) {


You never used disabled, just want to know enable or not.

wu-sheng · 2017-05-08T13:32:48Z

apm-sniffer/apm-agent-core/src/main/java/org/skywalking/apm/agent/core/plugin/PluginDefine.java

+
+    public static PluginDefine build(String define) throws IllegalPluginDefineException {
+        if (StringUtil.isEmpty(define)) {
+            throw new IllegalPluginDefineException();


Not constructor parameter, why?

wu-sheng · 2017-05-08T13:32:53Z

apm-sniffer/apm-agent-core/src/main/java/org/skywalking/apm/agent/core/plugin/PluginDefine.java

+
+        String[] pluginDefine = define.split("=");
+        if (pluginDefine.length != 2) {
+            throw new IllegalPluginDefineException();


Not constructor parameter, why?

coveralls · 2017-05-08T13:46:56Z

Coverage decreased (-0.09%) to 71.104% when pulling 32dfb848c3bd367e691d2a661aa31c5bda0e5b32 on ascrutae:zhangxin/feature/186 into e3a42f7 on wu-sheng:master.

coveralls · 2017-05-08T13:56:21Z

Coverage decreased (-0.1%) to 71.093% when pulling 152715a7ae008b0bb680b3af5df46734d8b6cf97 on ascrutae:zhangxin/feature/186 into e3a42f7 on wu-sheng:master.

coveralls · 2017-05-08T13:57:27Z

Coverage decreased (-0.01%) to 71.182% when pulling 152715a7ae008b0bb680b3af5df46734d8b6cf97 on ascrutae:zhangxin/feature/186 into e3a42f7 on wu-sheng:master.

wu-sheng

Still need some changes.

wu-sheng · 2017-05-09T05:48:40Z

apm-commons/apm-util/src/main/java/org/skywalking/apm/util/ConfigInitializer.java

+
+        String[] segments = value.split(",");
+        for (String segment : segments) {
+            result.add(segment);


You miss segment.trim() and avoid empty string.

wu-sheng · 2017-05-09T05:50:33Z

...c/main/java/org/skywalking/apm/agent/core/plugin/exception/IllegalPluginDefineException.java

+
+/**
+ * {@link IllegalPluginDefineException} present that the value of plugin define can
+ * not be formatted.


Can't understand the comments.....

wu-sheng · 2017-05-09T14:04:38Z

...olkit-activation/apm-toolkit-opentracing-activation/src/main/resources/skywalking-plugin.def

-org.skywalking.apm.toolkit.activation.opentracing.span.SkyWalkingSpanActivation
-org.skywalking.apm.toolkit.activation.opentracing.tracer.SkyWalkingTracerActivation
+opentracing-0.20.x=org.skywalking.apm.toolkit.activation.opentracing.span.SkyWalkingSpanActivation
+opentracing-0.20.x=org.skywalking.apm.toolkit.activation.opentracing.tracer.SkyWalkingTracerActivation


opentracing activation is based on our application toolkit, so the name should not include version, especially ot version. Keep the name just opentracing

wu-sheng · 2017-05-09T14:05:33Z

...kit-activation/apm-toolkit-trace-context-activation/src/main/resources/skywalking-plugin.def

Where does 3.x comes from? Our project version? Same as opentracing-toolkit-activation, just tracecontext is enough.

ascrutae · 2017-05-09T14:42:31Z

Both of test applications were disabled jdbc plugin

Here is the topological graph

Here is the trace segment graph

wu-sheng

LGTM

coveralls · 2017-05-10T05:46:51Z

Coverage decreased (-0.01%) to 71.182% when pulling f5565569c148d0a4d609f6158512128d5115e77f on ascrutae:zhangxin/feature/186 into e3a42f7 on wu-sheng:master.

coveralls · 2017-05-10T05:58:18Z

Coverage decreased (-0.1%) to 71.072% when pulling b04888ce9965c14f8302e286b91826fd33d06e44 on ascrutae:zhangxin/feature/186 into e3a42f7 on wu-sheng:master.

coveralls · 2017-05-10T05:58:25Z

Coverage decreased (-0.1%) to 71.072% when pulling b04888ce9965c14f8302e286b91826fd33d06e44 on ascrutae:zhangxin/feature/186 into e3a42f7 on wu-sheng:master.

coveralls · 2017-05-10T06:44:48Z

Coverage decreased (-0.1%) to 71.072% when pulling 3f902ca5c4e85ce7bcc25752722ea9a00755ec09 on ascrutae:zhangxin/feature/186 into e3a42f7 on wu-sheng:master.

ascrutae force-pushed the zhangxin/feature/186 branch from f2d4f30 to 56daa9a Compare May 5, 2017 10:51

wu-sheng requested changes May 5, 2017

View reviewed changes

ascrutae force-pushed the zhangxin/feature/186 branch 2 times, most recently from 4e6df40 to fa39a4c Compare May 7, 2017 14:36

wu-sheng requested changes May 8, 2017

View reviewed changes

Add mechanism to disable any or all plugins, Relate: apache#186

da32abf

ascrutae force-pushed the zhangxin/feature/186 branch from f88ea4d to da32abf Compare May 8, 2017 04:00

Merge branch 'master' into zhangxin/feature/186

9c7053b

ascrutae changed the title ~~[POC-PR] Add mechanism to disable any or all plugins~~ Add mechanism to disable any or all plugins May 8, 2017

wu-sheng requested changes May 8, 2017

View reviewed changes

ascrutae added 2 commits May 8, 2017 21:26

Add mechanism to disable any or all plugins

744b81a

Merge branch 'zhangxin/feature/186' of https://github.com/ascrutae/sk…

1f89be0

…y-walking into zhangxin/feature/186

wu-sheng requested changes May 8, 2017

View reviewed changes

ascrutae force-pushed the zhangxin/feature/186 branch from 32dfb84 to 152715a Compare May 8, 2017 13:46

wu-sheng requested changes May 9, 2017

View reviewed changes

ascrutae force-pushed the zhangxin/feature/186 branch 3 times, most recently from e12ee04 to 3f902ca Compare May 9, 2017 11:27

wu-sheng requested changes May 9, 2017

View reviewed changes

Add mechanism to disable any or all plugins

fec9d36

ascrutae force-pushed the zhangxin/feature/186 branch from 8ba8e5a to fec9d36 Compare May 9, 2017 14:39

wu-sheng approved these changes May 9, 2017

View reviewed changes

wu-sheng merged commit 4a56043 into apache:master May 9, 2017

wu-sheng mentioned this pull request May 9, 2017

Add mechanism to disable any or all plugins #186

Closed

ascrutae deleted the zhangxin/feature/186 branch September 28, 2020 05:45

DevPJ9 mentioned this pull request Oct 14, 2022

[BanyanDB] Support "IN" and "NOT IN" binary operation in criteria #9759

Closed

3 tasks

		private static class PluginDefine {


		private static final PluginDefine NOOP_PLUGIN = new PluginDefine() {

		@@ -1 +1 @@
		org.skywalking.apm.plugin.tomcat78x.define.TomcatInstrumentation
		tomcat-7.x=org.skywalking.apm.plugin.tomcat78x.define.TomcatInstrumentation

Add mechanism to disable any or all plugins #191

Add mechanism to disable any or all plugins #191

Conversation

ascrutae commented May 5, 2017

coveralls commented May 5, 2017

coveralls commented May 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented May 7, 2017

coveralls commented May 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented May 8, 2017

wu-sheng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ascrutae commented May 8, 2017

wu-sheng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented May 8, 2017

coveralls commented May 8, 2017

coveralls commented May 8, 2017

wu-sheng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ascrutae commented May 9, 2017

wu-sheng left a comment

Choose a reason for hiding this comment

coveralls commented May 10, 2017

coveralls commented May 10, 2017

coveralls commented May 10, 2017

coveralls commented May 10, 2017