You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by GitBox <gi...@apache.org> on 2020/01/06 13:17:13 UTC

[GitHub] [skywalking] tzy1316106836 opened a new pull request #4187: make a suggestion about DUBBO plugin

tzy1316106836 opened a new pull request #4187: make a suggestion about DUBBO plugin
URL: https://github.com/apache/skywalking/pull/4187
 
 
   Please answer these questions before submitting pull request
   
   - Why submit this pull request?
   - [ ] Bug fix
   
   Dubbo plugin will have the following problems when retrying:
   If the current link is triggered by other components that are not monitored, and it does not have EntrySpan, the stackDepth of the link is 0 when the dubbo is called internally. The traceId generated by the call is different. however, if the dubbo call timeout and retry, the traceId is the same, because the sw3 parameter of the last call is reused. In this scenario, The effect of multiple dubbo calls and dubbo retry should be the same. I think the problem needs to be fixed, that is, update the value of sw3 when retrying.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [skywalking] tzy1316106836 commented on a change in pull request #4187: make a suggestion about DUBBO plugin

Posted by GitBox <gi...@apache.org>.
tzy1316106836 commented on a change in pull request #4187: make a suggestion about DUBBO plugin
URL: https://github.com/apache/skywalking/pull/4187#discussion_r363612894
 
 

 ##########
 File path: apm-sniffer/apm-sdk-plugin/dubbo-plugin/src/main/java/org/apache/skywalking/apm/plugin/dubbo/DubboInterceptor.java
 ##########
 @@ -56,6 +57,9 @@ public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allAr
         Class<?>[] argumentsTypes, MethodInterceptResult result) throws Throwable {
         Invoker invoker = (Invoker)allArguments[0];
         Invocation invocation = (Invocation)allArguments[1];
+        if (invocation.getAttachments().containsKey(SW3CarrierItem.HEADER_NAME)) {
+            invocation.getAttachments().remove(SW3CarrierItem.HEADER_NAME);
+        }
 
 Review comment:
   done

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [skywalking] tzy1316106836 commented on a change in pull request #4187: make a suggestion about DUBBO plugin

Posted by GitBox <gi...@apache.org>.
tzy1316106836 commented on a change in pull request #4187: make a suggestion about DUBBO plugin
URL: https://github.com/apache/skywalking/pull/4187#discussion_r363593344
 
 

 ##########
 File path: apm-sniffer/apm-sdk-plugin/dubbo-plugin/src/main/java/org/apache/skywalking/apm/plugin/dubbo/DubboInterceptor.java
 ##########
 @@ -56,6 +57,9 @@ public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allAr
         Class<?>[] argumentsTypes, MethodInterceptResult result) throws Throwable {
         Invoker invoker = (Invoker)allArguments[0];
         Invocation invocation = (Invocation)allArguments[1];
+        if (invocation.getAttachments().containsKey(SW3CarrierItem.HEADER_NAME)) {
+            invocation.getAttachments().remove(SW3CarrierItem.HEADER_NAME);
+        }
 
 Review comment:
   When dubbo retry, the parameter sw3 will not be updated. 

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [skywalking] kezhenxu94 merged pull request #4187: make a suggestion about DUBBO plugin

Posted by GitBox <gi...@apache.org>.
kezhenxu94 merged pull request #4187: make a suggestion about DUBBO plugin
URL: https://github.com/apache/skywalking/pull/4187
 
 
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [skywalking] kezhenxu94 commented on a change in pull request #4187: make a suggestion about DUBBO plugin

Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on a change in pull request #4187: make a suggestion about DUBBO plugin
URL: https://github.com/apache/skywalking/pull/4187#discussion_r363594229
 
 

 ##########
 File path: apm-sniffer/apm-sdk-plugin/dubbo-plugin/src/main/java/org/apache/skywalking/apm/plugin/dubbo/DubboInterceptor.java
 ##########
 @@ -56,6 +57,9 @@ public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allAr
         Class<?>[] argumentsTypes, MethodInterceptResult result) throws Throwable {
         Invoker invoker = (Invoker)allArguments[0];
         Invocation invocation = (Invocation)allArguments[1];
+        if (invocation.getAttachments().containsKey(SW3CarrierItem.HEADER_NAME)) {
+            invocation.getAttachments().remove(SW3CarrierItem.HEADER_NAME);
+        }
 
 Review comment:
   > When dubbo retry, the parameter sw3 will not be updated.
   
   @tzy1316106836 what @wu-sheng meant is that you should not simply use `SW3CarrierItem.HEADER_NAME` as key to remove the context, you should use something like：
   
   ```java
               CarrierItem next = contextCarrier.items();
               while (next.hasNext()) {
                   next = next.next();
                   rpcContext.getAttachments().remove(next.getHeadKey()); // <<<==== HERE
               }
   ```
   
   because there are chances that the users are using SkyWalking 6.x, which uses protocol v2 and the key is `SW6CarrierItem.HEADER_NAME` instead of `SW3CarrierItem.HEADER_NAME`, what's more, `SW3CarrierItem` will be removed soon
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [skywalking] tzy1316106836 commented on a change in pull request #4187: make a suggestion about DUBBO plugin

Posted by GitBox <gi...@apache.org>.
tzy1316106836 commented on a change in pull request #4187: make a suggestion about DUBBO plugin
URL: https://github.com/apache/skywalking/pull/4187#discussion_r363593847
 
 

 ##########
 File path: apm-sniffer/apm-sdk-plugin/dubbo-plugin/src/main/java/org/apache/skywalking/apm/plugin/dubbo/DubboInterceptor.java
 ##########
 @@ -56,6 +57,9 @@ public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allAr
         Class<?>[] argumentsTypes, MethodInterceptResult result) throws Throwable {
         Invoker invoker = (Invoker)allArguments[0];
         Invocation invocation = (Invocation)allArguments[1];
+        if (invocation.getAttachments().containsKey(SW3CarrierItem.HEADER_NAME)) {
+            invocation.getAttachments().remove(SW3CarrierItem.HEADER_NAME);
+        }
 
 Review comment:
   Therefore, we should clear sw3 in the invocation first,  so that the latest sw3 will be placed according to the RpcContext

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [skywalking] codecov-io commented on issue #4187: make a suggestion about DUBBO plugin

Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #4187: make a suggestion about DUBBO plugin
URL: https://github.com/apache/skywalking/pull/4187#issuecomment-571144769
 
 
   # [Codecov](https://codecov.io/gh/apache/skywalking/pull/4187?src=pr&el=h1) Report
   > :exclamation: No coverage uploaded for pull request base (`master@d5ab0cf`). [Click here to learn what that means](https://docs.codecov.io/docs/error-reference#section-missing-base-commit).
   > The diff coverage is `43.62%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/skywalking/pull/4187/graphs/tree.svg?width=650&token=qrILxY5yA8&height=150&src=pr)](https://codecov.io/gh/apache/skywalking/pull/4187?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff            @@
   ##             master    #4187   +/-   ##
   =========================================
     Coverage          ?   26.84%           
   =========================================
     Files             ?     1163           
     Lines             ?    25426           
     Branches          ?     3691           
   =========================================
     Hits              ?     6826           
     Misses            ?    17991           
     Partials          ?      609
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/skywalking/pull/4187?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [...ettuce/v5/ClientOptionsConstructorInterceptor.java](https://codecov.io/gh/apache/skywalking/pull/4187/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLXNkay1wbHVnaW4vbGV0dHVjZS01LngtcGx1Z2luL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9za3l3YWxraW5nL2FwbS9wbHVnaW4vbGV0dHVjZS92NS9DbGllbnRPcHRpb25zQ29uc3RydWN0b3JJbnRlcmNlcHRvci5qYXZh) | `0% <ø> (ø)` | |
   | [...skywalking/apm/plugin/lettuce/v5/SWBiConsumer.java](https://codecov.io/gh/apache/skywalking/pull/4187/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLXNkay1wbHVnaW4vbGV0dHVjZS01LngtcGx1Z2luL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9za3l3YWxraW5nL2FwbS9wbHVnaW4vbGV0dHVjZS92NS9TV0JpQ29uc3VtZXIuamF2YQ==) | `0% <ø> (ø)` | |
   | [...ugin/lettuce/v5/RedisChannelWriterInterceptor.java](https://codecov.io/gh/apache/skywalking/pull/4187/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLXNkay1wbHVnaW4vbGV0dHVjZS01LngtcGx1Z2luL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9za3l3YWxraW5nL2FwbS9wbHVnaW4vbGV0dHVjZS92NS9SZWRpc0NoYW5uZWxXcml0ZXJJbnRlcmNlcHRvci5qYXZh) | `62.06% <ø> (ø)` | |
   | [...ugin/lettuce/v5/AsyncCommandMethodInterceptor.java](https://codecov.io/gh/apache/skywalking/pull/4187/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLXNkay1wbHVnaW4vbGV0dHVjZS01LngtcGx1Z2luL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9za3l3YWxraW5nL2FwbS9wbHVnaW4vbGV0dHVjZS92NS9Bc3luY0NvbW1hbmRNZXRob2RJbnRlcmNlcHRvci5qYXZh) | `0% <ø> (ø)` | |
   | [...apm/commons/datacarrier/buffer/BufferStrategy.java](https://codecov.io/gh/apache/skywalking/pull/4187/diff?src=pr&el=tree#diff-YXBtLWNvbW1vbnMvYXBtLWRhdGFjYXJyaWVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9za3l3YWxraW5nL2FwbS9jb21tb25zL2RhdGFjYXJyaWVyL2J1ZmZlci9CdWZmZXJTdHJhdGVneS5qYXZh) | `100% <ø> (ø)` | |
   | [.../lettuce/v5/RedisClientConstructorInterceptor.java](https://codecov.io/gh/apache/skywalking/pull/4187/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLXNkay1wbHVnaW4vbGV0dHVjZS01LngtcGx1Z2luL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9za3l3YWxraW5nL2FwbS9wbHVnaW4vbGV0dHVjZS92NS9SZWRpc0NsaWVudENvbnN0cnVjdG9ySW50ZXJjZXB0b3IuamF2YQ==) | `0% <ø> (ø)` | |
   | [...kywalking/apm/commons/datacarrier/DataCarrier.java](https://codecov.io/gh/apache/skywalking/pull/4187/diff?src=pr&el=tree#diff-YXBtLWNvbW1vbnMvYXBtLWRhdGFjYXJyaWVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9za3l3YWxraW5nL2FwbS9jb21tb25zL2RhdGFjYXJyaWVyL0RhdGFDYXJyaWVyLmphdmE=) | `72.97% <ø> (ø)` | |
   | [...lkit/log/logback/v1/x/LogbackPatternConverter.java](https://codecov.io/gh/apache/skywalking/pull/4187/diff?src=pr&el=tree#diff-YXBtLWFwcGxpY2F0aW9uLXRvb2xraXQvYXBtLXRvb2xraXQtbG9nYmFjay0xLngvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL3Rvb2xraXQvbG9nL2xvZ2JhY2svdjEveC9Mb2diYWNrUGF0dGVybkNvbnZlcnRlci5qYXZh) | `0% <ø> (ø)` | |
   | [...walking/apm/commons/datacarrier/buffer/Buffer.java](https://codecov.io/gh/apache/skywalking/pull/4187/diff?src=pr&el=tree#diff-YXBtLWNvbW1vbnMvYXBtLWRhdGFjYXJyaWVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9za3l3YWxraW5nL2FwbS9jb21tb25zL2RhdGFjYXJyaWVyL2J1ZmZlci9CdWZmZXIuamF2YQ==) | `0% <ø> (ø)` | |
   | [...e/skywalking/apm/plugin/lettuce/v5/SWConsumer.java](https://codecov.io/gh/apache/skywalking/pull/4187/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLXNkay1wbHVnaW4vbGV0dHVjZS01LngtcGx1Z2luL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9za3l3YWxraW5nL2FwbS9wbHVnaW4vbGV0dHVjZS92NS9TV0NvbnN1bWVyLmphdmE=) | `0% <ø> (ø)` | |
   | ... and [97 more](https://codecov.io/gh/apache/skywalking/pull/4187/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/skywalking/pull/4187?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/skywalking/pull/4187?src=pr&el=footer). Last update [d5ab0cf...09b1ad2](https://codecov.io/gh/apache/skywalking/pull/4187?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [skywalking] tzy1316106836 commented on issue #4187: make a suggestion about DUBBO plugin

Posted by GitBox <gi...@apache.org>.
tzy1316106836 commented on issue #4187: make a suggestion about DUBBO plugin
URL: https://github.com/apache/skywalking/pull/4187#issuecomment-571395434
 
 
   > @tzy1316106836 fix failed CI first, your code style doesn't meet the code style rules, add a space before `{`
   
   done

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [skywalking] codecov-io edited a comment on issue #4187: make a suggestion about DUBBO plugin

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #4187: make a suggestion about DUBBO plugin
URL: https://github.com/apache/skywalking/pull/4187#issuecomment-571144769
 
 
   # [Codecov](https://codecov.io/gh/apache/skywalking/pull/4187?src=pr&el=h1) Report
   > Merging [#4187](https://codecov.io/gh/apache/skywalking/pull/4187?src=pr&el=desc) into [master](https://codecov.io/gh/apache/skywalking/commit/8e9a1121cdbbded4f70a38332e3b163b79e09400?src=pr&el=desc) will **increase** coverage by `<.01%`.
   > The diff coverage is `n/a`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/skywalking/pull/4187/graphs/tree.svg?width=650&token=qrILxY5yA8&height=150&src=pr)](https://codecov.io/gh/apache/skywalking/pull/4187?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master    #4187      +/-   ##
   ==========================================
   + Coverage   26.84%   26.84%   +<.01%     
   ==========================================
     Files        1163     1163              
     Lines       25426    25426              
     Branches     3691     3691              
   ==========================================
   + Hits         6825     6826       +1     
     Misses      17991    17991              
   + Partials      610      609       -1
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/skywalking/pull/4187?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [.../agent/core/context/trace/AbstractTracingSpan.java](https://codecov.io/gh/apache/skywalking/pull/4187/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29udGV4dC90cmFjZS9BYnN0cmFjdFRyYWNpbmdTcGFuLmphdmE=) | `61.26% <0%> (+0.9%)` | :arrow_up: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/skywalking/pull/4187?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/skywalking/pull/4187?src=pr&el=footer). Last update [8e9a112...b13fa59](https://codecov.io/gh/apache/skywalking/pull/4187?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [skywalking] kezhenxu94 commented on issue #4187: make a suggestion about DUBBO plugin

Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on issue #4187: make a suggestion about DUBBO plugin
URL: https://github.com/apache/skywalking/pull/4187#issuecomment-571145674
 
 
   @tzy1316106836 fix failed CI first, your code style doesn't meet the code style rules, add a space before `{`

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [skywalking] kezhenxu94 commented on a change in pull request #4187: make a suggestion about DUBBO plugin

Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on a change in pull request #4187: make a suggestion about DUBBO plugin
URL: https://github.com/apache/skywalking/pull/4187#discussion_r363594229
 
 

 ##########
 File path: apm-sniffer/apm-sdk-plugin/dubbo-plugin/src/main/java/org/apache/skywalking/apm/plugin/dubbo/DubboInterceptor.java
 ##########
 @@ -56,6 +57,9 @@ public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allAr
         Class<?>[] argumentsTypes, MethodInterceptResult result) throws Throwable {
         Invoker invoker = (Invoker)allArguments[0];
         Invocation invocation = (Invocation)allArguments[1];
+        if (invocation.getAttachments().containsKey(SW3CarrierItem.HEADER_NAME)) {
+            invocation.getAttachments().remove(SW3CarrierItem.HEADER_NAME);
+        }
 
 Review comment:
   > When dubbo retry, the parameter sw3 will not be updated.
   
   @tzy1316106836 what @wu-sheng meant is that you should not simply use `SW3CarrierItem.HEADER_NAME` as key to remove the context, you should use something like：
   
   ```java
               CarrierItem next = contextCarrier.items();
               while (next.hasNext()) {
                   next = next.next();
                   rpcContext.getAttachments().remove(next.getHeadKey());
               }
   ```
   
   because there are chances that the users are using SkyWalking 6.x, which uses protocol v2 and the key is `SW6CarrierItem.HEADER_NAME` instead of `SW3CarrierItem.HEADER_NAME`, what's more, `SW3CarrierItem` will be removed soon
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [skywalking] wu-sheng commented on a change in pull request #4187: make a suggestion about DUBBO plugin

Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4187: make a suggestion about DUBBO plugin
URL: https://github.com/apache/skywalking/pull/4187#discussion_r363566166
 
 

 ##########
 File path: apm-sniffer/apm-sdk-plugin/dubbo-plugin/src/main/java/org/apache/skywalking/apm/plugin/dubbo/DubboInterceptor.java
 ##########
 @@ -56,6 +57,9 @@ public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allAr
         Class<?>[] argumentsTypes, MethodInterceptResult result) throws Throwable {
         Invoker invoker = (Invoker)allArguments[0];
         Invocation invocation = (Invocation)allArguments[1];
+        if (invocation.getAttachments().containsKey(SW3CarrierItem.HEADER_NAME)) {
+            invocation.getAttachments().remove(SW3CarrierItem.HEADER_NAME);
+        }
 
 Review comment:
   Remove should be something similar than this
   
   ```
               CarrierItem next = contextCarrier.items();
               while (next.hasNext()) {
                   next = next.next();
                   rpcContext.getAttachments().put(next.getHeadKey(), next.getHeadValue());
               }
   ```
   
   Key is defined in there.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services