wu-sheng commented on a change in pull request #7477:
URL: https://github.com/apache/skywalking/pull/7477#discussion_r690962844
##########
File path:
apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/plugin/PluginInitializer.java
##########
@@ -0,0 +1,50 @@
+package org.apache.skywalking.apm.agent.core.plugin;
+
+import org.apache.skywalking.apm.agent.core.conf.Config;
+import org.apache.skywalking.apm.agent.core.plugin.loader.AgentClassLoader;
+import org.apache.skywalking.apm.util.StringUtil;
+
+import java.util.regex.Pattern;
+
+/**
+ * @author dmz
+ * @date Create in 1:06 2021/8/17
Review comment:
These are not allowed, you should add comments about what this class
does.
##########
File path:
apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/plugin/PluginInitializer.java
##########
@@ -0,0 +1,50 @@
+package org.apache.skywalking.apm.agent.core.plugin;
+
+import org.apache.skywalking.apm.agent.core.conf.Config;
+import org.apache.skywalking.apm.agent.core.plugin.loader.AgentClassLoader;
+import org.apache.skywalking.apm.util.StringUtil;
+
+import java.util.regex.Pattern;
+
+/**
+ * @author dmz
+ * @date Create in 1:06 2021/8/17
+ */
+public class PluginInitializer {
+ /**
+ * create and init
Review comment:
This is pointless comment.
##########
File path:
apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/plugin/PluginInitializer.java
##########
@@ -0,0 +1,50 @@
+package org.apache.skywalking.apm.agent.core.plugin;
Review comment:
Error logs
> ERROR the following files don't have a valid license header:
apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/plugin/PluginInitializer.java
You need License header.
##########
File path:
apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/plugin/PluginInitializer.java
##########
@@ -0,0 +1,50 @@
+package org.apache.skywalking.apm.agent.core.plugin;
+
+import org.apache.skywalking.apm.agent.core.conf.Config;
+import org.apache.skywalking.apm.agent.core.plugin.loader.AgentClassLoader;
+import org.apache.skywalking.apm.util.StringUtil;
+
+import java.util.regex.Pattern;
+
+/**
+ * @author dmz
+ * @date Create in 1:06 2021/8/17
+ */
+public class PluginInitializer {
+ /**
+ * create and init
+ */
+ public static AbstractClassEnhancePluginDefine initialize(PluginDefine
pluginDefine)
+ throws ReflectiveOperationException {
+
+ AbstractClassEnhancePluginDefine pluginDefineInstance =
create(pluginDefine);
+
+ init(pluginDefineInstance, pluginDefine.getName());
+
+ return pluginDefineInstance;
+
+ }
+ /**
+ * create instance by reflection
+ */
+ private static AbstractClassEnhancePluginDefine create(PluginDefine
pluginDefine)
+ throws ReflectiveOperationException {
+ Class<?> pluginDefineClass =
Class.forName(pluginDefine.getDefineClass(), true,
AgentClassLoader.getDefault());
+ return
(AbstractClassEnhancePluginDefine)pluginDefineClass.newInstance();
+ }
+
+ /**
+ * just set isExtInstrumentation property in {@link
AbstractClassEnhancePluginDefine}
Review comment:
```suggestion
* Set isExtInstrumentation property in {@link
AbstractClassEnhancePluginDefine} according to agent.config
```
##########
File path:
apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/plugin/PluginInitializer.java
##########
@@ -0,0 +1,50 @@
+package org.apache.skywalking.apm.agent.core.plugin;
+
+import org.apache.skywalking.apm.agent.core.conf.Config;
+import org.apache.skywalking.apm.agent.core.plugin.loader.AgentClassLoader;
+import org.apache.skywalking.apm.util.StringUtil;
+
+import java.util.regex.Pattern;
+
+/**
+ * @author dmz
+ * @date Create in 1:06 2021/8/17
+ */
+public class PluginInitializer {
+ /**
+ * create and init
+ */
+ public static AbstractClassEnhancePluginDefine initialize(PluginDefine
pluginDefine)
+ throws ReflectiveOperationException {
+
+ AbstractClassEnhancePluginDefine pluginDefineInstance =
create(pluginDefine);
+
+ init(pluginDefineInstance, pluginDefine.getName());
+
+ return pluginDefineInstance;
+
+ }
+ /**
+ * create instance by reflection
Review comment:
Meaningless comment.
##########
File path:
apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/plugin/PluginBootstrap.java
##########
@@ -1,30 +1,27 @@
/*
- * Licensed to the Apache Software Foundation (ASF) under one or more
- * contributor license agreements. See the NOTICE file distributed with
- * this work for additional information regarding copyright ownership.
- * The ASF licenses this file to You under the Apache License, Version 2.0
- * (the "License"); you may not use this file except in compliance with
- * the License. You may obtain a copy of the License at
+ * Licensed to the Apache Software Foundation (ASF) under one or more
contributor license agreements. See the NOTICE
+ * file distributed with this work for additional information regarding
copyright ownership. The ASF licenses this file
+ * to You under the Apache License, Version 2.0 (the "License"); you may not
use this file except in compliance with the
+ * License. You may obtain a copy of the License at
*
- * http://www.apache.org/licenses/LICENSE-2.0
+ * http://www.apache.org/licenses/LICENSE-2.0
*
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
+ * Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on
+ * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
express or implied. See the License for the
+ * specific language governing permissions and limitations under the License.
Review comment:
These should not change. I think the key is importing codestyle,
https://github.com/apache/skywalking/blob/master/codeStyle.xml
##########
File path:
apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/plugin/PluginInitializer.java
##########
@@ -0,0 +1,63 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+
+package org.apache.skywalking.apm.agent.core.plugin;
+
+import org.apache.skywalking.apm.agent.core.conf.Config;
+import org.apache.skywalking.apm.agent.core.plugin.loader.AgentClassLoader;
+import org.apache.skywalking.apm.util.StringUtil;
+
+import java.util.regex.Pattern;
+
+/**
+ * create a AbstractClassEnhancePluginDefine instance according to {@link
PluginDefine} and init it with agent.config
+ */
+public class PluginInitializer {
+
+ public static AbstractClassEnhancePluginDefine initialize(PluginDefine
pluginDefine)
+ throws ReflectiveOperationException {
+
+ AbstractClassEnhancePluginDefine pluginDefineInstance =
create(pluginDefine);
+
+ init(pluginDefineInstance, pluginDefine.getName());
+
+ return pluginDefineInstance;
+
+ }
+
+ private static AbstractClassEnhancePluginDefine create(PluginDefine
pluginDefine)
+ throws ReflectiveOperationException {
+ Class<?> pluginDefineClass =
Class.forName(pluginDefine.getDefineClass(), true,
AgentClassLoader.getDefault());
+ return (AbstractClassEnhancePluginDefine)
pluginDefineClass.newInstance();
+ }
+
+ /**
+ * Set isExtInstrumentation property in {@link
AbstractClassEnhancePluginDefine} according to agent.config
+ */
+ private static void init(AbstractClassEnhancePluginDefine instance, String
pluginName) {
+ if (StringUtil.isNotEmpty(Config.Plugin.PLUGINS_IN_EXT_CLASS_LOADER)) {
+ String[] pluginMatchRules =
Config.Plugin.PLUGINS_IN_EXT_CLASS_LOADER.split(",");
+ for (String pluginMatchRule : pluginMatchRules) {
+ final Pattern p = Pattern.compile(pluginMatchRule.replace("*",
".*"));
Review comment:
It is better to compile all rules at the beginning, otherwise, this
would clearly impact the starting time.
##########
File path:
apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/plugin/PluginInitializer.java
##########
@@ -0,0 +1,63 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+
+package org.apache.skywalking.apm.agent.core.plugin;
+
+import org.apache.skywalking.apm.agent.core.conf.Config;
+import org.apache.skywalking.apm.agent.core.plugin.loader.AgentClassLoader;
+import org.apache.skywalking.apm.util.StringUtil;
+
+import java.util.regex.Pattern;
+
+/**
+ * create a AbstractClassEnhancePluginDefine instance according to {@link
PluginDefine} and init it with agent.config
+ */
+public class PluginInitializer {
+
+ public static AbstractClassEnhancePluginDefine initialize(PluginDefine
pluginDefine)
+ throws ReflectiveOperationException {
+
+ AbstractClassEnhancePluginDefine pluginDefineInstance =
create(pluginDefine);
+
+ init(pluginDefineInstance, pluginDefine.getName());
+
+ return pluginDefineInstance;
+
+ }
+
+ private static AbstractClassEnhancePluginDefine create(PluginDefine
pluginDefine)
+ throws ReflectiveOperationException {
+ Class<?> pluginDefineClass =
Class.forName(pluginDefine.getDefineClass(), true,
AgentClassLoader.getDefault());
+ return (AbstractClassEnhancePluginDefine)
pluginDefineClass.newInstance();
+ }
+
+ /**
+ * Set isExtInstrumentation property in {@link
AbstractClassEnhancePluginDefine} according to agent.config
+ */
+ private static void init(AbstractClassEnhancePluginDefine instance, String
pluginName) {
+ if (StringUtil.isNotEmpty(Config.Plugin.PLUGINS_IN_EXT_CLASS_LOADER)) {
+ String[] pluginMatchRules =
Config.Plugin.PLUGINS_IN_EXT_CLASS_LOADER.split(",");
+ for (String pluginMatchRule : pluginMatchRules) {
+ final Pattern p = Pattern.compile(pluginMatchRule.replace("*",
".*"));
Review comment:
Include the split process.
##########
File path:
apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/plugin/PluginBootstrap.java
##########
@@ -63,9 +64,7 @@
for (PluginDefine pluginDefine : pluginClassList) {
Review comment:
`PluginInitializer#init` could be called before this loop.
##########
File path:
apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/plugin/PluginInitializer.java
##########
@@ -0,0 +1,63 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+
+package org.apache.skywalking.apm.agent.core.plugin;
+
+import org.apache.skywalking.apm.agent.core.conf.Config;
+import org.apache.skywalking.apm.agent.core.plugin.loader.AgentClassLoader;
+import org.apache.skywalking.apm.util.StringUtil;
+
+import java.util.regex.Pattern;
+
+/**
+ * create a AbstractClassEnhancePluginDefine instance according to {@link
PluginDefine} and init it with agent.config
+ */
+public class PluginInitializer {
+
+ public static AbstractClassEnhancePluginDefine initialize(PluginDefine
pluginDefine)
Review comment:
```suggestion
public static AbstractClassEnhancePluginDefine create(PluginDefine
pluginDefine)
```
##########
File path:
apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/plugin/PluginInitializer.java
##########
@@ -0,0 +1,63 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+
+package org.apache.skywalking.apm.agent.core.plugin;
+
+import org.apache.skywalking.apm.agent.core.conf.Config;
+import org.apache.skywalking.apm.agent.core.plugin.loader.AgentClassLoader;
+import org.apache.skywalking.apm.util.StringUtil;
+
+import java.util.regex.Pattern;
+
+/**
+ * create a AbstractClassEnhancePluginDefine instance according to {@link
PluginDefine} and init it with agent.config
+ */
+public class PluginInitializer {
+
+ public static AbstractClassEnhancePluginDefine initialize(PluginDefine
pluginDefine)
Review comment:
You actually don't need another `create` and `init`. There is no point
to split a 10-20 lines method into 3 methods, but no one is using other 2
methods.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
