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/03/08 07:02:58 UTC

[GitHub] [skywalking] wu-sheng opened a new pull request #4462: [Agent Core] Support lazy ContextCarrier injection and lazy peer id setting 

wu-sheng opened a new pull request #4462: [Agent Core] Support lazy ContextCarrier injection and lazy peer id setting 
URL: https://github.com/apache/skywalking/pull/4462
 
 
   This PR's idea was beginning when I am reviewing @huangyoje 's PR #4441. He did hijacking the ContextCarrier, which is not supposed to happen. After the discussion, I think that plugin has to do lazy injection due to the framework limitation.
   
   Here is the proposal, right now, we officially support
   1. Create exit span with `null` as peer, if the user wouldn't do injection.
   1. Exit span could do `span#inject` directly, by leverage the span's internal tracing context ref.
   
   FYI @apache/skywalking-committers This would be the first API change(adding) after the async span feature. Please review.
   
   @huangyoje Please cross check this PR change, and give me the feedback whether this PR could remove your hijacking requirements.

----------------------------------------------------------------
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 edited a comment on issue #4462: [Agent Core] Support lazy ContextCarrier injection and lazy peer id setting

Posted by GitBox <gi...@apache.org>.
wu-sheng edited a comment on issue #4462: [Agent Core] Support lazy ContextCarrier injection and lazy peer id setting 
URL: https://github.com/apache/skywalking/pull/4462#issuecomment-596180895
 
 
   > return exitSpan.getOperatioName() != null && exitSpan.getPeer() != null;
   
   I prefer you use the plugin internal variables rather than using `exitSpan#get*`. @huangyoje 
   Because the peer name could be exchanged to peer id. So avoid of using that, use something you could control.

----------------------------------------------------------------
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 #4462: [Agent Core] Support lazy ContextCarrier injection and lazy peer id setting

Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4462: [Agent Core] Support lazy ContextCarrier injection and lazy peer id setting 
URL: https://github.com/apache/skywalking/pull/4462#discussion_r389344994
 
 

 ##########
 File path: apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/trace/ExitTypeSpan.java
 ##########
 @@ -18,8 +18,15 @@
 
 package org.apache.skywalking.apm.agent.core.context.trace;
 
