Add a plugin that supports the Solon framework. #697

xsShuang · 2024-06-20T04:18:30Z

Add an agent plugin to support

Add a test case for the new plugin, refer to the doc
Add a component id in the component-libraries.yml
Add a logo in the UI repo

wu-sheng · 2024-06-20T04:26:17Z

...n/solon-2.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/solon/FastPathMatcher.java

+
+package org.apache.skywalking.apm.plugin.solon;
+
+public class FastPathMatcher {


What is this? I think we didn't discuss about this part.

Controlling which paths require plugins through environment variables, so I used this class, which comes from this project https://github.com/apache/skywalking-java/blob/main/apm-sniffer/optional-plugins/trace-ignore-plugin/src/main/java/org/apache/skywalking/apm/plugin/trace/ignore/matcher/FastPathMatcher.java

Ignore plugin is reading configurations from the agent folder, why do ou need that for Solon?

solon 不需要他，是我自己想添加这样一个功能。尽管有 trace-ignore-plugin 这个插件，但我认为这个插件是在链路追踪后写入数据时阻止忽略的数据写入，而我希望的是插件在生效时，直接过滤排除的 url 请求地址

这个类用进行 URL 地址匹配，从而实现排除某些不需要链路追踪 URL 的目的

Please keep in English in GH discussion.

I don't think ignoring should be implemented by a specific plugin of a specific framework.

Please remove this part.

AFAIK, the existing ignore plugin could collaborate with your plugin automatically.

wu-sheng · 2024-06-20T06:01:48Z

Also, I can see you didn't follow the document to add relative plugin tests.

Notice, as we are only hosting your codes, to guarantee it works for end users, we need you to finish plugin tests and verify the data.

xsShuang · 2024-06-20T07:43:19Z

Also, I can see you didn't follow the document to add relative plugin tests.

Notice, as we are only hosting your codes, to guarantee it works for end users, we need you to finish plugin tests and verify the data.

抱歉，我没有linux和mac环境来运行自动化测试，但我编写了我提交插件的测试文件，请问有其他的办法可以进行测试吗？

wu-sheng · 2024-06-20T07:47:23Z

Please use English on Github.

You could use VM or something to run the tests.

wu-sheng · 2024-06-20T08:09:43Z

Please add it into CI control file to make sure it runnable, and attach your local test results.

Meanwhile please be clear about supported versions in docs and test supported list.

wu-sheng · 2024-06-20T11:32:25Z

test/plugin/scenarios/solon-2.x-scenario/support-version.list

+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+2.8.3


We should not support only one version. You could verify all supported versions(patch versions), and list all minor versions, one patch version per minor version(such as 2.8.3 for 2.8.x, 2.7.3 for 2.7.x).

wu-sheng · 2024-06-20T11:33:16Z

test/plugin/scenarios/solon-2.x-scenario/config/expectedData.yaml

+      spanLayer: Http
+      startTime: not null
+      endTime: not null
+      componentId: 14


There is no Solon component related. What is the meaning of your tests?

Sorry, this isn't the final code, I just wanted to get a different computer to continue with the code

...ugin/src/main/java/org/apache/skywalking/apm/plugin/solon/SolonActionExecuteInterceptor.java

xsShuang · 2024-06-20T13:37:01Z

wu-sheng · 2024-06-20T14:49:45Z

Please follow other comments to fix these

GHA control file needs to add this new test case.
Docs should be updated. changes.md and supported-list.md
Verification version list.

wu-sheng · 2024-06-21T13:07:51Z

Any update?

xsShuang · 2024-06-21T14:16:41Z

Any update?

Sorry, it's been a busy day. I'll take care of it tomorrow.

xsShuang · 2024-06-22T05:14:56Z

Please follow other comments to fix these

GHA control file needs to add this new test case.

Docs should be updated. changes.md and supported-list.md

Verification version list.

I have submitted the appropriate documents as per your instructions, please help me to confirm whether it is correct or not, thank you!

wu-sheng · 2024-06-22T05:18:57Z

Resolve the conflicts, then I could make CI works to verify.

# Conflicts: # CHANGES.md

wu-sheng · 2024-06-22T07:57:18Z

Please run tests locally, rather than costing us too much on running repeatedly.

wu-sheng · 2024-06-22T07:59:50Z

test/plugin/scenarios/solon-2.x-scenario/support-version.list

+2.7.0
+2.7.1
+2.7.2
+2.7.3
+2.7.4
+2.7.5
+2.7.6
+2.8.0
+2.8.1
+2.8.2
+2.8.3
+2.8.4


I didn't follow my comments. 2.7(per minor version) and 2.8 should keep one version only.

Sorry, my English is really bad and I have to rely on translation software, which leads to a lot of places that I can't fully understand.

CN slack is there. If you have doubts, ask there.

wu-sheng · 2024-06-22T08:01:11Z

test/plugin/scenarios/solon-2.x-scenario/config/expectedData.yaml

+      - {key: url, value: not null}
+      - {key: http.method, value: GET}
+      skipAnalysis: 'false'
+    - operationName: 'Solon:GET:/testcase/error'


None of entry spans have reference verifications, this test scenarios can't prove the tracing continues.

Solon is a RPC framework AFAIK, your test doesn't cover an important part.

Sorry, I can't fully understand the meaning of automated test scripts. I wrote this script with reference to spring. From the results I get from running it, I think I can get the link tracing information

If no reference, I can't see how it links.

You should check the segments of traces, and understand the data.

As I have explained, we don't maintain the plugins, but host for you(for example). After this, I can't ask you every time when someone submitted an issue/discussion. So, the best way is, test scenarios themselves can prove it works as expected.

docs/en/setup/service-agent/java-agent/Supported-list.md

wu-sheng · 2024-06-28T03:39:00Z

apm-sniffer/config/agent.config

@@ -320,3 +320,15 @@ plugin.nettyhttp.supported_content_types_prefix=${SW_PLUGIN_NETTYHTTP_SUPPORTED_
 plugin.rocketmqclient.collect_message_keys=${SW_PLUGIN_ROCKETMQCLIENT_COLLECT_MESSAGE_KEYS:false}
 # If set to true, the tags of messages would be collected by the plugin for RocketMQ Java client.
 plugin.rocketmqclient.collect_message_tags=${SW_PLUGIN_ROCKETMQCLIENT_COLLECT_MESSAGE_TAGS:false}
+# How many parameter characters to keep and send to the OAP backend, -1 save all, 0 save nothing, >0 save specified length of parameter string to the tag. default is 0.
+plugin.solon.http_params_length_threshold=${SW_PLUGIN_SOLON_HTTP_PARAMS_LENGTH_THRESHOLD:0}
+# How many header characters to keep and send to the OAP backend, -1 save all, 0 save nothing, >0 save specified length of header string to the tag. default is 0.


The comments/descriptions should be polished. Refer to the above one.

wu-sheng · 2024-06-28T03:40:47Z

...ugin/src/main/java/org/apache/skywalking/apm/plugin/solon/SolonActionExecuteInterceptor.java

@@ -97,7 +82,7 @@ public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allAr
    public Object afterMethod(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes,
                              Object ret) {
        Context ctx = (Context) allArguments[0];
-        if (afterExceptionHandling) {
+        if (SolonPluginConfig.Plugin.Solon.AFTER_EXCEPTION_HANDLING) {


Why do you accept the error unprocessed? I think that is far away of the purpose of tracing. Error codes are important.

I have no control over when the user handles exceptions

My question is about this config. Why do we need this config?

This configuration is where I need the user to tell me if he handled the exception, and if he did, I'll set up exception logging for him and use the status code after he handled it

If you want to process that, why don't you create an interceptor for filter(s)? It should be easy too.

I spoke with the solon author and he thinks that adding a filter customised for tracking doesn't make sense in the framework and that there should be plugins for that. But I want to make tracking available to users using solon without having to change any code.

Add plugin configuration and instructions

wu-sheng · 2024-06-28T05:57:21Z

apm-sniffer/config/agent.config

+# Define the max length of collected HTTP header. The default value(=0) means not collecting.
+plugin.solon.http_headers_length_threshold=${SW_PLUGIN_SOLON_HTTP_HEADERS_LENGTH_THRESHOLD:0}


This config field doesn't exist in the codes. Why it is here?

wu-sheng · 2024-06-28T05:58:20Z

apm-sniffer/config/agent.config

+# It controls what header data should be collected, values must be in lower case, if empty, collect all. default is empty.
+plugin.solon.include_http_headers=${SW_PLUGIN_SOLON_INCLUDE_HTTP_HEADERS:}


Again, you should not collect all headers. All collected things should be very specific.

Header usually carries auth token, this is auto collecting is dangerous and could cause sensitive data leak.

wu-sheng · 2024-06-28T05:59:34Z

apm-sniffer/config/agent.config

+# Intercept method name, default is invoke
+plugin.solon.intercept_method_name=${SW_PLUGIN_SOLON_INTERCEPT_METHOD_NAME:invoke}


Inceptor method should not be defined by the configurations. This is a kind of config abuse.

wu-sheng · 2024-06-28T06:00:04Z

docs/en/setup/service-agent/java-agent/configurations.md

-| `plugin.jedis.trace_redis_parameters`                           | If set to true, the parameters of Redis commands would be collected by Jedis agent.                                                                                                                                                                                                                                                                                                                                                                                                                                                                    | SW_PLUGIN_JEDIS_TRACE_REDIS_PARAMETERS                           | `false`                                                                                                                                                                                                                                                                                                                                                                                                                                              |
-| `plugin.jedis.redis_parameter_max_length`                       | If set to positive number and `plugin.jedis.trace_redis_parameters` is set to `true`, Redis command parameters would be collected and truncated to this length.                                                                                                                                                                                                                                                                                                                                                                                        | SW_PLUGIN_JEDIS_REDIS_PARAMETER_MAX_LENGTH                       | `128`                                                                                                                                                                                                                                                                                                                                                                                                                                                |
-| `plugin.jedis.operation_mapping_write`                          | Specify which command should be converted to `write` operation                                                                                                                                                                                                                                                                                                                                                                                                                                                                                         | SW_PLUGIN_JEDIS_OPERATION_MAPPING_WRITE                          |                                                                                                                                                                                                                                                                                                                                                                                                                                                      |
+| property key                                                    | Description                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                            | **System Environment Variable**                                  | Default                                                                                                                                                                                                                                                                                                                                                                                                                                            |


You are changing the whole file? Please don't do this.

wu-sheng · 2024-06-28T06:01:25Z

...solon-2.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/solon/SolonPluginConfig.java

+            /**
+             * intercept method name, default is invoke
+             */
+            public static String INTERCEPT_METHOD_NAME = "invoke";
+            /**
+             * is after exception handling, default is false, if true, the plugin will intercept the method after the exception handling
+             */


These two don't make sense. And the comments are not helpful.
Even you are not good at English, there are many AI tools could help you polish that.

Besides the comments, I am highly concerned about these two configurations. Please explain more clearly.
I never saw a framework plugin did this. The error code process should be very simple and straight forward.

Add plugin configuration and instructions

...ugin/src/main/java/org/apache/skywalking/apm/plugin/solon/SolonActionExecuteInterceptor.java

wu-sheng · 2024-06-28T08:23:05Z

Please use v2 plugin APIs for better performance, and don't use runtime context to collaborate inside an interceptor.

Modified to V2 version of the interceptor

Remove the code that collects all headers by default

wu-sheng · 2024-06-28T13:02:48Z

...ugin/src/main/java/org/apache/skywalking/apm/plugin/solon/SolonActionExecuteInterceptor.java

+            next = next.next();
+            next.setHeadValue(ctx.header(next.getHeadKey()));
+        }
+        String operationName = "Solon:" + ctx.method() + ":" + ctx.path();


Suggested change

String operationName = "Solon:" + ctx.method() + ":" + ctx.path();

String operationName = ctx.method() + ":" + ctx.path();

The operation name of the span should be framework stack irrelevant. And you need to change test cases accordingly.

...ugin/src/main/java/org/apache/skywalking/apm/plugin/solon/SolonActionExecuteInterceptor.java

wu-sheng · 2024-06-28T13:04:22Z

...ugin/src/main/java/org/apache/skywalking/apm/plugin/solon/SolonActionExecuteInterceptor.java

+            if (SolonPluginConfig.Plugin.Solon.HTTP_HEADERS_LENGTH_THRESHOLD > 0 && headerStr.length() > SolonPluginConfig.Plugin.Solon.HTTP_HEADERS_LENGTH_THRESHOLD) {
+                headerStr = headerStr.substring(0, SolonPluginConfig.Plugin.Solon.HTTP_HEADERS_LENGTH_THRESHOLD);


You should control header by key list, because every key has a length limitation by HTTP. Once the number of keys is limited, we don't need to worry about overload.

Dev

wu-sheng

lgtm, thanks for your patience.

Feat: Add a plugin that supports the Solon framework.

be675c2

xsShuang mentioned this pull request Jun 20, 2024

Add support for the Solon framework apache/skywalking#12347

Merged

Update ComponentsDefine.java

06732e8

wu-sheng reviewed Jun 20, 2024

View reviewed changes

wu-sheng added plugin feature labels Jun 20, 2024

wu-sheng self-requested a review June 20, 2024 04:26

xsShuang added 2 commits June 20, 2024 16:02

Add Solon plugin test

924c61f

Merge remote-tracking branch 'origin/main'

ffd56db

wu-sheng reviewed Jun 20, 2024

View reviewed changes

...ugin/src/main/java/org/apache/skywalking/apm/plugin/solon/SolonActionExecuteInterceptor.java Show resolved Hide resolved

Add Solon plugin test

dbd24b4

Add Solon plugin test and update doc.

0b2afff

xsShuang added 4 commits June 22, 2024 13:30

Merge remote-tracking branch 'origin/main'

b96e8c5

# Conflicts: # CHANGES.md

merge the latest commits.

88ef3ca

Modify the test code.

0716d24

Modify the test code.

1704f4d

wu-sheng reviewed Jun 22, 2024

View reviewed changes

wu-sheng reviewed Jun 28, 2024

View reviewed changes

xsShuang added 2 commits June 28, 2024 12:30

Add plugin configuration and instructions

4b5e558

Merge pull request #5 from xsShuang/dev

5fbdc2a

Add plugin configuration and instructions

wu-sheng reviewed Jun 28, 2024

View reviewed changes

xsShuang added 2 commits June 28, 2024 15:00

Add plugin configuration and instructions

4982488

Merge pull request #6 from xsShuang/dev

916dc21

Add plugin configuration and instructions

wu-sheng reviewed Jun 28, 2024

View reviewed changes

...ugin/src/main/java/org/apache/skywalking/apm/plugin/solon/SolonActionExecuteInterceptor.java Show resolved Hide resolved

xsShuang added 3 commits June 28, 2024 16:16

Modified to V2 version of the interceptor

505e99c

Modified to V2 version of the interceptor

115727f

Modified to V2 version of the interceptor

24799a6

xsShuang added 4 commits June 28, 2024 17:07

Merge pull request #7 from xsShuang/dev

22a4a8f

Modified to V2 version of the interceptor

Modified to V2 version of the interceptor

3524717

Remove the code that collects all headers by default

160668c

Merge pull request #8 from xsShuang/dev

08bdfcb

Remove the code that collects all headers by default

wu-sheng reviewed Jun 28, 2024

View reviewed changes

...ugin/src/main/java/org/apache/skywalking/apm/plugin/solon/SolonActionExecuteInterceptor.java Show resolved Hide resolved

wu-sheng reviewed Jun 28, 2024

View reviewed changes

xsShuang added 4 commits June 28, 2024 22:46

Optimised code

38cb7b1

Modify the test code.

193b532

Modify the test code.

20c3545

Merge pull request #9 from xsShuang/dev

9f3392e

Dev

wu-sheng approved these changes Jun 29, 2024

View reviewed changes

wu-sheng added this to the 9.3.0 milestone Jun 29, 2024

wu-sheng merged commit 0fc3cd8 into apache:main Jun 29, 2024
191 checks passed


		package org.apache.skywalking.apm.plugin.solon;

		public class FastPathMatcher {

+.7.0
+.7.1
+.7.2
+.7.3
+.7.4
+.7.5
+.7.6
+.8.0
+.8.1
+.8.2
+.8.3
+.8.4

		# Define the max length of collected HTTP header. The default value(=0) means not collecting.
		plugin.solon.http_headers_length_threshold=${SW_PLUGIN_SOLON_HTTP_HEADERS_LENGTH_THRESHOLD:0}

		# It controls what header data should be collected, values must be in lower case, if empty, collect all. default is empty.
		plugin.solon.include_http_headers=${SW_PLUGIN_SOLON_INCLUDE_HTTP_HEADERS:}

		# Intercept method name, default is invoke
		plugin.solon.intercept_method_name=${SW_PLUGIN_SOLON_INTERCEPT_METHOD_NAME:invoke}

	String operationName = "Solon:" + ctx.method() + ":" + ctx.path();
	String operationName = ctx.method() + ":" + ctx.path();

		if (SolonPluginConfig.Plugin.Solon.HTTP_HEADERS_LENGTH_THRESHOLD > 0 && headerStr.length() > SolonPluginConfig.Plugin.Solon.HTTP_HEADERS_LENGTH_THRESHOLD) {
		headerStr = headerStr.substring(0, SolonPluginConfig.Plugin.Solon.HTTP_HEADERS_LENGTH_THRESHOLD);

Add a plugin that supports the Solon framework. #697

Add a plugin that supports the Solon framework. #697

Conversation

xsShuang commented Jun 20, 2024 • edited Loading

Add an agent plugin to support

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

wu-sheng commented Jun 20, 2024

xsShuang commented Jun 20, 2024

wu-sheng commented Jun 20, 2024

wu-sheng commented Jun 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xsShuang commented Jun 20, 2024

wu-sheng commented Jun 20, 2024

wu-sheng commented Jun 21, 2024

xsShuang commented Jun 21, 2024

xsShuang commented Jun 22, 2024

wu-sheng commented Jun 22, 2024

wu-sheng commented Jun 22, 2024

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

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

Choose a reason for hiding this comment

wu-sheng commented Jun 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wu-sheng left a comment

Choose a reason for hiding this comment

xsShuang commented Jun 20, 2024 •

edited

Loading