You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by "zhaoxiaojie0415 (via GitHub)" <gi...@apache.org> on 2023/03/02 11:53:15 UTC
[GitHub] [skywalking-java] zhaoxiaojie0415 opened a new pull request, #467: Fix thrift plugin generate duplicate traceid
zhaoxiaojie0415 opened a new pull request, #467:
URL: https://github.com/apache/skywalking-java/pull/467
<!--
⚠️ Please make sure to read this template first, pull requests that don't accord with this template
maybe closed without notice.
Texts surrounded by `<` and `>` are meant to be replaced by you, e.g. <framework name>, <issue number>.
Put an `x` in the `[ ]` to mark the item as CHECKED. `[x]`
-->
### Fix thrift plugin would generate duplicate traceid when an error occurs in sendBase phase
In thrift plugin，it generate span before sendBase method and stop this span after receiveBase. However, if an error occurs in sendBase method, this thrift-client span will not be stopped, which leads to the tracecontext can not be cleaned properly. Therefore, we stop the span in the handldException method around sendBase method.
--
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-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng merged pull request #467: Fix thrift plugin generate duplicate traceid
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng merged PR #467:
URL: https://github.com/apache/skywalking-java/pull/467
--
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-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] zhaoxiaojie0415 commented on a diff in pull request #467: Fix thrift plugin generate duplicate traceid
Posted by "zhaoxiaojie0415 (via GitHub)" <gi...@apache.org>.
zhaoxiaojie0415 commented on code in PR #467:
URL: https://github.com/apache/skywalking-java/pull/467#discussion_r1125612984
##########
test/plugin/scenarios/thrift-scenario/support-version.list:
##########
@@ -17,3 +17,4 @@
0.12.0
0.11.0
0.10.0
+0.9.3
Review Comment:
> Generally, as 0.9.3 is a old version, we don't have to put that in the test list, if others can pass.
>
> > I encouter a maven wrapper problem in local test env
>
> You may need a proxy, sometimes the network doesn't work.
Thanks. I remove the 0.9.3 in the latest submit and other cases can pass locally. Between 0.9.3 and 0.10.0, there has some difference for async method(for example, the interface of 0.9.3 generic type erasure， while 0.10.0 not), which leads to the test case fail.
--
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-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] zhaoxiaojie0415 commented on a diff in pull request #467: Fix thrift plugin generate duplicate traceid
Posted by "zhaoxiaojie0415 (via GitHub)" <gi...@apache.org>.
zhaoxiaojie0415 commented on code in PR #467:
URL: https://github.com/apache/skywalking-java/pull/467#discussion_r1124358315
##########
test/plugin/scenarios/thrift-scenario/support-version.list:
##########
@@ -17,3 +17,4 @@
0.12.0
0.11.0
0.10.0
+0.9.3
Review Comment:
> @zhaoxiaojie0415 Is there any update about this?
In thrift 0.9.3, it generate the interface with the param Generic erase which leads to test case fail.
Is it appropriate to write two versions of test cases? （Actually 0.9.3 and 0.10.0 use same plugin code, but need to use different test cases）. However, this will generate two support-version.list files.
--
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-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on a diff in pull request #467: Fix thrift plugin generate duplicate traceid
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #467:
URL: https://github.com/apache/skywalking-java/pull/467#discussion_r1124198463
##########
test/plugin/scenarios/thrift-scenario/support-version.list:
##########
@@ -17,3 +17,4 @@
0.12.0
0.11.0
0.10.0
+0.9.3
Review Comment:
@zhaoxiaojie0415 Is there any update about this?
--
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-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on pull request #467: Fix thrift plugin generate duplicate traceid
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #467:
URL: https://github.com/apache/skywalking-java/pull/467#issuecomment-1453442829
```
2023-03-03T11:53:18.5792938Z java.lang.ClassCastException: org.apache.skywalking.apm.testcase.thrift.protocol.GreeterService$AsyncClient$echo_call cannot be cast to java.lang.String
2023-03-03T11:53:18.5793776Z at org.apache.skywalking.apm.testcase.thrift.client.service.AsyncClient$1.onComplete(AsyncClient.java:60) [classes!/:?]
2023-03-03T11:53:18.5794541Z at org.apache.skywalking.apm.plugin.thrift.client.TAsyncMethodCallInterceptor$1.onComplete(TAsyncMethodCallInterceptor.java:53) [?:?]
2023-03-03T11:53:18.5795453Z at org.apache.thrift.async.TAsyncMethodCall.cleanUpAndFireCallback(TAsyncMethodCall.java:229) ~[libthrift-0.9.3.jar!/:0.9.3]
2023-03-03T11:53:18.5796303Z at org.apache.thrift.async.TAsyncMethodCall.doReadingResponseBody(TAsyncMethodCall.java:219) [libthrift-0.9.3.jar!/:0.9.3]
2023-03-03T11:53:18.5797065Z at org.apache.thrift.async.TAsyncMethodCall.transition(TAsyncMethodCall.java:195) [libthrift-0.9.3.jar!/:0.9.3]
2023-03-03T11:53:18.5797867Z at org.apache.thrift.async.TAsyncClientManager$SelectThread.transitionMethods(TAsyncClientManager.java:143) [libthrift-0.9.3.jar!/:0.9.3]
2023-03-03T11:53:18.5798892Z at org.apache.thrift.async.TAsyncClientManager$SelectThread.run(TAsyncClientManager.java:113) [libthrift-0.9.3.jar!/:0.9.3]
2023-03-03T11:53:18.5799788Z [2023-03-03 11:53:09:870] [ERROR] - org.apache.thrift.async.TAsyncClientManager$SelectThread.run(TAsyncClientManager.java:117) - Ignoring uncaught exception in SelectThread
2023-03-03T11:53:18.5800392Z java.lang.RuntimeException: Can not do async finish for the span repeatedly.
2023-03-03T11:53:18.5801210Z at org.apache.skywalking.apm.agent.core.context.trace.AbstractTracingSpan.asyncFinish(AbstractTracingSpan.java:332) ~[skywalking-agent.jar:8.15.0-SNAPSHOT]
2023-03-03T11:53:18.5802272Z at org.apache.skywalking.apm.plugin.thrift.client.TAsyncMethodCallInterceptor$1.onError(TAsyncMethodCallInterceptor.java:61) ~[?:?]
2023-03-03T11:53:18.5803095Z at org.apache.thrift.async.TAsyncMethodCall.onError(TAsyncMethodCall.java:210) ~[libthrift-0.9.3.jar!/:0.9.3]
2023-03-03T11:53:18.5803828Z at org.apache.thrift.async.TAsyncMethodCall.transition(TAsyncMethodCall.java:204) ~[libthrift-0.9.3.jar!/:0.9.3]
2023-03-03T11:53:18.5805562Z at org.apache.thrift.async.TAsyncClientManager$SelectThread.transitionMethods(TAsyncClientManager.java:143) ~[libthrift-0.9.3.jar!/:0.9.3]
2023-03-03T11:53:18.5827973Z at org.apache.thrift.async.TAsyncClientManager$SelectThread.run(TAsyncClientManager.java:113) [libthrift-0.9.3.jar!/:0.9.3]
2023-03-03T11:53:18.5828788Z To receive actual data
```
Please make sure you have tested locally.
--
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-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] zhaoxiaojie0415 commented on a diff in pull request #467: Fix thrift plugin generate duplicate traceid
Posted by "zhaoxiaojie0415 (via GitHub)" <gi...@apache.org>.
zhaoxiaojie0415 commented on code in PR #467:
URL: https://github.com/apache/skywalking-java/pull/467#discussion_r1124358315
##########
test/plugin/scenarios/thrift-scenario/support-version.list:
##########
@@ -17,3 +17,4 @@
0.12.0
0.11.0
0.10.0
+0.9.3
Review Comment:
> @zhaoxiaojie0415 Is there any update about this?
In thrift 0.9.3, it generate the interface with the param Generic erase which leads to test case fail.
Is it appropriate to write two versions of test cases? （Actually 0.9.3 and 0.10.0 use same plugin code, but need to use different test cases）
--
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-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on a diff in pull request #467: Fix thrift plugin generate duplicate traceid
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #467:
URL: https://github.com/apache/skywalking-java/pull/467#discussion_r1123054937
##########
test/plugin/scenarios/thrift-scenario/support-version.list:
##########
@@ -17,3 +17,4 @@
0.12.0
0.11.0
0.10.0
+0.9.3
Review Comment:
From the test log, https://pipelines.actions.githubusercontent.com/serviceHosts/c62bd4a6-a619-4835-aa78-99798d940fc7/_apis/pipelines/1/runs/13668/signedlogcontent/18?urlExpires=2023-03-02T12%3A54%3A46.4948869Z&urlSigningMethod=HMACV1&urlSignature=nKo%2FKZeAMBo7y2mS0i%2Fl80EobjyjEsN7ami57gKjrao%3D
This test scenario can't be compiled in 0.9.3. Please check locally.
--
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-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] zhaoxiaojie0415 commented on a diff in pull request #467: Fix thrift plugin generate duplicate traceid
Posted by "zhaoxiaojie0415 (via GitHub)" <gi...@apache.org>.
zhaoxiaojie0415 commented on code in PR #467:
URL: https://github.com/apache/skywalking-java/pull/467#discussion_r1124358315
##########
test/plugin/scenarios/thrift-scenario/support-version.list:
##########
@@ -17,3 +17,4 @@
0.12.0
0.11.0
0.10.0
+0.9.3
Review Comment:
> @zhaoxiaojie0415 Is there any update about this?
In thrift 0.9.3, it generate the interface with the param Generic erase which leads to test case fail.
Is it appropriate to write two versions of test cases? （Actually 0.9.3 and 0.10.0 use same plugin code, but need to use different test cases）. However, this will generate two support-version.list files.
--
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-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] zhaoxiaojie0415 commented on a diff in pull request #467: Fix thrift plugin generate duplicate traceid
Posted by "zhaoxiaojie0415 (via GitHub)" <gi...@apache.org>.
zhaoxiaojie0415 commented on code in PR #467:
URL: https://github.com/apache/skywalking-java/pull/467#discussion_r1124563265
##########
test/plugin/scenarios/thrift-scenario/support-version.list:
##########
@@ -17,3 +17,4 @@
0.12.0
0.11.0
0.10.0
+0.9.3
Review Comment:
Sorry, I encouter a maven wrapper problem in local test env which can not package test/plugin/pom.xml and then can not run test cases locally. It would take me more time to solve it.
--
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-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on a diff in pull request #467: Fix thrift plugin generate duplicate traceid
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #467:
URL: https://github.com/apache/skywalking-java/pull/467#discussion_r1124585084
##########
test/plugin/scenarios/thrift-scenario/support-version.list:
##########
@@ -17,3 +17,4 @@
0.12.0
0.11.0
0.10.0
+0.9.3
Review Comment:
Generally, as 0.9.3 is a old version, we don't have to put that in the test list, if others can pass.
> I encouter a maven wrapper problem in local test env
You may need a proxy, sometimes the network doesn't work.
--
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-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] zhaoxiaojie0415 commented on a diff in pull request #467: Fix thrift plugin generate duplicate traceid
Posted by "zhaoxiaojie0415 (via GitHub)" <gi...@apache.org>.
zhaoxiaojie0415 commented on code in PR #467:
URL: https://github.com/apache/skywalking-java/pull/467#discussion_r1125612984
##########
test/plugin/scenarios/thrift-scenario/support-version.list:
##########
@@ -17,3 +17,4 @@
0.12.0
0.11.0
0.10.0
+0.9.3
Review Comment:
> Generally, as 0.9.3 is a old version, we don't have to put that in the test list, if others can pass.
>
> > I encouter a maven wrapper problem in local test env
>
> You may need a proxy, sometimes the network doesn't work.
Thanks. I remove the 0.9.3 in the latest submit. Between 0.9.3 and 0.10.0, there has some difference for async method(for example, the interface of 0.9.3 generic type erasure， while 0.10.0 not), which leads to the test case fail.
--
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-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] zhaoxiaojie0415 commented on pull request #467: Fix thrift plugin generate duplicate traceid
Posted by "zhaoxiaojie0415 (via GitHub)" <gi...@apache.org>.
zhaoxiaojie0415 commented on PR #467:
URL: https://github.com/apache/skywalking-java/pull/467#issuecomment-1451749914
https://github.com/apache/skywalking/issues/10438
--
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-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] lujiajing1126 commented on pull request #467: Fix thrift plugin generate duplicate traceid
Posted by "lujiajing1126 (via GitHub)" <gi...@apache.org>.
lujiajing1126 commented on PR #467:
URL: https://github.com/apache/skywalking-java/pull/467#issuecomment-1452928949
> In thrift plugin，it generate span before sendBase method and stop this span after receiveBase. However, if an error occurs in sendBase method, this thrift-client span will not be stopped, which leads to the tracecontext can not be cleaned properly. Therefore, we stop the span in the handldException method around sendBase method.
If so, we may choose an improper interception point.
I'm not familiar with thrift. But If you stop the span in the `handleMethodException`, can you make sure this span will not be stopped again in the after hook of the so-called `receiveBase` method. If the span is closed twice, it may cause undefined behavior and thus break the full stacked-base tracing subsystem.
--
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-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] zhaoxiaojie0415 commented on pull request #467: Fix thrift plugin generate duplicate traceid
Posted by "zhaoxiaojie0415 (via GitHub)" <gi...@apache.org>.
zhaoxiaojie0415 commented on PR #467:
URL: https://github.com/apache/skywalking-java/pull/467#issuecomment-1452961500
> > In thrift plugin，it generate span before sendBase method and stop this span after receiveBase. However, if an error occurs in sendBase method, this thrift-client span will not be stopped, which leads to the tracecontext can not be cleaned properly. Therefore, we stop the span in the handldException method around sendBase method.
>
> If so, we may choose an improper interception point in the current impl.
>
> I'm not familiar with thrift. But If you stop the span in the `handleMethodException`, can you make sure this span will not be stopped again in the after hook of the so-called `receiveBase` method. If the span is closed twice, it may cause undefined behavior and thus break the tracing stack.
If an exception has been thrown by sendBase method，this thrift client request would not call receiveBase subsequently.
For example, after thrift compile, the java file may be as follows
<img width="941" alt="企业微信截图_c9ac35cd-41b3-45cb-bfb5-55b2587520e9" src="https://user-images.githubusercontent.com/13363629/222633108-3412c3a2-df7f-45f8-953c-a300cb7dc5e4.png">
<img width="704" alt="企业微信截图_d8fe5176-4a00-4f0d-b37c-a8612fa3dbd8" src="https://user-images.githubusercontent.com/13363629/222633115-af66f1e1-76c4-4a1c-86a8-6194aa5302ea.png">
--
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-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org