-public interface WithPeerInfo {
+import org.apache.skywalking.apm.agent.core.context.ContextCarrier;
+
+/**
+ * The exit span has some additional behaviours
+ */
+public interface ExitTypeSpan {
     int getPeerId();
 
     String getPeer();
+
+    void inject(ContextCarrier carrier);
 
 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] wu-sheng commented on issue #4462: [Agent Core] Support lazy ContextCarrier injection and lazy peer id setting

Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #4462: [Agent Core] Support lazy ContextCarrier injection and lazy peer id setting 
URL: https://github.com/apache/skywalking/pull/4462#issuecomment-596178862
 
 
   > Wow, this PR is friendly to write any plugin on the async scenario. Before this, we usually need to waste much time to find the right point to enhance.
   
   @dmsolr Yes, that was the idea. But I would say just using this only if necessary. 

----------------------------------------------------------------
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 #4462: [Agent Core] Support lazy ContextCarrier injection and lazy peer id setting

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #4462: [Agent Core] Support lazy ContextCarrier injection and lazy peer id setting 
URL: https://github.com/apache/skywalking/pull/4462#issuecomment-596176455
 
 
   # [Codecov](https://codecov.io/gh/apache/skywalking/pull/4462?src=pr&el=h1) Report
   > Merging [#4462](https://codecov.io/gh/apache/skywalking/pull/4462?src=pr&el=desc) into [master](https://codecov.io/gh/apache/skywalking/commit/0439dccc3054a342d92a160f4e60352eba9ce4e5?src=pr&el=desc) will **decrease** coverage by `<.01%`.
   > The diff coverage is `51.61%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/skywalking/pull/4462/graphs/tree.svg?width=650&token=qrILxY5yA8&height=150&src=pr)](https://codecov.io/gh/apache/skywalking/pull/4462?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master    #4462      +/-   ##
   ==========================================
   - Coverage   24.97%   24.97%   -0.01%     
   ==========================================
     Files        1230     1231       +1     
     Lines       28490    28504      +14     
     Branches     3906     3907       +1     
   ==========================================
   + Hits         7115     7118       +3     
   - Misses      20718    20727       +9     
   - Partials      657      659       +2
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/skywalking/pull/4462?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [...alking/apm/agent/core/context/trace/LocalSpan.java](https://codecov.io/gh/apache/skywalking/pull/4462/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29udGV4dC90cmFjZS9Mb2NhbFNwYW4uamF2YQ==) | `0% <0%> (ø)` | :arrow_up: |
   | [...ing/apm/agent/core/context/trace/NoopExitSpan.java](https://codecov.io/gh/apache/skywalking/pull/4462/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29udGV4dC90cmFjZS9Ob29wRXhpdFNwYW4uamF2YQ==) | `0% <0%> (ø)` | |
   | [...walking/apm/agent/core/context/trace/ExitSpan.java](https://codecov.io/gh/apache/skywalking/pull/4462/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29udGV4dC90cmFjZS9FeGl0U3Bhbi5qYXZh) | `53.57% <25%> (ø)` | :arrow_up: |
   | [...walking/apm/agent/core/context/TracingContext.java](https://codecov.io/gh/apache/skywalking/pull/4462/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29udGV4dC9UcmFjaW5nQ29udGV4dC5qYXZh) | `60.6% <62.5%> (-1.39%)` | :arrow_down: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/skywalking/pull/4462?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/4462?src=pr&el=footer). Last update [0439dcc...0bdafde](https://codecov.io/gh/apache/skywalking/pull/4462?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] wu-sheng commented on issue #4462: [Agent Core] Support lazy ContextCarrier injection and lazy peer id setting

Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #4462: [Agent Core] Support lazy ContextCarrier injection and lazy peer id setting 
URL: https://github.com/apache/skywalking/pull/4462#issuecomment-596190355
 
 
   Merged. Update your PR please.

----------------------------------------------------------------
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 #4462: [Agent Core] Support lazy ContextCarrier injection and lazy peer id setting

Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4462: [Agent Core] Support lazy ContextCarrier injection and lazy peer id setting 
URL: https://github.com/apache/skywalking/pull/4462#discussion_r389344616
 
 

 ##########
 File path: apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/AbstractTracerContext.java
 ##########
 @@ -31,6 +31,15 @@
      */
     void inject(ContextCarrier carrier);
 
+    /**
+     * Prepare for the cross-process propagation based on the given exit span. The given exit span should belong to the
+     * current context. How to initialize the carrier, depends on the implementation.
+     *
+     * @param carrier  to carry the context for crossing process.
+     * @param exitSpan to represent the scope of current injection.
+     */
+    void _inject(AbstractSpan exitSpan, ContextCarrier carrier);
 
 Review comment:
   From my idea, the interface is just open at the codes level, in here, to balance the noop tracing context and real tracing context. Anyway, I could change it to `inject` to more clear comment.

----------------------------------------------------------------
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] huangyoje commented on issue #4462: [Agent Core] Support lazy ContextCarrier injection and lazy peer id setting

Posted by GitBox <gi...@apache.org>.
huangyoje commented on issue #4462: [Agent Core] Support lazy ContextCarrier injection and lazy peer id setting 
URL: https://github.com/apache/skywalking/pull/4462#issuecomment-596180101
 
 
   I just tried with this PR, and in my scenario, I encountered a problem:
   1. I created `ExitSpan` without valid op name and peer host
   2. the op name and peer host will be set to `ExitSpan` later, but the ordering is uncertain
   3. but when can I invoke the `ExitSpan.inject(ContextCarrier)`?
       
   Does the below way is OK?
   ```java
   void tryInject(ExitSpan exitSpan) {
         if (isValid(exitSpan)) {
              return null;   
         }
         ContextCarrier carrier = new ContextCarrier();
         exitSpan.inject(carrier);
         return carrier;
   }
   
   boolean isValid(ExitSpan exitSpan) {
          return exitSpan.getOperatioName() != null && exitSpan.getPeer() != null;
   }
   ```
   
   
   

----------------------------------------------------------------
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 #4462: [Agent Core] Support lazy ContextCarrier injection and lazy peer id setting

Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on a change in pull request #4462: [Agent Core] Support lazy ContextCarrier injection and lazy peer id setting 
URL: https://github.com/apache/skywalking/pull/4462#discussion_r389342564
 
 

 ##########
 File path: apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/AbstractTracerContext.java
 ##########
 @@ -31,6 +31,15 @@
      */
     void inject(ContextCarrier carrier);
 
+    /**
+     * Prepare for the cross-process propagation based on the given exit span. The given exit span should belong to the
+     * current context. How to initialize the carrier, depends on the implementation.
+     *
+     * @param carrier  to carry the context for crossing process.
+     * @param exitSpan to represent the scope of current injection.
+     */
+    void _inject(AbstractSpan exitSpan, ContextCarrier carrier);
 
 Review comment:
   why add the leading `_`

----------------------------------------------------------------
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 #4462: [Agent Core] Support lazy ContextCarrier injection and lazy peer id setting

Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4462: [Agent Core] Support lazy ContextCarrier injection and lazy peer id setting 
URL: https://github.com/apache/skywalking/pull/4462#discussion_r389343743
 
 

 ##########
 File path: apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/trace/ExitTypeSpan.java
 ##########
 @@ -18,8 +18,15 @@
 
 package org.apache.skywalking.apm.agent.core.context.trace;
 
-public interface WithPeerInfo {
+import org.apache.skywalking.apm.agent.core.context.ContextCarrier;
+
+/**
+ * The exit span has some additional behaviours
+ */
+public interface ExitTypeSpan {
     int getPeerId();
 
     String getPeer();
+
+    void inject(ContextCarrier carrier);
 
 Review comment:
   From the API level, I could only return `ExitTypeSpan`, is that useful?

----------------------------------------------------------------
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] huangyoje commented on issue #4462: [Agent Core] Support lazy ContextCarrier injection and lazy peer id setting

Posted by GitBox <gi...@apache.org>.
huangyoje commented on issue #4462: [Agent Core] Support lazy ContextCarrier injection and lazy peer id setting 
URL: https://github.com/apache/skywalking/pull/4462#issuecomment-596188806
 
 
   > Waiting for @huangyoje's local test confirmation.
   
   My loacl tests have all passed!  And Thanks for this PR.

----------------------------------------------------------------
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 #4462: [Agent Core] Support lazy ContextCarrier injection and lazy peer id setting

Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4462: [Agent Core] Support lazy ContextCarrier injection and lazy peer id setting 
URL: https://github.com/apache/skywalking/pull/4462#discussion_r389346933
 
 

 ##########
 File path: apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/AbstractTracerContext.java
 ##########
 @@ -31,6 +31,15 @@
      */
     void inject(ContextCarrier carrier);
 
+    /**
+     * Prepare for the cross-process propagation based on the given exit span. The given exit span should belong to the
+     * current context. How to initialize the carrier, depends on the implementation.
+     *
+     * @param carrier  to carry the context for crossing process.
+     * @param exitSpan to represent the scope of current injection.
+     */
+    void _inject(AbstractSpan exitSpan, ContextCarrier carrier);
 
 Review comment:
   Fixed, should be better now.

----------------------------------------------------------------
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 merged pull request #4462: [Agent Core] Support lazy ContextCarrier injection and lazy peer id setting

Posted by GitBox <gi...@apache.org>.
wu-sheng merged pull request #4462: [Agent Core] Support lazy ContextCarrier injection and lazy peer id setting 
URL: https://github.com/apache/skywalking/pull/4462
 
 
   

----------------------------------------------------------------
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 #4462: [Agent Core] Support lazy ContextCarrier injection and lazy peer id setting

Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on a change in pull request #4462: [Agent Core] Support lazy ContextCarrier injection and lazy peer id setting 
URL: https://github.com/apache/skywalking/pull/4462#discussion_r389344282
 
 

 ##########
 File path: apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/AbstractTracerContext.java
 ##########
 @@ -31,6 +31,15 @@
      */
     void inject(ContextCarrier carrier);
 
+    /**
+     * Prepare for the cross-process propagation based on the given exit span. The given exit span should belong to the
+     * current context. How to initialize the carrier, depends on the implementation.
+     *
+     * @param carrier  to carry the context for crossing process.
+     * @param exitSpan to represent the scope of current injection.
+     */
+    void _inject(AbstractSpan exitSpan, ContextCarrier carrier);
 
 Review comment:
   > I was wanting people think this could be used as an open API.
   
   All methods in an `interface` are **OPEN API** IMO, there's no conventions that methods start with `_` is private or open in Java, on the contrary, many languages use the leading `_` to indicate that the method is `private`

----------------------------------------------------------------
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 issue #4462: [Agent Core] Support lazy ContextCarrier injection and lazy peer id setting

Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #4462: [Agent Core] Support lazy ContextCarrier injection and lazy peer id setting 
URL: https://github.com/apache/skywalking/pull/4462#issuecomment-596180895
 
 
   > return exitSpan.getOperatioName() != null && exitSpan.getPeer() != null;
   
   I prefer you use the plugin internal variables rather than using `exitSpan#get*`. @huangyoje 

----------------------------------------------------------------
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 #4462: [Agent Core] Support lazy ContextCarrier injection and lazy peer id setting

Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on a change in pull request #4462: [Agent Core] Support lazy ContextCarrier injection and lazy peer id setting 
URL: https://github.com/apache/skywalking/pull/4462#discussion_r389346029
 
 

 ##########
 File path: apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/AbstractTracerContext.java
 ##########
 @@ -31,6 +31,15 @@
      */
     void inject(ContextCarrier carrier);
 
+    /**
+     * Prepare for the cross-process propagation based on the given exit span. The given exit span should belong to the
+     * current context. How to initialize the carrier, depends on the implementation.
+     *
+     * @param carrier  to carry the context for crossing process.
+     * @param exitSpan to represent the scope of current injection.
+     */
+    void _inject(AbstractSpan exitSpan, ContextCarrier carrier);
 
 Review comment:
   > I was wanting people think this could be used as an open API. 
   
   Well, I think you just expressed the opposite meanings ("want people **NOT** to used as an open API"), which makes me more confused why add this method here, instead of in `TracingContext`, the `ExitSpan` holds an instance of `TracingContext` instead of `AbstractTracerContext`, it should illuminate both our concerns
   

----------------------------------------------------------------
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 #4462: [Agent Core] Support lazy ContextCarrier injection and lazy peer id setting

Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #4462: [Agent Core] Support lazy ContextCarrier injection and lazy peer id setting 
URL: https://github.com/apache/skywalking/pull/4462#issuecomment-596176455
 
 
   # [Codecov](https://codecov.io/gh/apache/skywalking/pull/4462?src=pr&el=h1) Report
   > Merging [#4462](https://codecov.io/gh/apache/skywalking/pull/4462?src=pr&el=desc) into [master](https://codecov.io/gh/apache/skywalking/commit/0439dccc3054a342d92a160f4e60352eba9ce4e5?src=pr&el=desc) will **decrease** coverage by `<.01%`.
   > The diff coverage is `51.61%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/skywalking/pull/4462/graphs/tree.svg?width=650&token=qrILxY5yA8&height=150&src=pr)](https://codecov.io/gh/apache/skywalking/pull/4462?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master    #4462      +/-   ##
   ==========================================
   - Coverage   24.97%   24.97%   -0.01%     
   ==========================================
     Files        1230     1231       +1     
     Lines       28490    28504      +14     
     Branches     3906     3907       +1     
   ==========================================
   + Hits         7115     7118       +3     
   - Misses      20718    20727       +9     
   - Partials      657      659       +2
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/skywalking/pull/4462?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [...alking/apm/agent/core/context/trace/LocalSpan.java](https://codecov.io/gh/apache/skywalking/pull/4462/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29udGV4dC90cmFjZS9Mb2NhbFNwYW4uamF2YQ==) | `0% <0%> (ø)` | :arrow_up: |
   | [...ing/apm/agent/core/context/trace/NoopExitSpan.java](https://codecov.io/gh/apache/skywalking/pull/4462/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29udGV4dC90cmFjZS9Ob29wRXhpdFNwYW4uamF2YQ==) | `0% <0%> (ø)` | |
   | [...g/apm/agent/core/context/IgnoredTracerContext.java](https://codecov.io/gh/apache/skywalking/pull/4462/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29udGV4dC9JZ25vcmVkVHJhY2VyQ29udGV4dC5qYXZh) | `68.96% <0%> (-2.47%)` | :arrow_down: |
   | [...walking/apm/agent/core/context/trace/ExitSpan.java](https://codecov.io/gh/apache/skywalking/pull/4462/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29udGV4dC90cmFjZS9FeGl0U3Bhbi5qYXZh) | `53.57% <25%> (ø)` | :arrow_up: |
   | [...walking/apm/agent/core/context/TracingContext.java](https://codecov.io/gh/apache/skywalking/pull/4462/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29udGV4dC9UcmFjaW5nQ29udGV4dC5qYXZh) | `60.6% <62.5%> (-1.39%)` | :arrow_down: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/skywalking/pull/4462?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/4462?src=pr&el=footer). Last update [0439dcc...3be6195](https://codecov.io/gh/apache/skywalking/pull/4462?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] huangyoje commented on issue #4462: [Agent Core] Support lazy ContextCarrier injection and lazy peer id setting

Posted by GitBox <gi...@apache.org>.
huangyoje commented on issue #4462: [Agent Core] Support lazy ContextCarrier injection and lazy peer id setting 
URL: https://github.com/apache/skywalking/pull/4462#issuecomment-596195143
 
 
   > Merged. Update your PR please.
   
   OK.

----------------------------------------------------------------
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 issue #4462: [Agent Core] Support lazy ContextCarrier injection and lazy peer id setting

Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #4462: [Agent Core] Support lazy ContextCarrier injection and lazy peer id setting 
URL: https://github.com/apache/skywalking/pull/4462#issuecomment-596180482
 
 
   > the op name and peer host will be set to ExitSpan later, but the ordering is uncertain
   
   This is fine. just be sure these are set before the `#inject`. If you need to do before injecting check, then you could. Just seems strange, but at least not hijacking the core APIs. But please keep the comments clear about why you are doing 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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [skywalking] huangyoje edited a comment on issue #4462: [Agent Core] Support lazy ContextCarrier injection and lazy peer id setting

Posted by GitBox <gi...@apache.org>.
huangyoje edited a comment on issue #4462: [Agent Core] Support lazy ContextCarrier injection and lazy peer id setting 
URL: https://github.com/apache/skywalking/pull/4462#issuecomment-596180101
 
 
   I just tried with this PR, and in my scenario, I encountered a problem:
   1. I created `ExitSpan` without valid op name and peer host
   2. the op name and peer host will be set to `ExitSpan` later, but the ordering is uncertain
   3. but when can I invoke the `ExitSpan.inject(ContextCarrier)`?
       
   Does the below way is OK?
   ```java
   ContextCarrier tryInject(ExitSpan exitSpan) {
         if (isValid(exitSpan)) {
              return null;   
         }
         ContextCarrier carrier = new ContextCarrier();
         exitSpan.inject(carrier);
         return carrier;
   }
   
   boolean isValid(ExitSpan exitSpan) {
          return exitSpan.getOperatioName() != null && exitSpan.getPeer() != null;
   }
   ```
   
   
   

----------------------------------------------------------------
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 #4462: [Agent Core] Support lazy ContextCarrier injection and lazy peer id setting

Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on a change in pull request #4462: [Agent Core] Support lazy ContextCarrier injection and lazy peer id setting 
URL: https://github.com/apache/skywalking/pull/4462#discussion_r389346029
 
 

 ##########
 File path: apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/AbstractTracerContext.java
 ##########
 @@ -31,6 +31,15 @@
      */
     void inject(ContextCarrier carrier);
 
+    /**
+     * Prepare for the cross-process propagation based on the given exit span. The given exit span should belong to the
+     * current context. How to initialize the carrier, depends on the implementation.
+     *
+     * @param carrier  to carry the context for crossing process.
+     * @param exitSpan to represent the scope of current injection.
+     */
+    void _inject(AbstractSpan exitSpan, ContextCarrier carrier);
 
 Review comment:
   > I was wanting people think this could be used as an open API. 
   
   Well, I think you just expressed the opposite meanings ("want people **NOT** to used as an open API"), which makes me more confused why add this method here, instead of in `TracingContext` (package private), the `ExitSpan` holds an instance of `TracingContext` instead of `AbstractTracerContext`, it should illuminate both our concerns
   

----------------------------------------------------------------
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 issue #4462: [Agent Core] Support lazy ContextCarrier injection and lazy peer id setting

Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #4462: [Agent Core] Support lazy ContextCarrier injection and lazy peer id setting 
URL: https://github.com/apache/skywalking/pull/4462#issuecomment-596182378
 
 
   Waiting for @huangyoje's local test confirmation.

----------------------------------------------------------------
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] huangyoje commented on issue #4462: [Agent Core] Support lazy ContextCarrier injection and lazy peer id setting

Posted by GitBox <gi...@apache.org>.
huangyoje commented on issue #4462: [Agent Core] Support lazy ContextCarrier injection and lazy peer id setting 
URL: https://github.com/apache/skywalking/pull/4462#issuecomment-596181689
 
 
   > I prefer you use the plugin internal variables rather than using `exitSpan#get*`. @huangyoje
   > Because the peer name could be exchanged to peer id. So avoid of using that, use something you could control.
   
   OK, I will try to avoid using `exitSpan#get*`.

----------------------------------------------------------------
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 #4462: [Agent Core] Support lazy ContextCarrier injection and lazy peer id setting

Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on a change in pull request #4462: [Agent Core] Support lazy ContextCarrier injection and lazy peer id setting 
URL: https://github.com/apache/skywalking/pull/4462#discussion_r389344185
 
 

 ##########
 File path: apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/trace/ExitTypeSpan.java
 ##########
 @@ -18,8 +18,15 @@
 
 package org.apache.skywalking.apm.agent.core.context.trace;
 
-public interface WithPeerInfo {
+import org.apache.skywalking.apm.agent.core.context.ContextCarrier;
+
+/**
+ * The exit span has some additional behaviours
+ */
+public interface ExitTypeSpan {
     int getPeerId();
 
     String getPeer();
+
+    void inject(ContextCarrier carrier);
 
 Review comment:
   > From the API level, I could only return `ExitTypeSpan`, is that useful?
   
   Yes, from the interface you have to return `ExitTypeSpan`, but you can return `ExitSpan` in its implementations, and chaining with the more specific types
   
   <img width="752" alt="image" src="https://user-images.githubusercontent.com/15965696/76158658-263e0500-6153-11ea-84b5-c2153faffbaa.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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [skywalking] dmsolr commented on issue #4462: [Agent Core] Support lazy ContextCarrier injection and lazy peer id setting

Posted by GitBox <gi...@apache.org>.
dmsolr commented on issue #4462: [Agent Core] Support lazy ContextCarrier injection and lazy peer id setting 
URL: https://github.com/apache/skywalking/pull/4462#issuecomment-596178309
 
 
   Wow, this PR is friendly to write any plugin on the async scenario. Before this, we usually need to waste much time to find the right point to enhance.

----------------------------------------------------------------
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 #4462: [Agent Core] Support lazy ContextCarrier injection and lazy peer id setting

Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4462: [Agent Core] Support lazy ContextCarrier injection and lazy peer id setting 
URL: https://github.com/apache/skywalking/pull/4462#discussion_r389343743
 
 

 ##########
 File path: apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/trace/ExitTypeSpan.java
 ##########
 @@ -18,8 +18,15 @@
 
 package org.apache.skywalking.apm.agent.core.context.trace;
 
-public interface WithPeerInfo {
+import org.apache.skywalking.apm.agent.core.context.ContextCarrier;
+
+/**
+ * The exit span has some additional behaviours
+ */
+public interface ExitTypeSpan {
     int getPeerId();
 
     String getPeer();
+
+    void inject(ContextCarrier carrier);
 
 Review comment:
   From the API lever, I could only return `ExitTypeSpan`, is that useful?

----------------------------------------------------------------
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 #4462: [Agent Core] Support lazy ContextCarrier injection and lazy peer id setting

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #4462: [Agent Core] Support lazy ContextCarrier injection and lazy peer id setting 
URL: https://github.com/apache/skywalking/pull/4462#issuecomment-596176455
 
 
   # [Codecov](https://codecov.io/gh/apache/skywalking/pull/4462?src=pr&el=h1) Report
   > Merging [#4462](https://codecov.io/gh/apache/skywalking/pull/4462?src=pr&el=desc) into [master](https://codecov.io/gh/apache/skywalking/commit/0439dccc3054a342d92a160f4e60352eba9ce4e5?src=pr&el=desc) will **decrease** coverage by `<.01%`.
   > The diff coverage is `51.61%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/skywalking/pull/4462/graphs/tree.svg?width=650&token=qrILxY5yA8&height=150&src=pr)](https://codecov.io/gh/apache/skywalking/pull/4462?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master    #4462      +/-   ##
   ==========================================
   - Coverage   24.97%   24.96%   -0.01%     
   ==========================================
     Files        1230     1231       +1     
     Lines       28490    28504      +14     
     Branches     3906     3907       +1     
   ==========================================
   + Hits         7115     7117       +2     
   - Misses      20718    20727       +9     
   - Partials      657      660       +3
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/skywalking/pull/4462?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [...alking/apm/agent/core/context/trace/LocalSpan.java](https://codecov.io/gh/apache/skywalking/pull/4462/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29udGV4dC90cmFjZS9Mb2NhbFNwYW4uamF2YQ==) | `0% <0%> (ø)` | :arrow_up: |
   | [...ing/apm/agent/core/context/trace/NoopExitSpan.java](https://codecov.io/gh/apache/skywalking/pull/4462/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29udGV4dC90cmFjZS9Ob29wRXhpdFNwYW4uamF2YQ==) | `0% <0%> (ø)` | |
   | [...g/apm/agent/core/context/IgnoredTracerContext.java](https://codecov.io/gh/apache/skywalking/pull/4462/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29udGV4dC9JZ25vcmVkVHJhY2VyQ29udGV4dC5qYXZh) | `68.96% <0%> (-2.47%)` | :arrow_down: |
   | [...walking/apm/agent/core/context/trace/ExitSpan.java](https://codecov.io/gh/apache/skywalking/pull/4462/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29udGV4dC90cmFjZS9FeGl0U3Bhbi5qYXZh) | `53.57% <25%> (ø)` | :arrow_up: |
   | [...walking/apm/agent/core/context/TracingContext.java](https://codecov.io/gh/apache/skywalking/pull/4462/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29udGV4dC9UcmFjaW5nQ29udGV4dC5qYXZh) | `60.6% <62.5%> (-1.39%)` | :arrow_down: |
   | [...ache/skywalking/apm/agent/core/jvm/JVMService.java](https://codecov.io/gh/apache/skywalking/pull/4462/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvanZtL0pWTVNlcnZpY2UuamF2YQ==) | `55% <0%> (-1.67%)` | :arrow_down: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/skywalking/pull/4462?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/4462?src=pr&el=footer). Last update [0439dcc...3be6195](https://codecov.io/gh/apache/skywalking/pull/4462?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] wu-sheng commented on a change in pull request #4462: [Agent Core] Support lazy ContextCarrier injection and lazy peer id setting

Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4462: [Agent Core] Support lazy ContextCarrier injection and lazy peer id setting 
URL: https://github.com/apache/skywalking/pull/4462#discussion_r389344993
 
 

 ##########
 File path: apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/AbstractTracerContext.java
 ##########
 @@ -31,6 +31,15 @@
      */
     void inject(ContextCarrier carrier);
 
+    /**
+     * Prepare for the cross-process propagation based on the given exit span. The given exit span should belong to the
+     * current context. How to initialize the carrier, depends on the implementation.
+     *
+     * @param carrier  to carry the context for crossing process.
+     * @param exitSpan to represent the scope of current injection.
+     */
+    void _inject(AbstractSpan exitSpan, ContextCarrier carrier);
 
 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] kezhenxu94 commented on a change in pull request #4462: [Agent Core] Support lazy ContextCarrier injection and lazy peer id setting

Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on a change in pull request #4462: [Agent Core] Support lazy ContextCarrier injection and lazy peer id setting 
URL: https://github.com/apache/skywalking/pull/4462#discussion_r389344282
 
 

 ##########
 File path: apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/AbstractTracerContext.java
 ##########
 @@ -31,6 +31,15 @@
      */
     void inject(ContextCarrier carrier);
 
+    /**
+     * Prepare for the cross-process propagation based on the given exit span. The given exit span should belong to the
+     * current context. How to initialize the carrier, depends on the implementation.
+     *
+     * @param carrier  to carry the context for crossing process.
+     * @param exitSpan to represent the scope of current injection.
+     */
+    void _inject(AbstractSpan exitSpan, ContextCarrier carrier);
 
 Review comment:
   > I was wanting people think this could be used as an open API.
   
   All methods in an `interface` are **OPEN API** IMO, there's no conventions that methods start with `_` is private or open, on the contrary, many languages use the leading `_` to indicate that the method is `private`

----------------------------------------------------------------
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 #4462: [Agent Core] Support lazy ContextCarrier injection and lazy peer id setting

Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4462: [Agent Core] Support lazy ContextCarrier injection and lazy peer id setting 
URL: https://github.com/apache/skywalking/pull/4462#discussion_r389343447
 
 

 ##########
 File path: apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/AbstractTracerContext.java
 ##########
 @@ -31,6 +31,15 @@
      */
     void inject(ContextCarrier carrier);
 
+    /**
+     * Prepare for the cross-process propagation based on the given exit span. The given exit span should belong to the
+     * current context. How to initialize the carrier, depends on the implementation.
+     *
+     * @param carrier  to carry the context for crossing process.
+     * @param exitSpan to represent the scope of current injection.
+     */
+    void _inject(AbstractSpan exitSpan, ContextCarrier carrier);
 
 Review comment:
   I was wanting people think this could be used as an open API. That is the only repo. Do you want to keep the same method name? `inject`?

----------------------------------------------------------------
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 #4462: [Agent Core] Support lazy ContextCarrier injection and lazy peer id setting

Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4462: [Agent Core] Support lazy ContextCarrier injection and lazy peer id setting 
URL: https://github.com/apache/skywalking/pull/4462#discussion_r389343461
 
 

 ##########
 File path: apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/trace/ExitTypeSpan.java
 ##########
 @@ -18,8 +18,15 @@
 
 package org.apache.skywalking.apm.agent.core.context.trace;
 
-public interface WithPeerInfo {
+import org.apache.skywalking.apm.agent.core.context.ContextCarrier;
+
+/**
+ * The exit span has some additional behaviours
+ */
+public interface ExitTypeSpan {
     int getPeerId();
 
     String getPeer();
+
+    void inject(ContextCarrier carrier);
 
 Review comment:
   After the tests pass, will do.

----------------------------------------------------------------
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 #4462: [Agent Core] Support lazy ContextCarrier injection and lazy peer id setting

Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on a change in pull request #4462: [Agent Core] Support lazy ContextCarrier injection and lazy peer id setting 
URL: https://github.com/apache/skywalking/pull/4462#discussion_r389343319
 
 

 ##########
 File path: apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/trace/ExitTypeSpan.java
 ##########
 @@ -18,8 +18,15 @@
 
 package org.apache.skywalking.apm.agent.core.context.trace;
 
-public interface WithPeerInfo {
+import org.apache.skywalking.apm.agent.core.context.ContextCarrier;
+
+/**
+ * The exit span has some additional behaviours
+ */
+public interface ExitTypeSpan {
     int getPeerId();
 
     String getPeer();
+
+    void inject(ContextCarrier carrier);
 
 Review comment:
   Consider returning `this` for chaining

----------------------------------------------------------------
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] huangyoje commented on issue #4462: [Agent Core] Support lazy ContextCarrier injection and lazy peer id setting

Posted by GitBox <gi...@apache.org>.
huangyoje commented on issue #4462: [Agent Core] Support lazy ContextCarrier injection and lazy peer id setting 
URL: https://github.com/apache/skywalking/pull/4462#issuecomment-596180873
 
 
   > This is fine. just be sure these are set before the `#inject`. If you need to do before injecting check, then you could. Just seems strange, but at least not hijacking the core APIs. But please keep the comments clear about why you are doing this.
   
   OK, I am testing with this PR, I will feedback the result as soon as the test is over.
   

----------------------------------------------------------------
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 #4462: [Agent Core] Support lazy ContextCarrier injection and lazy peer id setting

Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4462: [Agent Core] Support lazy ContextCarrier injection and lazy peer id setting 
URL: https://github.com/apache/skywalking/pull/4462#discussion_r389346783
 
 

 ##########
 File path: apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/AbstractTracerContext.java
 ##########
 @@ -31,6 +31,15 @@
      */
     void inject(ContextCarrier carrier);
 
+    /**
+     * Prepare for the cross-process propagation based on the given exit span. The given exit span should belong to the
+     * current context. How to initialize the carrier, depends on the implementation.
+     *
+     * @param carrier  to carry the context for crossing process.
+     * @param exitSpan to represent the scope of current injection.
+     */
+    void _inject(AbstractSpan exitSpan, ContextCarrier carrier);
 
 Review comment:
   Good point. Let me do that.

----------------------------------------------------------------
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 #4462: [Agent Core] Support lazy ContextCarrier injection and lazy peer id setting

Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on a change in pull request #4462: [Agent Core] Support lazy ContextCarrier injection and lazy peer id setting 
URL: https://github.com/apache/skywalking/pull/4462#discussion_r389344282
 
 

 ##########
 File path: apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/AbstractTracerContext.java
 ##########
 @@ -31,6 +31,15 @@
      */
     void inject(ContextCarrier carrier);
 
+    /**
+     * Prepare for the cross-process propagation based on the given exit span. The given exit span should belong to the
+     * current context. How to initialize the carrier, depends on the implementation.
+     *
+     * @param carrier  to carry the context for crossing process.
+     * @param exitSpan to represent the scope of current injection.
+     */
+    void _inject(AbstractSpan exitSpan, ContextCarrier carrier);
 
 Review comment:
   > I was wanting people think this could be used as an open API. That is the only repo. Do you want to keep the same method name? `inject`?
   
   All methods in an `interface` are **OPEN API** IMO, there's no conventions that methods start with `_` is private or open, on the contrary, many languages use the leading `_` to indicate that the method is `private`

----------------------------------------------------------------
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