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/23 08:35:28 UTC

[GitHub] [skywalking] mrproliu opened a new pull request #4555: Correlation protocol implement

mrproliu opened a new pull request #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555
 
 
   Please answer these questions before submitting pull request
   
   - Why submit this pull request?
   - [ ] Bug fix
   - [x] New feature provided
   - [ ] Improve performance
   
   - Related issues
   #4509 
   ___
   ### New feature or improvement
   - Implement Correlation protocol, java version.
   

----------------------------------------------------------------
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 #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

kezhenxu94 commented on issue #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#issuecomment-603861737
 
 
   > It was best to commit one pull request for one feature
   
   @geektcp  It's sure better, but never mind, we do a squash when merging

----------------------------------------------------------------
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] mrproliu commented on a change in pull request #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

mrproliu commented on a change in pull request #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#discussion_r396531316
 
 

 ##########
 File path: docs/en/setup/service-agent/java-agent/Application-toolkit-correlation.md
 ##########
 @@ -0,0 +1,33 @@
+## Cross Process Correlation
+
+## Introduce
+This plugin is help user to transport custom data in the tracing context. [Follow this to get protocol description.](../../../protocols/Skywalking-Cross-Process-Correlation-Headers-Protocol-v1.md)
+
+## How to use it
+* Dependency the toolkit, such as using maven or gradle
+```xml
+   <dependency>
+      <groupId>org.apache.skywalking</groupId>
+      <artifactId>apm-toolkit-trace</artifactId>
+      <version>${skywalking.version}</version>
+   </dependency>
+```
+
+* Use `CorrelationContext.set()` API to setting custom data.
+```java
+CorrelationSettingResult settingResult = CorrelationContext.set("customKey", "customValue");
 
 Review comment:
   Changed that return the previous value only. 

----------------------------------------------------------------
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 #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

wu-sheng commented on a change in pull request #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#discussion_r396325974
 
 

 ##########
 File path: apm-sniffer/config/agent.config
 ##########
 @@ -77,3 +77,9 @@ logging.level=${SW_LOGGING_LEVEL:INFO}
 
 # mysql plugin configuration
 # plugin.mysql.trace_sql_parameters=${SW_MYSQL_TRACE_SQL_PARAMETERS:false}
+
+# Correlation max key count in tracing context
+correlation.key_count=${SW_CORRELATION_KEY_COUNT:3}
+
+# Correlation max value length in the every key
+correlation.value_length=${SW_CORRELATION_VALUE_LENGTH:128}
 
 Review comment:
   We don't add any configuration in the default agent.config, unless they are necessary. For the correlation context, they are not. Please remove them, and keep them in the documentation only.

----------------------------------------------------------------
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 #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

kezhenxu94 commented on a change in pull request #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#discussion_r396896476
 
 

 ##########
 File path: docs/en/setup/service-agent/java-agent/Application-toolkit-trace.md
 ##########
 @@ -53,3 +53,13 @@ public User methodYouWantToTrace(String param1, String param2) {
 }
 ```
 
+* Use `CorrelationContext.set()` API to setting custom data in tracing context. 
+```java
+optional<String> previous = CorrelationContext.set("customKey", "customValue");
+```
+
+* Use `CorrelationContext.get()` API to get custom data.
+```java
+optional<String> value = CorrelationContext.get("customKey");
 
 Review comment:
   ```suggestion
   Optional<String> value = CorrelationContext.get("customKey");
   ```

----------------------------------------------------------------
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 #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

codecov-io edited a comment on issue #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#issuecomment-602475846
 
 
   # [Codecov](https://codecov.io/gh/apache/skywalking/pull/4555?src=pr&el=h1) Report
   > Merging [#4555](https://codecov.io/gh/apache/skywalking/pull/4555?src=pr&el=desc) into [master](https://codecov.io/gh/apache/skywalking/commit/e7f66c911eaf18a14793e5edf09788cb5f391f29&el=desc) will **increase** coverage by `0.12%`.
   > The diff coverage is `52.04%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/skywalking/pull/4555/graphs/tree.svg?width=650&height=150&src=pr&token=qrILxY5yA8)](https://codecov.io/gh/apache/skywalking/pull/4555?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master    #4555      +/-   ##
   ==========================================
   + Coverage   25.57%   25.69%   +0.12%     
   ==========================================
     Files        1246     1250       +4     
     Lines       28975    29071      +96     
     Branches     3974     3990      +16     
   ==========================================
   + Hits         7410     7471      +61     
   - Misses      20873    20909      +36     
   + Partials      692      691       -1     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/skywalking/pull/4555?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [...walking/apm/agent/core/context/ContextManager.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29udGV4dC9Db250ZXh0TWFuYWdlci5qYXZh) | `50.00% <0.00%> (-2.39%)` | :arrow_down: |
   | [...alking/apm/agent/core/context/ContextSnapshot.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29udGV4dC9Db250ZXh0U25hcHNob3QuamF2YQ==) | `0.00% <0.00%> (ø)` | |
   | [...ng/apm/agent/core/context/MockContextSnapshot.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLXRlc3QtdG9vbHMvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29udGV4dC9Nb2NrQ29udGV4dFNuYXBzaG90LmphdmE=) | `0.00% <0.00%> (ø)` | |
   | [...vation/trace/CorrelationContextGetInterceptor.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLXRvb2xraXQtYWN0aXZhdGlvbi9hcG0tdG9vbGtpdC10cmFjZS1hY3RpdmF0aW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9za3l3YWxraW5nL2FwbS90b29sa2l0L2FjdGl2YXRpb24vdHJhY2UvQ29ycmVsYXRpb25Db250ZXh0R2V0SW50ZXJjZXB0b3IuamF2YQ==) | `0.00% <0.00%> (ø)` | |
   | [...vation/trace/CorrelationContextPutInterceptor.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLXRvb2xraXQtYWN0aXZhdGlvbi9hcG0tdG9vbGtpdC10cmFjZS1hY3RpdmF0aW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9za3l3YWxraW5nL2FwbS90b29sa2l0L2FjdGl2YXRpb24vdHJhY2UvQ29ycmVsYXRpb25Db250ZXh0UHV0SW50ZXJjZXB0b3IuamF2YQ==) | `0.00% <0.00%> (ø)` | |
   | [...olkit/activation/trace/TraceContextActivation.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLXRvb2xraXQtYWN0aXZhdGlvbi9hcG0tdG9vbGtpdC10cmFjZS1hY3RpdmF0aW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9za3l3YWxraW5nL2FwbS90b29sa2l0L2FjdGl2YXRpb24vdHJhY2UvVHJhY2VDb250ZXh0QWN0aXZhdGlvbi5qYXZh) | `0.00% <0.00%> (ø)` | |
   | [...g/apm/agent/core/context/IgnoredTracerContext.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29udGV4dC9JZ25vcmVkVHJhY2VyQ29udGV4dC5qYXZh) | `71.42% <40.00%> (ø)` | |
   | [...walking/apm/agent/core/context/TracingContext.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29udGV4dC9UcmFjaW5nQ29udGV4dC5qYXZh) | `61.66% <60.00%> (+0.22%)` | :arrow_up: |
   | [...ing/apm/agent/core/context/CorrelationContext.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29udGV4dC9Db3JyZWxhdGlvbkNvbnRleHQuamF2YQ==) | `66.66% <66.66%> (ø)` | |
   | [.../apache/skywalking/apm/agent/core/conf/Config.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29uZi9Db25maWcuamF2YQ==) | `70.17% <100.00%> (+1.08%)` | :arrow_up: |
   | ... and [11 more](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/skywalking/pull/4555?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/4555?src=pr&el=footer). Last update [e7f66c9...c497d7a](https://codecov.io/gh/apache/skywalking/pull/4555?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 a change in pull request #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

kezhenxu94 commented on a change in pull request #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#discussion_r396918542
 
 

 ##########
 File path: apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/CorrelationContext.java
 ##########
 @@ -0,0 +1,150 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.skywalking.apm.agent.core.context;
+
+import org.apache.skywalking.apm.agent.core.base64.Base64;
+import org.apache.skywalking.apm.agent.core.conf.Config;
+import org.apache.skywalking.apm.util.StringUtil;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Optional;
+import java.util.stream.Collectors;
+
+/**
+ * Correlation context, use to propagation user custom data.
+ * Working on the protocol and delegate set/get method.
+ */
+public class CorrelationContext {
+
+    private final Map<String, String> data;
+
+    public CorrelationContext() {
+        this.data = new HashMap<>(Config.Correlation.ELEMENT_MAX_NUMBER);
+    }
+
+    public Optional<String> set(String key, String value) {
+        // key must not null
+        if (key == null) {
+            return Optional.empty();
+        }
+        if (value == null) {
+            value = "";
+        }
 
 Review comment:
   > Do you mean delete the key when the value is null? Should I add a new method to implement the `remove` method?
   
   Adding a new API `remove` looks good to me, but I prefer to remove the item when the `value == null`(or even when `value.isEmpty()`) because the places in the context are "precious" (only 3 items are allowed by default), and propagating `null` or `""` is a waste of bandwidth, WDYT @wu-sheng 

----------------------------------------------------------------
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] mrproliu commented on a change in pull request #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

mrproliu commented on a change in pull request #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#discussion_r397151122
 
 

 ##########
 File path: apm-application-toolkit/apm-toolkit-trace/src/main/java/org/apache/skywalking/apm/toolkit/trace/CorrelationContext.java
 ##########
 @@ -0,0 +1,45 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.skywalking.apm.toolkit.trace;
+
+import java.util.Optional;
+
+/**
+ * CorrelationContext is the interactive API for end user to put/set custom data.
+ */
+public class CorrelationContext {
 
 Review comment:
   Sure. I could change to `TraceContext#putCorrelation` and `TraceContext#getCorrelation`, replace the `CorrelationContext` toolkit. 
   I think it's the same for users, they refer to the document and use them.

----------------------------------------------------------------
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 #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

wu-sheng commented on a change in pull request #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#discussion_r396875190
 
 

 ##########
 File path: test/plugin/scenarios/apm-toolkit-trace-scenario/src/main/java/org/apache/skywalking/apm/toolkit/trace/CorrelationContext.java
 ##########
 @@ -0,0 +1,46 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.skywalking.apm.toolkit.trace;
+
+import java.util.Optional;
+
+/**
+ * Propagate the custom data in the tracing context.
 
 Review comment:
   ```suggestion
    * CorrelationContext is the interactive API for end user to put/set custom data. 
   ```

----------------------------------------------------------------
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 #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

wu-sheng commented on a change in pull request #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#discussion_r396414619
 
 

 ##########
 File path: apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/conf/Config.java
 ##########
 @@ -394,4 +394,16 @@
             public static int HTTP_PARAMS_LENGTH_THRESHOLD = 1024;
         }
     }
+
+    public static class Correlation {
+        /**
+         * Max key count in the correlation context.
+         */
+        public static int KEY_COUNT = 3;
+
+        /**
+         * Max value length in each key.
+         */
+        public static int VALUE_LENGTH = 128;
 
 Review comment:
   Should be renamed to `VALUE_MAX_LENGTH `.  

----------------------------------------------------------------
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 #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

codecov-io edited a comment on issue #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#issuecomment-602475846
 
 
   # [Codecov](https://codecov.io/gh/apache/skywalking/pull/4555?src=pr&el=h1) Report
   > Merging [#4555](https://codecov.io/gh/apache/skywalking/pull/4555?src=pr&el=desc) into [master](https://codecov.io/gh/apache/skywalking/commit/2ec8e2ad7b67d3f1e5d551b33151922448fd8309&el=desc) will **increase** coverage by `0.11%`.
   > The diff coverage is `57.44%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/skywalking/pull/4555/graphs/tree.svg?width=650&height=150&src=pr&token=qrILxY5yA8)](https://codecov.io/gh/apache/skywalking/pull/4555?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master    #4555      +/-   ##
   ==========================================
   + Coverage   25.57%   25.68%   +0.11%     
   ==========================================
     Files        1246     1251       +5     
     Lines       28975    29067      +92     
     Branches     3974     3988      +14     
   ==========================================
   + Hits         7410     7467      +57     
   - Misses      20873    20911      +38     
   + Partials      692      689       -3     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/skywalking/pull/4555?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [...walking/apm/agent/core/context/ContextManager.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29udGV4dC9Db250ZXh0TWFuYWdlci5qYXZh) | `50.00% <0.00%> (-2.39%)` | :arrow_down: |
   | [...alking/apm/agent/core/context/ContextSnapshot.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29udGV4dC9Db250ZXh0U25hcHNob3QuamF2YQ==) | `0.00% <0.00%> (ø)` | |
   | [...ng/apm/agent/core/context/MockContextSnapshot.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLXRlc3QtdG9vbHMvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29udGV4dC9Nb2NrQ29udGV4dFNuYXBzaG90LmphdmE=) | `0.00% <0.00%> (ø)` | |
   | [...activation/trace/CorrelationContextActivation.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLXRvb2xraXQtYWN0aXZhdGlvbi9hcG0tdG9vbGtpdC10cmFjZS1hY3RpdmF0aW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9za3l3YWxraW5nL2FwbS90b29sa2l0L2FjdGl2YXRpb24vdHJhY2UvQ29ycmVsYXRpb25Db250ZXh0QWN0aXZhdGlvbi5qYXZh) | `0.00% <0.00%> (ø)` | |
   | [...vation/trace/CorrelationContextGetInterceptor.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLXRvb2xraXQtYWN0aXZhdGlvbi9hcG0tdG9vbGtpdC10cmFjZS1hY3RpdmF0aW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9za3l3YWxraW5nL2FwbS90b29sa2l0L2FjdGl2YXRpb24vdHJhY2UvQ29ycmVsYXRpb25Db250ZXh0R2V0SW50ZXJjZXB0b3IuamF2YQ==) | `0.00% <0.00%> (ø)` | |
   | [...vation/trace/CorrelationContextSetInterceptor.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLXRvb2xraXQtYWN0aXZhdGlvbi9hcG0tdG9vbGtpdC10cmFjZS1hY3RpdmF0aW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9za3l3YWxraW5nL2FwbS90b29sa2l0L2FjdGl2YXRpb24vdHJhY2UvQ29ycmVsYXRpb25Db250ZXh0U2V0SW50ZXJjZXB0b3IuamF2YQ==) | `0.00% <0.00%> (ø)` | |
   | [...g/apm/agent/core/context/IgnoredTracerContext.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29udGV4dC9JZ25vcmVkVHJhY2VyQ29udGV4dC5qYXZh) | `71.42% <40.00%> (ø)` | |
   | [...walking/apm/agent/core/context/TracingContext.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29udGV4dC9UcmFjaW5nQ29udGV4dC5qYXZh) | `61.66% <60.00%> (+0.22%)` | :arrow_up: |
   | [...ing/apm/agent/core/context/CorrelationContext.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29udGV4dC9Db3JyZWxhdGlvbkNvbnRleHQuamF2YQ==) | `88.63% <88.63%> (ø)` | |
   | [.../apache/skywalking/apm/agent/core/conf/Config.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29uZi9Db25maWcuamF2YQ==) | `70.17% <100.00%> (+1.08%)` | :arrow_up: |
   | ... and [13 more](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/skywalking/pull/4555?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/4555?src=pr&el=footer). Last update [2ec8e2a...9e3afc5](https://codecov.io/gh/apache/skywalking/pull/4555?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 #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

wu-sheng commented on a change in pull request #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#discussion_r396872008
 
 

 ##########
 File path: docs/en/setup/service-agent/java-agent/README.md
 ##########
 @@ -125,6 +125,8 @@ property key | Description | Default |
 `plugin.tomcat.collect_http_params`| This config item controls that whether the Tomcat plugin should collect the parameters of the request. Also, activate implicitly in the profiled trace. | `false` |
 `plugin.springmvc.collect_http_params`| This config item controls that whether the SpringMVC plugin should collect the parameters of the request, when your Spring application is based on Tomcat, consider only setting either `plugin.tomcat.collect_http_params` or `plugin.springmvc.collect_http_params`. Also, activate implicitly in the profiled trace. | `false` |
 `plugin.http.http_params_length_threshold`| When `COLLECT_HTTP_PARAMS` is enabled, how many characters to keep and send to the OAP backend, use negative values to keep and send the complete parameters, NB. this config item is added for the sake of performance.  | `1024` |
+`correlation.element_max_number`|Correlation max element count in the correlation context.|`3`|
 
 Review comment:
   ```suggestion
   `correlation.element_max_number`| Max element count of the correlation context.|`3`|
   ```

----------------------------------------------------------------
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 #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

wu-sheng commented on issue #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#issuecomment-602565704
 
 
   NOTICE, this is an agent core level API change, we must be very carefully about all codes. 

----------------------------------------------------------------
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 #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

wu-sheng commented on a change in pull request #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#discussion_r396417992
 
 

 ##########
 File path: apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/IgnoredTracerContext.java
 ##########
 @@ -32,30 +32,33 @@
 public class IgnoredTracerContext implements AbstractTracerContext {
     private static final NoopSpan NOOP_SPAN = new NoopSpan();
 
+    private final CorrelationContext correlationContext;
+
     private int stackDepth;
 
     public IgnoredTracerContext() {
         this.stackDepth = 0;
+        this.correlationContext = new CorrelationContext();
     }
 
     @Override
     public void inject(ContextCarrier carrier) {
-
+        carrier.getCorrelationContext().resetFrom(this.correlationContext);
 
 Review comment:
   `inject` is for further propagation, I am confused the implementation here.

----------------------------------------------------------------
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 #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

kezhenxu94 commented on a change in pull request #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#discussion_r396897128
 
 

 ##########
 File path: docs/en/setup/service-agent/java-agent/README.md
 ##########
 @@ -158,7 +160,7 @@ Now, we have the following known bootstrap plugins.
     * If you want to use OpenTracing Java APIs, try [SkyWalking OpenTracing compatible tracer](Opentracing.md). More details you could find at http://opentracing.io
     * If you want to print trace context(e.g. traceId) in your logs, choose the log frameworks, [log4j](Application-toolkit-log4j-1.x.md), 
 [log4j2](Application-toolkit-log4j-2.x.md), [logback](Application-toolkit-logback-1.x.md)
-    * If you want to use annotations or SkyWalking native APIs to read context, try [SkyWalking manual APIs](Application-toolkit-trace.md)
+    * If you want your codes to interact with SkyWalking agent, including `getting trace id`, `set tags`, `propagating custom data` etc.. Try [SkyWalking manual APIs](Application-toolkit-trace.md).
 
 Review comment:
   please unify the forms,  
   
   `getting trace id`, `setting tags`, `propagating custom data`
   
   or 
   
   `get trace id`, `set tags`, `propagate custom data`

----------------------------------------------------------------
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 #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

kezhenxu94 commented on a change in pull request #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#discussion_r397148598
 
 

 ##########
 File path: apm-application-toolkit/apm-toolkit-trace/src/main/java/org/apache/skywalking/apm/toolkit/trace/CorrelationContext.java
 ##########
 @@ -0,0 +1,45 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.skywalking.apm.toolkit.trace;
+
+import java.util.Optional;
+
+/**
+ * CorrelationContext is the interactive API for end user to put/set custom data.
+ */
+public class CorrelationContext {
 
 Review comment:
   Looks good to me

----------------------------------------------------------------
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 #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

wu-sheng commented on a change in pull request #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#discussion_r396952280
 
 

 ##########
 File path: apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/CorrelationContext.java
 ##########
 @@ -0,0 +1,150 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.skywalking.apm.agent.core.context;
+
+import org.apache.skywalking.apm.agent.core.base64.Base64;
+import org.apache.skywalking.apm.agent.core.conf.Config;
+import org.apache.skywalking.apm.util.StringUtil;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Optional;
+import java.util.stream.Collectors;
+
+/**
+ * Correlation context, use to propagation user custom data.
+ * Working on the protocol and delegate set/get method.
+ */
+public class CorrelationContext {
+
+    private final Map<String, String> data;
+
+    public CorrelationContext() {
+        this.data = new HashMap<>(Config.Correlation.ELEMENT_MAX_NUMBER);
+    }
+
+    public Optional<String> set(String key, String value) {
+        // key must not null
+        if (key == null) {
+            return Optional.empty();
+        }
+        if (value == null) {
+            value = "";
+        }
+
+        // check value length
+        if (value.length() > Config.Correlation.VALUE_MAX_LENGTH) {
+            return Optional.empty();
+        }
+
+        // already contain key
+        if (data.containsKey(key)) {
+            final String previousValue = data.put(key, value);
+            return Optional.of(previousValue);
+        }
+
+        // check keys count
+        if (data.size() >= Config.Correlation.ELEMENT_MAX_NUMBER) {
+            return Optional.empty();
+        }
+
+        // setting
+        data.put(key, value);
+        return Optional.empty();
+    }
+
+    public Optional<String> get(String key) {
+        if (key == null) {
+            return Optional.empty();
+        }
+
+        return Optional.ofNullable(data.get(key));
+    }
+
+    /**
+     * Serialize this {@link CorrelationContext} to a {@link String}
+     *
+     * @return the serialization string.
+     */
+    String serialize() {
+        if (data.isEmpty()) {
+            return "";
+        }
+
+        return data.entrySet().stream()
+            .map(entry -> Base64.encode(entry.getKey()) + ":" + Base64.encode(entry.getValue()))
+            .collect(Collectors.joining(","));
+    }
+
+    /**
+     * Deserialize data from {@link String}
+     */
+    void deserialize(String value) {
+        if (StringUtil.isEmpty(value)) {
+            return;
+        }
+
+        for (String perData : value.split(",")) {
+            final String[] parts = perData.split(":");
+            String perDataKey = parts[0];
+            String perDataValue = parts.length > 1 ? parts[1] : "";
+            data.put(Base64.decode2UTFString(perDataKey), Base64.decode2UTFString(perDataValue));
+        }
+    }
+
+    /**
+     * Prepare for the cross-process propagation. Inject the {@link #data} into {@link ContextCarrier#getCorrelationContext()}
+     */
+    void inject(ContextCarrier carrier) {
+        carrier.getCorrelationContext().data.putAll(this.data);
 
 Review comment:
   We will propagate all context key/value(s) without any protection. Please fix 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] kezhenxu94 commented on a change in pull request #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

kezhenxu94 commented on a change in pull request #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#discussion_r396896402
 
 

 ##########
 File path: docs/en/setup/service-agent/java-agent/Application-toolkit-trace.md
 ##########
 @@ -53,3 +53,13 @@ public User methodYouWantToTrace(String param1, String param2) {
 }
 ```
 
+* Use `CorrelationContext.set()` API to setting custom data in tracing context. 
 
 Review comment:
   ```suggestion
   * Use `CorrelationContext.set()` API to set custom data in tracing context. 
   ```

----------------------------------------------------------------
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 #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

wu-sheng commented on a change in pull request #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#discussion_r396417564
 
 

 ##########
 File path: apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/CorrelationContext.java
 ##########
 @@ -0,0 +1,164 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.skywalking.apm.agent.core.context;
+
+import org.apache.skywalking.apm.agent.core.base64.Base64;
+import org.apache.skywalking.apm.agent.core.conf.Config;
+import org.apache.skywalking.apm.util.StringUtil;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Objects;
+import java.util.stream.Collectors;
+
+/**
+ * Correlation context, use to propagation user custom data.
+ * Working on the protocol and delegate set/get method.
+ */
+public class CorrelationContext {
+
+    private volatile Map<String, String> data;
+
+    public CorrelationContext() {
+        this.data = new HashMap<>(0);
+    }
+
+    public SettingResult set(String key, String value) {
+        // key must not null
+        if (key == null) {
+            return SettingResult.buildWithSettingError("Key Cannot be null");
+        }
+        if (value == null) {
+            value = "";
+        }
+
+        // check value length
+        if (value.length() > Config.Correlation.VALUE_LENGTH) {
+            return SettingResult.buildWithSettingError("Out out correlation value length limit");
+        }
+
+        // already contain key
+        if (data.containsKey(key)) {
+            final String previousValue = data.put(key, value);
+            return SettingResult.buildWithSuccess(previousValue);
+        }
+
+        // check keys count
+        if (data.size() >= Config.Correlation.KEY_COUNT) {
+            return SettingResult.buildWithSettingError("Out out correlation key count limit");
+        }
+
+        // setting
+        data.put(key, value);
+        return SettingResult.buildWithSuccess(null);
+    }
+
+    public String get(String key) {
+        if (key == null) {
+            return "";
+        }
+
+        final String value = data.get(key);
+        return value == null ? "" : value;
+    }
+
+    /**
+     * Serialize this {@link CorrelationContext} to a {@link String}
+     *
+     * @return the serialization string.
+     */
+    String serialize() {
+        if (data.isEmpty()) {
+            return "";
+        }
+
+        return data.entrySet().stream()
+            .map(entry -> Base64.encode(entry.getKey()) + ":" + Base64.encode(entry.getValue()))
+            .collect(Collectors.joining(","));
+    }
+
+    /**
+     * Deserialize data from {@link String}
+     */
+    void deserialize(String value) {
+        if (StringUtil.isEmpty(value)) {
+            return;
+        }
+
+        for (String perData : value.split(",")) {
+            final String[] parts = perData.split(":");
+            String perDataKey = parts[0];
+            String perDataValue = parts.length > 1 ? parts[1] : "";
+            data.put(Base64.decode2UTFString(perDataKey), Base64.decode2UTFString(perDataValue));
+        }
+    }
+
+    /**
+     * Reset correlation context from other context
+     */
+    public void resetFrom(CorrelationContext context) {
 
 Review comment:
   Why isn't this `inject`/`extract` like the existing API design?

----------------------------------------------------------------
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 #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

wu-sheng commented on a change in pull request #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#discussion_r396853059
 
 

 ##########
 File path: apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/conf/Config.java
 ##########
 @@ -394,4 +394,16 @@
             public static int HTTP_PARAMS_LENGTH_THRESHOLD = 1024;
         }
     }
+
+    public static class Correlation {
+        /**
+         * Max element count in the correlation context.
+         */
+        public static int ELEMENT_MAX_NUMBER = 3;
+
+        /**
+         * Max value length in each key.
 
 Review comment:
   ```suggestion
            * Max value length of each element.
   ```

----------------------------------------------------------------
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] mrproliu commented on a change in pull request #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

mrproliu commented on a change in pull request #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#discussion_r396537419
 
 

 ##########
 File path: apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/ContextSnapshot.java
 ##########
 @@ -49,12 +49,15 @@
 
     private int entryApplicationInstanceId = DictionaryUtil.nullValue();
 
-    ContextSnapshot(ID traceSegmentId, int spanId, List<DistributedTraceId> distributedTraceIds) {
+    private CorrelationContext correlationContext;
+
+    ContextSnapshot(ID traceSegmentId, int spanId, List<DistributedTraceId> distributedTraceIds, CorrelationContext correlationContext) {
         this.traceSegmentId = traceSegmentId;
         this.spanId = spanId;
         if (distributedTraceIds != null) {
             this.primaryDistributedTraceId = distributedTraceIds.get(0);
         }
+        this.correlationContext = correlationContext;
 
 Review comment:
   In this case, I neglected the interference between threads. Change that using `clone` mode.

----------------------------------------------------------------
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 #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

wu-sheng commented on a change in pull request #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#discussion_r396872352
 
 

 ##########
 File path: docs/en/setup/service-agent/java-agent/README.md
 ##########
 @@ -158,7 +160,7 @@ Now, we have the following known bootstrap plugins.
     * If you want to use OpenTracing Java APIs, try [SkyWalking OpenTracing compatible tracer](Opentracing.md). More details you could find at http://opentracing.io
     * If you want to print trace context(e.g. traceId) in your logs, choose the log frameworks, [log4j](Application-toolkit-log4j-1.x.md), 
 [log4j2](Application-toolkit-log4j-2.x.md), [logback](Application-toolkit-logback-1.x.md)
-    * If you want to use annotations or SkyWalking native APIs to read context, try [SkyWalking manual APIs](Application-toolkit-trace.md)
+    * If you want your codes interact with SkyWalking agent, including `getting trace id`, `set tags`, etc.. Try [SkyWalking manual APIs](Application-toolkit-trace.md).
 
 Review comment:
   ```suggestion
       * If you want your codes to interact with SkyWalking agent, including `getting trace id`, `set tags`, `propagating custom data` etc.. Try [SkyWalking manual APIs](Application-toolkit-trace.md).
   ```

----------------------------------------------------------------
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 #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

codecov-io edited a comment on issue #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#issuecomment-602475846
 
 
   # [Codecov](https://codecov.io/gh/apache/skywalking/pull/4555?src=pr&el=h1) Report
   > Merging [#4555](https://codecov.io/gh/apache/skywalking/pull/4555?src=pr&el=desc) into [master](https://codecov.io/gh/apache/skywalking/commit/7c9b6cf054f8e534afcc5366e01858b384fd7e8d&el=desc) will **increase** coverage by `0.12%`.
   > The diff coverage is `53.68%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/skywalking/pull/4555/graphs/tree.svg?width=650&height=150&src=pr&token=qrILxY5yA8)](https://codecov.io/gh/apache/skywalking/pull/4555?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master    #4555      +/-   ##
   ==========================================
   + Coverage   25.54%   25.67%   +0.12%     
   ==========================================
     Files        1246     1251       +5     
     Lines       28975    29068      +93     
     Branches     3974     3987      +13     
   ==========================================
   + Hits         7403     7464      +61     
   - Misses      20880    20915      +35     
   + Partials      692      689       -3     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/skywalking/pull/4555?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [...walking/apm/agent/core/context/ContextManager.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29udGV4dC9Db250ZXh0TWFuYWdlci5qYXZh) | `50.00% <0.00%> (-2.39%)` | :arrow_down: |
   | [...alking/apm/agent/core/context/ContextSnapshot.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29udGV4dC9Db250ZXh0U25hcHNob3QuamF2YQ==) | `0.00% <0.00%> (ø)` | |
   | [...ng/apm/agent/core/context/MockContextSnapshot.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLXRlc3QtdG9vbHMvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29udGV4dC9Nb2NrQ29udGV4dFNuYXBzaG90LmphdmE=) | `0.00% <0.00%> (ø)` | |
   | [...activation/trace/CorrelationContextActivation.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLXRvb2xraXQtYWN0aXZhdGlvbi9hcG0tdG9vbGtpdC10cmFjZS1hY3RpdmF0aW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9za3l3YWxraW5nL2FwbS90b29sa2l0L2FjdGl2YXRpb24vdHJhY2UvQ29ycmVsYXRpb25Db250ZXh0QWN0aXZhdGlvbi5qYXZh) | `0.00% <0.00%> (ø)` | |
   | [...vation/trace/CorrelationContextGetInterceptor.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLXRvb2xraXQtYWN0aXZhdGlvbi9hcG0tdG9vbGtpdC10cmFjZS1hY3RpdmF0aW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9za3l3YWxraW5nL2FwbS90b29sa2l0L2FjdGl2YXRpb24vdHJhY2UvQ29ycmVsYXRpb25Db250ZXh0R2V0SW50ZXJjZXB0b3IuamF2YQ==) | `0.00% <0.00%> (ø)` | |
   | [...vation/trace/CorrelationContextSetInterceptor.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLXRvb2xraXQtYWN0aXZhdGlvbi9hcG0tdG9vbGtpdC10cmFjZS1hY3RpdmF0aW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9za3l3YWxraW5nL2FwbS90b29sa2l0L2FjdGl2YXRpb24vdHJhY2UvQ29ycmVsYXRpb25Db250ZXh0U2V0SW50ZXJjZXB0b3IuamF2YQ==) | `0.00% <0.00%> (ø)` | |
   | [...g/apm/agent/core/context/IgnoredTracerContext.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29udGV4dC9JZ25vcmVkVHJhY2VyQ29udGV4dC5qYXZh) | `71.42% <40.00%> (ø)` | |
   | [...walking/apm/agent/core/context/TracingContext.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29udGV4dC9UcmFjaW5nQ29udGV4dC5qYXZh) | `61.66% <60.00%> (+0.22%)` | :arrow_up: |
   | [...ing/apm/agent/core/context/CorrelationContext.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29udGV4dC9Db3JyZWxhdGlvbkNvbnRleHQuamF2YQ==) | `76.59% <76.59%> (ø)` | |
   | [.../apache/skywalking/apm/agent/core/conf/Config.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29uZi9Db25maWcuamF2YQ==) | `70.17% <100.00%> (+1.08%)` | :arrow_up: |
   | ... and [12 more](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/skywalking/pull/4555?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/4555?src=pr&el=footer). Last update [7c9b6cf...fcf4aee](https://codecov.io/gh/apache/skywalking/pull/4555?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 a change in pull request #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

kezhenxu94 commented on a change in pull request #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#discussion_r396896440
 
 

 ##########
 File path: docs/en/setup/service-agent/java-agent/Application-toolkit-trace.md
 ##########
 @@ -53,3 +53,13 @@ public User methodYouWantToTrace(String param1, String param2) {
 }
 ```
 
+* Use `CorrelationContext.set()` API to setting custom data in tracing context. 
+```java
+optional<String> previous = CorrelationContext.set("customKey", "customValue");
 
 Review comment:
   ```suggestion
   Optional<String> previous = CorrelationContext.set("customKey", "customValue");
   ```

----------------------------------------------------------------
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 #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

kezhenxu94 commented on a change in pull request #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#discussion_r396963284
 
 

 ##########
 File path: apm-application-toolkit/apm-toolkit-trace/src/main/java/org/apache/skywalking/apm/toolkit/trace/CorrelationContext.java
 ##########
 @@ -0,0 +1,45 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.skywalking.apm.toolkit.trace;
+
+import java.util.Optional;
+
+/**
+ * CorrelationContext is the interactive API for end user to put/set custom data.
+ */
+public class CorrelationContext {
+
+    /**
+     * Try to get the custom value from trace context.
+     *
+     * @return custom data value.
+     */
+    public static Optional<String> get(String key) {
+        return Optional.empty();
+    }
+
+    /**
+     * Setting the custom key/value into trace context.
+     *
+     * @return previous value if it exists.
+     */
+    public static Optional<String> set(String key, String value) {
+        return Optional.empty();
+    }
 
 Review comment:
   > We could guarantee there is no EMPTY_STRING as the value, as we discussed above. 
   
   Since [we have reached a consensus](https://github.com/apache/skywalking/pull/4555#discussion_r396902387) that `null`/`""` values should trigger a removal, this concern is gone accordingly

----------------------------------------------------------------
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 #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

wu-sheng commented on a change in pull request #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#discussion_r396327190
 
 

 ##########
 File path: docs/en/setup/service-agent/java-agent/Application-toolkit-correlation.md
 ##########
 @@ -0,0 +1,33 @@
+## Cross Process Correlation
+
+## Introduce
+This plugin is help user to transport custom data in the tracing context. [Follow this to get protocol description.](../../../protocols/Skywalking-Cross-Process-Correlation-Headers-Protocol-v1.md)
+
+## How to use it
+* Dependency the toolkit, such as using maven or gradle
+```xml
+   <dependency>
+      <groupId>org.apache.skywalking</groupId>
+      <artifactId>apm-toolkit-trace</artifactId>
+      <version>${skywalking.version}</version>
+   </dependency>
+```
+
+* Use `CorrelationContext.set()` API to setting custom data.
+```java
+CorrelationSettingResult settingResult = CorrelationContext.set("customKey", "customValue");
 
 Review comment:
   A question about the design, what is the point of result, how do the user codes react with 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] kezhenxu94 commented on a change in pull request #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

kezhenxu94 commented on a change in pull request #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#discussion_r396918542
 
 

 ##########
 File path: apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/CorrelationContext.java
 ##########
 @@ -0,0 +1,150 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.skywalking.apm.agent.core.context;
+
+import org.apache.skywalking.apm.agent.core.base64.Base64;
+import org.apache.skywalking.apm.agent.core.conf.Config;
+import org.apache.skywalking.apm.util.StringUtil;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Optional;
+import java.util.stream.Collectors;
+
+/**
+ * Correlation context, use to propagation user custom data.
+ * Working on the protocol and delegate set/get method.
+ */
+public class CorrelationContext {
+
+    private final Map<String, String> data;
+
+    public CorrelationContext() {
+        this.data = new HashMap<>(Config.Correlation.ELEMENT_MAX_NUMBER);
+    }
+
+    public Optional<String> set(String key, String value) {
+        // key must not null
+        if (key == null) {
+            return Optional.empty();
+        }
+        if (value == null) {
+            value = "";
+        }
 
 Review comment:
   > Do you mean delete the key when the value is null? Should I add a new method to implement the `remove` method?
   
   Adding a new API `remove` looks good to me, but I prefer to remove the item when the `value == null`(or even when `value.isEmpty()`) because the places in the context are "precious", and propagating `null` or `""` is a waste of bandwidth, WDYT @wu-sheng 

----------------------------------------------------------------
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 #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

codecov-io edited a comment on issue #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#issuecomment-602475846
 
 
   # [Codecov](https://codecov.io/gh/apache/skywalking/pull/4555?src=pr&el=h1) Report
   > Merging [#4555](https://codecov.io/gh/apache/skywalking/pull/4555?src=pr&el=desc) into [master](https://codecov.io/gh/apache/skywalking/commit/f1b2b298b1dcb75f5c211c71b0b956d447859d1d&el=desc) will **increase** coverage by `0.09%`.
   > The diff coverage is `51.00%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/skywalking/pull/4555/graphs/tree.svg?width=650&height=150&src=pr&token=qrILxY5yA8)](https://codecov.io/gh/apache/skywalking/pull/4555?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master    #4555      +/-   ##
   ==========================================
   + Coverage   25.57%   25.67%   +0.09%     
   ==========================================
     Files        1246     1251       +5     
     Lines       28975    29073      +98     
     Branches     3974     3990      +16     
   ==========================================
   + Hits         7410     7464      +54     
   - Misses      20873    20918      +45     
   + Partials      692      691       -1     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/skywalking/pull/4555?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [...walking/apm/agent/core/context/ContextManager.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29udGV4dC9Db250ZXh0TWFuYWdlci5qYXZh) | `50.00% <0.00%> (-2.39%)` | :arrow_down: |
   | [...alking/apm/agent/core/context/ContextSnapshot.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29udGV4dC9Db250ZXh0U25hcHNob3QuamF2YQ==) | `0.00% <0.00%> (ø)` | |
   | [...ng/apm/agent/core/context/MockContextSnapshot.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLXRlc3QtdG9vbHMvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29udGV4dC9Nb2NrQ29udGV4dFNuYXBzaG90LmphdmE=) | `0.00% <0.00%> (ø)` | |
   | [...activation/trace/CorrelationContextActivation.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLXRvb2xraXQtYWN0aXZhdGlvbi9hcG0tdG9vbGtpdC10cmFjZS1hY3RpdmF0aW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9za3l3YWxraW5nL2FwbS90b29sa2l0L2FjdGl2YXRpb24vdHJhY2UvQ29ycmVsYXRpb25Db250ZXh0QWN0aXZhdGlvbi5qYXZh) | `0.00% <0.00%> (ø)` | |
   | [...vation/trace/CorrelationContextGetInterceptor.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLXRvb2xraXQtYWN0aXZhdGlvbi9hcG0tdG9vbGtpdC10cmFjZS1hY3RpdmF0aW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9za3l3YWxraW5nL2FwbS90b29sa2l0L2FjdGl2YXRpb24vdHJhY2UvQ29ycmVsYXRpb25Db250ZXh0R2V0SW50ZXJjZXB0b3IuamF2YQ==) | `0.00% <0.00%> (ø)` | |
   | [...vation/trace/CorrelationContextSetInterceptor.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLXRvb2xraXQtYWN0aXZhdGlvbi9hcG0tdG9vbGtpdC10cmFjZS1hY3RpdmF0aW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9za3l3YWxraW5nL2FwbS90b29sa2l0L2FjdGl2YXRpb24vdHJhY2UvQ29ycmVsYXRpb25Db250ZXh0U2V0SW50ZXJjZXB0b3IuamF2YQ==) | `0.00% <0.00%> (ø)` | |
   | [...g/apm/agent/core/context/IgnoredTracerContext.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29udGV4dC9JZ25vcmVkVHJhY2VyQ29udGV4dC5qYXZh) | `71.42% <40.00%> (ø)` | |
   | [...walking/apm/agent/core/context/TracingContext.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29udGV4dC9UcmFjaW5nQ29udGV4dC5qYXZh) | `61.66% <60.00%> (+0.22%)` | :arrow_up: |
   | [...ing/apm/agent/core/context/CorrelationContext.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29udGV4dC9Db3JyZWxhdGlvbkNvbnRleHQuamF2YQ==) | `66.66% <66.66%> (ø)` | |
   | [.../apache/skywalking/apm/agent/core/conf/Config.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29uZi9Db25maWcuamF2YQ==) | `70.17% <100.00%> (+1.08%)` | :arrow_up: |
   | ... and [13 more](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/skywalking/pull/4555?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/4555?src=pr&el=footer). Last update [f1b2b29...ec8be4a](https://codecov.io/gh/apache/skywalking/pull/4555?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 #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

wu-sheng commented on a change in pull request #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#discussion_r396414164
 
 

 ##########
 File path: apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/conf/Config.java
 ##########
 @@ -394,4 +394,16 @@
             public static int HTTP_PARAMS_LENGTH_THRESHOLD = 1024;
         }
     }
+
+    public static class Correlation {
+        /**
+         * Max key count in the correlation context.
+         */
+        public static int KEY_COUNT = 3;
 
 Review comment:
   Should be renamed to `ELEMENT_MAX_NUMBER`.

----------------------------------------------------------------
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 #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

wu-sheng commented on a change in pull request #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#discussion_r396327981
 
 

 ##########
 File path: docs/en/setup/service-agent/java-agent/Application-toolkit-correlation.md
 ##########
 @@ -0,0 +1,33 @@
+## Cross Process Correlation
+
+## Introduce
+This plugin is help user to transport custom data in the tracing context. [Follow this to get protocol description.](../../../protocols/Skywalking-Cross-Process-Correlation-Headers-Protocol-v1.md)
+
+## How to use it
+* Dependency the toolkit, such as using maven or gradle
+```xml
+   <dependency>
+      <groupId>org.apache.skywalking</groupId>
+      <artifactId>apm-toolkit-trace</artifactId>
+      <version>${skywalking.version}</version>
+   </dependency>
+```
+
+* Use `CorrelationContext.set()` API to setting custom data.
+```java
+CorrelationSettingResult settingResult = CorrelationContext.set("customKey", "customValue");
+```
+_Sample codes only_
+
+1. Add `CorrelationContext.set` to setting your custom data.
+1. Key and value only support `String` type.
+1. Please follow [the agent configuration](README.md#table-of-agent-configuration-properties) to get more setting limit.
+
+* Use `CorrelationContext.get()` API to get custom data.
+```java
+CorrelationContext.get("customKey");
+```
+_Sample codes only_
+
+1. `CorrelationContext.get` to get custom data.
+1. Return empty string if cannot found the custom key.
 
 Review comment:
   I think in the JDK 1.8, the recommended way is returning `option`. We can't have that because agent used to be JDK1.6 compatible, but we are not anymore. The new API should follow the JDK1.8 sytle.

----------------------------------------------------------------
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 #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

codecov-io edited a comment on issue #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#issuecomment-602475846
 
 
   # [Codecov](https://codecov.io/gh/apache/skywalking/pull/4555?src=pr&el=h1) Report
   > Merging [#4555](https://codecov.io/gh/apache/skywalking/pull/4555?src=pr&el=desc) into [master](https://codecov.io/gh/apache/skywalking/commit/7c9b6cf054f8e534afcc5366e01858b384fd7e8d&el=desc) will **increase** coverage by `0.12%`.
   > The diff coverage is `52.57%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/skywalking/pull/4555/graphs/tree.svg?width=650&height=150&src=pr&token=qrILxY5yA8)](https://codecov.io/gh/apache/skywalking/pull/4555?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master    #4555      +/-   ##
   ==========================================
   + Coverage   25.54%   25.67%   +0.12%     
   ==========================================
     Files        1246     1251       +5     
     Lines       28975    29070      +95     
     Branches     3974     3987      +13     
   ==========================================
   + Hits         7403     7463      +60     
   - Misses      20880    20917      +37     
   + Partials      692      690       -2     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/skywalking/pull/4555?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [...walking/apm/agent/core/context/ContextManager.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29udGV4dC9Db250ZXh0TWFuYWdlci5qYXZh) | `50.00% <0.00%> (-2.39%)` | :arrow_down: |
   | [...alking/apm/agent/core/context/ContextSnapshot.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29udGV4dC9Db250ZXh0U25hcHNob3QuamF2YQ==) | `0.00% <0.00%> (ø)` | |
   | [...ng/apm/agent/core/context/MockContextSnapshot.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLXRlc3QtdG9vbHMvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29udGV4dC9Nb2NrQ29udGV4dFNuYXBzaG90LmphdmE=) | `0.00% <0.00%> (ø)` | |
   | [...activation/trace/CorrelationContextActivation.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLXRvb2xraXQtYWN0aXZhdGlvbi9hcG0tdG9vbGtpdC10cmFjZS1hY3RpdmF0aW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9za3l3YWxraW5nL2FwbS90b29sa2l0L2FjdGl2YXRpb24vdHJhY2UvQ29ycmVsYXRpb25Db250ZXh0QWN0aXZhdGlvbi5qYXZh) | `0.00% <0.00%> (ø)` | |
   | [...vation/trace/CorrelationContextGetInterceptor.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLXRvb2xraXQtYWN0aXZhdGlvbi9hcG0tdG9vbGtpdC10cmFjZS1hY3RpdmF0aW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9za3l3YWxraW5nL2FwbS90b29sa2l0L2FjdGl2YXRpb24vdHJhY2UvQ29ycmVsYXRpb25Db250ZXh0R2V0SW50ZXJjZXB0b3IuamF2YQ==) | `0.00% <0.00%> (ø)` | |
   | [...vation/trace/CorrelationContextSetInterceptor.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLXRvb2xraXQtYWN0aXZhdGlvbi9hcG0tdG9vbGtpdC10cmFjZS1hY3RpdmF0aW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9za3l3YWxraW5nL2FwbS90b29sa2l0L2FjdGl2YXRpb24vdHJhY2UvQ29ycmVsYXRpb25Db250ZXh0U2V0SW50ZXJjZXB0b3IuamF2YQ==) | `0.00% <0.00%> (ø)` | |
   | [...g/apm/agent/core/context/IgnoredTracerContext.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29udGV4dC9JZ25vcmVkVHJhY2VyQ29udGV4dC5qYXZh) | `71.42% <40.00%> (ø)` | |
   | [...walking/apm/agent/core/context/TracingContext.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29udGV4dC9UcmFjaW5nQ29udGV4dC5qYXZh) | `61.66% <60.00%> (+0.22%)` | :arrow_up: |
   | [...ing/apm/agent/core/context/CorrelationContext.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29udGV4dC9Db3JyZWxhdGlvbkNvbnRleHQuamF2YQ==) | `76.59% <76.59%> (ø)` | |
   | [.../apache/skywalking/apm/agent/core/conf/Config.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29uZi9Db25maWcuamF2YQ==) | `70.17% <100.00%> (+1.08%)` | :arrow_up: |
   | ... and [13 more](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/skywalking/pull/4555?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/4555?src=pr&el=footer). Last update [7c9b6cf...d1b7e05](https://codecov.io/gh/apache/skywalking/pull/4555?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] codecov-io commented on issue #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

codecov-io commented on issue #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#issuecomment-602475846
 
 
   # [Codecov](https://codecov.io/gh/apache/skywalking/pull/4555?src=pr&el=h1) Report
   > Merging [#4555](https://codecov.io/gh/apache/skywalking/pull/4555?src=pr&el=desc) into [master](https://codecov.io/gh/apache/skywalking/commit/2ec8e2ad7b67d3f1e5d551b33151922448fd8309&el=desc) will **increase** coverage by `0.11%`.
   > The diff coverage is `57.44%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/skywalking/pull/4555/graphs/tree.svg?width=650&height=150&src=pr&token=qrILxY5yA8)](https://codecov.io/gh/apache/skywalking/pull/4555?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master    #4555      +/-   ##
   ==========================================
   + Coverage   25.57%   25.68%   +0.11%     
   ==========================================
     Files        1246     1251       +5     
     Lines       28975    29067      +92     
     Branches     3974     3988      +14     
   ==========================================
   + Hits         7410     7467      +57     
   - Misses      20873    20911      +38     
   + Partials      692      689       -3     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/skywalking/pull/4555?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [...walking/apm/agent/core/context/ContextManager.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29udGV4dC9Db250ZXh0TWFuYWdlci5qYXZh) | `50.00% <0.00%> (-2.39%)` | :arrow_down: |
   | [...alking/apm/agent/core/context/ContextSnapshot.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29udGV4dC9Db250ZXh0U25hcHNob3QuamF2YQ==) | `0.00% <0.00%> (ø)` | |
   | [...ng/apm/agent/core/context/MockContextSnapshot.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLXRlc3QtdG9vbHMvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29udGV4dC9Nb2NrQ29udGV4dFNuYXBzaG90LmphdmE=) | `0.00% <0.00%> (ø)` | |
   | [...activation/trace/CorrelationContextActivation.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLXRvb2xraXQtYWN0aXZhdGlvbi9hcG0tdG9vbGtpdC10cmFjZS1hY3RpdmF0aW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9za3l3YWxraW5nL2FwbS90b29sa2l0L2FjdGl2YXRpb24vdHJhY2UvQ29ycmVsYXRpb25Db250ZXh0QWN0aXZhdGlvbi5qYXZh) | `0.00% <0.00%> (ø)` | |
   | [...vation/trace/CorrelationContextGetInterceptor.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLXRvb2xraXQtYWN0aXZhdGlvbi9hcG0tdG9vbGtpdC10cmFjZS1hY3RpdmF0aW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9za3l3YWxraW5nL2FwbS90b29sa2l0L2FjdGl2YXRpb24vdHJhY2UvQ29ycmVsYXRpb25Db250ZXh0R2V0SW50ZXJjZXB0b3IuamF2YQ==) | `0.00% <0.00%> (ø)` | |
   | [...vation/trace/CorrelationContextSetInterceptor.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLXRvb2xraXQtYWN0aXZhdGlvbi9hcG0tdG9vbGtpdC10cmFjZS1hY3RpdmF0aW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9za3l3YWxraW5nL2FwbS90b29sa2l0L2FjdGl2YXRpb24vdHJhY2UvQ29ycmVsYXRpb25Db250ZXh0U2V0SW50ZXJjZXB0b3IuamF2YQ==) | `0.00% <0.00%> (ø)` | |
   | [...g/apm/agent/core/context/IgnoredTracerContext.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29udGV4dC9JZ25vcmVkVHJhY2VyQ29udGV4dC5qYXZh) | `71.42% <40.00%> (ø)` | |
   | [...walking/apm/agent/core/context/TracingContext.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29udGV4dC9UcmFjaW5nQ29udGV4dC5qYXZh) | `61.66% <60.00%> (+0.22%)` | :arrow_up: |
   | [...ing/apm/agent/core/context/CorrelationContext.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29udGV4dC9Db3JyZWxhdGlvbkNvbnRleHQuamF2YQ==) | `88.63% <88.63%> (ø)` | |
   | [.../apache/skywalking/apm/agent/core/conf/Config.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29uZi9Db25maWcuamF2YQ==) | `70.17% <100.00%> (+1.08%)` | :arrow_up: |
   | ... and [13 more](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/skywalking/pull/4555?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/4555?src=pr&el=footer). Last update [2ec8e2a...9e3afc5](https://codecov.io/gh/apache/skywalking/pull/4555?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 #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

wu-sheng commented on a change in pull request #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#discussion_r396953120
 
 

 ##########
 File path: apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/CorrelationContext.java
 ##########
 @@ -0,0 +1,150 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.skywalking.apm.agent.core.context;
+
+import org.apache.skywalking.apm.agent.core.base64.Base64;
+import org.apache.skywalking.apm.agent.core.conf.Config;
+import org.apache.skywalking.apm.util.StringUtil;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Optional;
+import java.util.stream.Collectors;
+
+/**
+ * Correlation context, use to propagation user custom data.
+ * Working on the protocol and delegate set/get method.
+ */
+public class CorrelationContext {
+
+    private final Map<String, String> data;
+
+    public CorrelationContext() {
+        this.data = new HashMap<>(Config.Correlation.ELEMENT_MAX_NUMBER);
+    }
+
+    public Optional<String> set(String key, String value) {
+        // key must not null
+        if (key == null) {
+            return Optional.empty();
+        }
+        if (value == null) {
+            value = "";
+        }
 
 Review comment:
   Agree, we should remove the element when value doesn't exist. And for `remove`, adding or not both works for me. But the logic of `null/empty` triggers `removing` should be documented clearly.

----------------------------------------------------------------
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 #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

kezhenxu94 commented on a change in pull request #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#discussion_r396909089
 
 

 ##########
 File path: apm-application-toolkit/apm-toolkit-trace/src/main/java/org/apache/skywalking/apm/toolkit/trace/CorrelationContext.java
 ##########
 @@ -0,0 +1,45 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.skywalking.apm.toolkit.trace;
+
+import java.util.Optional;
+
+/**
+ * CorrelationContext is the interactive API for end user to put/set custom data.
+ */
+public class CorrelationContext {
+
+    /**
+     * Try to get the custom value from trace context.
+     *
+     * @return custom data value.
+     */
+    public static Optional<String> get(String key) {
+        return Optional.empty();
+    }
+
+    /**
+     * Setting the custom key/value into trace context.
+     *
+     * @return previous value if it exists.
+     */
+    public static Optional<String> set(String key, String value) {
+        return Optional.empty();
+    }
 
 Review comment:
   Personally, I don't recommend to return `Optional<String>` in these two APIs, for `String`s, we(developers, our users are developers too) don't usually **JUST** care about the nullability, we also want to check whether it's empty/blank, with `Optional<String>`, we have to unwrap the `Optional`, and check `isEmpty`/`isBlank`:
   
   ```java
           Optional<String> s = CorrelationContext.get(CORRELATION_CONTEXT_KEY);
           if (s.isPresent() && Strings.isNotBlank(s.get())) {
               // ...
           }
   ```
   
   OR
   
   ```java
           if (!Strings.isNullOrEmpty(CorrelationContext.get(CORRELATION_CONTEXT_KEY).orElse(""))) {
               
           }
   ```
   
   My point is that in this specific situation, `Optional<String>` may not bring much convenience as it should have done, simply returning nullable `String` may do, in the contrast

----------------------------------------------------------------
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 #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

wu-sheng commented on a change in pull request #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#discussion_r396419139
 
 

 ##########
 File path: apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/ContextSnapshot.java
 ##########
 @@ -49,12 +49,15 @@
 
     private int entryApplicationInstanceId = DictionaryUtil.nullValue();
 
-    ContextSnapshot(ID traceSegmentId, int spanId, List<DistributedTraceId> distributedTraceIds) {
+    private CorrelationContext correlationContext;
+
+    ContextSnapshot(ID traceSegmentId, int spanId, List<DistributedTraceId> distributedTraceIds, CorrelationContext correlationContext) {
         this.traceSegmentId = traceSegmentId;
         this.spanId = spanId;
         if (distributedTraceIds != null) {
             this.primaryDistributedTraceId = distributedTraceIds.get(0);
         }
+        this.correlationContext = correlationContext;
 
 Review comment:
   As you are propagating the context, and this is called a snapshot, you should clone the existing context to new thread. Otherwise, this is not a `ContextSnapshot`. Think in this way, when you do the thread snapshot, what does it mean?

----------------------------------------------------------------
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 #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

wu-sheng commented on a change in pull request #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#discussion_r396326677
 
 

 ##########
 File path: docs/en/setup/service-agent/java-agent/Application-toolkit-correlation.md
 ##########
 @@ -0,0 +1,33 @@
+## Cross Process Correlation
+
+## Introduce
+This plugin is help user to transport custom data in the tracing context. [Follow this to get protocol description.](../../../protocols/Skywalking-Cross-Process-Correlation-Headers-Protocol-v1.md)
+
+## How to use it
+* Dependency the toolkit, such as using maven or gradle
+```xml
+   <dependency>
+      <groupId>org.apache.skywalking</groupId>
+      <artifactId>apm-toolkit-trace</artifactId>
+      <version>${skywalking.version}</version>
 
 Review comment:
   Clearly, you are adding the new APIs in the same lib jar, but you separate the document.

----------------------------------------------------------------
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] mrproliu commented on a change in pull request #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

mrproliu commented on a change in pull request #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#discussion_r397135036
 
 

 ##########
 File path: apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/CorrelationContext.java
 ##########
 @@ -0,0 +1,150 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.skywalking.apm.agent.core.context;
+
+import org.apache.skywalking.apm.agent.core.base64.Base64;
+import org.apache.skywalking.apm.agent.core.conf.Config;
+import org.apache.skywalking.apm.util.StringUtil;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Optional;
+import java.util.stream.Collectors;
+
+/**
+ * Correlation context, use to propagation user custom data.
+ * Working on the protocol and delegate set/get method.
+ */
+public class CorrelationContext {
+
+    private final Map<String, String> data;
+
+    public CorrelationContext() {
+        this.data = new HashMap<>(Config.Correlation.ELEMENT_MAX_NUMBER);
+    }
+
+    public Optional<String> set(String key, String value) {
+        // key must not null
+        if (key == null) {
+            return Optional.empty();
+        }
+        if (value == null) {
+            value = "";
+        }
 
 Review comment:
   fixed. It will remove the item when the value is empty or 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] mrproliu commented on a change in pull request #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

mrproliu commented on a change in pull request #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#discussion_r396535655
 
 

 ##########
 File path: apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/IgnoredTracerContext.java
 ##########
 @@ -32,30 +32,33 @@
 public class IgnoredTracerContext implements AbstractTracerContext {
     private static final NoopSpan NOOP_SPAN = new NoopSpan();
 
+    private final CorrelationContext correlationContext;
+
     private int stackDepth;
 
     public IgnoredTracerContext() {
         this.stackDepth = 0;
+        this.correlationContext = new CorrelationContext();
     }
 
     @Override
     public void inject(ContextCarrier carrier) {
-
+        carrier.getCorrelationContext().resetFrom(this.correlationContext);
 
 Review comment:
   Add the `inject` and `extract` into correlation context, and delete the `resetFrom` 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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [skywalking] mrproliu commented on a change in pull request #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

mrproliu commented on a change in pull request #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#discussion_r396419697
 
 

 ##########
 File path: docs/en/setup/service-agent/java-agent/Application-toolkit-correlation.md
 ##########
 @@ -0,0 +1,33 @@
+## Cross Process Correlation
+
+## Introduce
+This plugin is help user to transport custom data in the tracing context. [Follow this to get protocol description.](../../../protocols/Skywalking-Cross-Process-Correlation-Headers-Protocol-v1.md)
+
+## How to use it
+* Dependency the toolkit, such as using maven or gradle
+```xml
+   <dependency>
+      <groupId>org.apache.skywalking</groupId>
+      <artifactId>apm-toolkit-trace</artifactId>
+      <version>${skywalking.version}</version>
 
 Review comment:
   I seem we have [Application-toolkit-trace.md](https://github.com/apache/skywalking/blob/259a147682e82c862f5d4af90237e099b6d38b13/docs/en/setup/service-agent/java-agent/Application-toolkit-trace.md) and [Application-toolkit-trace-cross-thread.md](https://github.com/apache/skywalking/blob/259a147682e82c862f5d4af90237e099b6d38b13/docs/en/setup/service-agent/java-agent/Application-toolkit-trace-cross-thread.md), I not sure where do I need to put this into?

----------------------------------------------------------------
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] mrproliu commented on a change in pull request #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

mrproliu commented on a change in pull request #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#discussion_r396919210
 
 

 ##########
 File path: apm-application-toolkit/apm-toolkit-trace/src/main/java/org/apache/skywalking/apm/toolkit/trace/CorrelationContext.java
 ##########
 @@ -0,0 +1,45 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.skywalking.apm.toolkit.trace;
+
+import java.util.Optional;
+
+/**
+ * CorrelationContext is the interactive API for end user to put/set custom data.
+ */
+public class CorrelationContext {
+
+    /**
+     * Try to get the custom value from trace context.
+     *
+     * @return custom data value.
+     */
+    public static Optional<String> get(String key) {
+        return Optional.empty();
+    }
+
+    /**
+     * Setting the custom key/value into trace context.
+     *
+     * @return previous value if it exists.
+     */
+    public static Optional<String> set(String key, String value) {
+        return Optional.empty();
+    }
 
 Review comment:
   You're saying to unwrap the `Optional<String>`, so we don't need to use `get`(or `orElse`)  and `Strings.isNullOrEmpty` to double-check. It's looking good to me. @wu-sheng any suggestion?

----------------------------------------------------------------
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 #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

codecov-io edited a comment on issue #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#issuecomment-602475846
 
 
   # [Codecov](https://codecov.io/gh/apache/skywalking/pull/4555?src=pr&el=h1) Report
   > Merging [#4555](https://codecov.io/gh/apache/skywalking/pull/4555?src=pr&el=desc) into [master](https://codecov.io/gh/apache/skywalking/commit/f1b2b298b1dcb75f5c211c71b0b956d447859d1d?src=pr&el=desc) will **increase** coverage by `0.1%`.
   > The diff coverage is `54.83%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/skywalking/pull/4555/graphs/tree.svg?width=650&token=qrILxY5yA8&height=150&src=pr)](https://codecov.io/gh/apache/skywalking/pull/4555?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff            @@
   ##           master    #4555     +/-   ##
   =========================================
   + Coverage   25.57%   25.67%   +0.1%     
   =========================================
     Files        1246     1251      +5     
     Lines       28975    29066     +91     
     Branches     3974     3987     +13     
   =========================================
   + Hits         7410     7464     +54     
   - Misses      20873    20913     +40     
   + Partials      692      689      -3
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/skywalking/pull/4555?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [...ng/apm/agent/core/context/MockContextSnapshot.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLXRlc3QtdG9vbHMvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29udGV4dC9Nb2NrQ29udGV4dFNuYXBzaG90LmphdmE=) | `0% <0%> (ø)` | :arrow_up: |
   | [...vation/trace/CorrelationContextGetInterceptor.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLXRvb2xraXQtYWN0aXZhdGlvbi9hcG0tdG9vbGtpdC10cmFjZS1hY3RpdmF0aW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9za3l3YWxraW5nL2FwbS90b29sa2l0L2FjdGl2YXRpb24vdHJhY2UvQ29ycmVsYXRpb25Db250ZXh0R2V0SW50ZXJjZXB0b3IuamF2YQ==) | `0% <0%> (ø)` | |
   | [...vation/trace/CorrelationContextSetInterceptor.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLXRvb2xraXQtYWN0aXZhdGlvbi9hcG0tdG9vbGtpdC10cmFjZS1hY3RpdmF0aW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9za3l3YWxraW5nL2FwbS90b29sa2l0L2FjdGl2YXRpb24vdHJhY2UvQ29ycmVsYXRpb25Db250ZXh0U2V0SW50ZXJjZXB0b3IuamF2YQ==) | `0% <0%> (ø)` | |
   | [...alking/apm/agent/core/context/ContextSnapshot.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29udGV4dC9Db250ZXh0U25hcHNob3QuamF2YQ==) | `0% <0%> (ø)` | :arrow_up: |
   | [...activation/trace/CorrelationContextActivation.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLXRvb2xraXQtYWN0aXZhdGlvbi9hcG0tdG9vbGtpdC10cmFjZS1hY3RpdmF0aW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9za3l3YWxraW5nL2FwbS90b29sa2l0L2FjdGl2YXRpb24vdHJhY2UvQ29ycmVsYXRpb25Db250ZXh0QWN0aXZhdGlvbi5qYXZh) | `0% <0%> (ø)` | |
   | [...walking/apm/agent/core/context/ContextManager.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29udGV4dC9Db250ZXh0TWFuYWdlci5qYXZh) | `50% <0%> (-2.39%)` | :arrow_down: |
   | [.../agent/core/context/SW7CorrelationCarrierItem.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29udGV4dC9TVzdDb3JyZWxhdGlvbkNhcnJpZXJJdGVtLmphdmE=) | `100% <100%> (ø)` | |
   | [.../apache/skywalking/apm/agent/core/conf/Config.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29uZi9Db25maWcuamF2YQ==) | `70.17% <100%> (+1.08%)` | :arrow_up: |
   | [...walking/apm/agent/core/context/ContextCarrier.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29udGV4dC9Db250ZXh0Q2Fycmllci5qYXZh) | `70.96% <100%> (+0.96%)` | :arrow_up: |
   | [...g/apm/agent/core/context/IgnoredTracerContext.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29udGV4dC9JZ25vcmVkVHJhY2VyQ29udGV4dC5qYXZh) | `71.42% <40%> (ø)` | :arrow_up: |
   | ... and [13 more](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/skywalking/pull/4555?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/4555?src=pr&el=footer). Last update [f1b2b29...d6d488c](https://codecov.io/gh/apache/skywalking/pull/4555?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] mrproliu commented on a change in pull request #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

mrproliu commented on a change in pull request #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#discussion_r396533735
 
 

 ##########
 File path: apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/CorrelationContext.java
 ##########
 @@ -0,0 +1,164 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.skywalking.apm.agent.core.context;
+
+import org.apache.skywalking.apm.agent.core.base64.Base64;
+import org.apache.skywalking.apm.agent.core.conf.Config;
+import org.apache.skywalking.apm.util.StringUtil;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Objects;
+import java.util.stream.Collectors;
+
+/**
+ * Correlation context, use to propagation user custom data.
+ * Working on the protocol and delegate set/get method.
+ */
+public class CorrelationContext {
+
+    private volatile Map<String, String> data;
 
 Review comment:
   Change that to final, and use the `extract` and `inject` to operate the data.

----------------------------------------------------------------
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] mrproliu commented on a change in pull request #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

mrproliu commented on a change in pull request #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#discussion_r397135217
 
 

 ##########
 File path: apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/CorrelationContext.java
 ##########
 @@ -0,0 +1,150 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.skywalking.apm.agent.core.context;
+
+import org.apache.skywalking.apm.agent.core.base64.Base64;
+import org.apache.skywalking.apm.agent.core.conf.Config;
+import org.apache.skywalking.apm.util.StringUtil;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Optional;
+import java.util.stream.Collectors;
+
+/**
+ * Correlation context, use to propagation user custom data.
+ * Working on the protocol and delegate set/get method.
+ */
+public class CorrelationContext {
+
+    private final Map<String, String> data;
+
+    public CorrelationContext() {
+        this.data = new HashMap<>(Config.Correlation.ELEMENT_MAX_NUMBER);
+    }
+
+    public Optional<String> set(String key, String value) {
+        // key must not null
+        if (key == null) {
+            return Optional.empty();
+        }
+        if (value == null) {
+            value = "";
+        }
+
+        // check value length
+        if (value.length() > Config.Correlation.VALUE_MAX_LENGTH) {
+            return Optional.empty();
+        }
+
+        // already contain key
+        if (data.containsKey(key)) {
+            final String previousValue = data.put(key, value);
+            return Optional.of(previousValue);
+        }
+
+        // check keys count
+        if (data.size() >= Config.Correlation.ELEMENT_MAX_NUMBER) {
+            return Optional.empty();
+        }
+
+        // setting
+        data.put(key, value);
+        return Optional.empty();
+    }
+
+    public Optional<String> get(String key) {
+        if (key == null) {
+            return Optional.empty();
+        }
+
+        return Optional.ofNullable(data.get(key));
+    }
+
+    /**
+     * Serialize this {@link CorrelationContext} to a {@link String}
+     *
+     * @return the serialization string.
+     */
+    String serialize() {
+        if (data.isEmpty()) {
+            return "";
+        }
+
+        return data.entrySet().stream()
+            .map(entry -> Base64.encode(entry.getKey()) + ":" + Base64.encode(entry.getValue()))
+            .collect(Collectors.joining(","));
+    }
+
+    /**
+     * Deserialize data from {@link String}
+     */
+    void deserialize(String value) {
+        if (StringUtil.isEmpty(value)) {
+            return;
+        }
+
+        for (String perData : value.split(",")) {
+            final String[] parts = perData.split(":");
+            String perDataKey = parts[0];
+            String perDataValue = parts.length > 1 ? parts[1] : "";
+            data.put(Base64.decode2UTFString(perDataKey), Base64.decode2UTFString(perDataValue));
+        }
+    }
+
+    /**
+     * Prepare for the cross-process propagation. Inject the {@link #data} into {@link ContextCarrier#getCorrelationContext()}
+     */
+    void inject(ContextCarrier carrier) {
+        carrier.getCorrelationContext().data.putAll(this.data);
 
 Review comment:
   fixed.

----------------------------------------------------------------
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 #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

wu-sheng commented on a change in pull request #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#discussion_r396416910
 
 

 ##########
 File path: apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/CorrelationContext.java
 ##########
 @@ -0,0 +1,164 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.skywalking.apm.agent.core.context;
+
+import org.apache.skywalking.apm.agent.core.base64.Base64;
+import org.apache.skywalking.apm.agent.core.conf.Config;
+import org.apache.skywalking.apm.util.StringUtil;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Objects;
+import java.util.stream.Collectors;
+
+/**
+ * Correlation context, use to propagation user custom data.
+ * Working on the protocol and delegate set/get method.
+ */
+public class CorrelationContext {
+
+    private volatile Map<String, String> data;
 
 Review comment:
   Why this could be changed? And further as a `volatile`? What are the 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.
 
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 #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

wu-sheng commented on a change in pull request #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#discussion_r397918574
 
 

 ##########
 File path: docs/en/setup/service-agent/java-agent/Application-toolkit-trace.md
 ##########
 @@ -53,3 +53,14 @@ public User methodYouWantToTrace(String param1, String param2) {
 }
 ```
 
+* Use `TraceContext.getCorrelation()` API to put custom data in tracing context. 
+```java
+Optional<String> previous = TraceContext.getCorrelation("customKey", "customValue");
 
 Review comment:
   I think this is a typo of the documentation? get with 2 parameters?

----------------------------------------------------------------
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 #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

wu-sheng commented on a change in pull request #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#discussion_r396420954
 
 

 ##########
 File path: docs/en/setup/service-agent/java-agent/Application-toolkit-correlation.md
 ##########
 @@ -0,0 +1,33 @@
+## Cross Process Correlation
+
+## Introduce
+This plugin is help user to transport custom data in the tracing context. [Follow this to get protocol description.](../../../protocols/Skywalking-Cross-Process-Correlation-Headers-Protocol-v1.md)
+
+## How to use it
+* Dependency the toolkit, such as using maven or gradle
+```xml
+   <dependency>
+      <groupId>org.apache.skywalking</groupId>
+      <artifactId>apm-toolkit-trace</artifactId>
+      <version>${skywalking.version}</version>
 
 Review comment:
   I don't think this feature has anything related to across-thread toolkit. The across-thread thing only happens in the codes level, but for user, they don't care and can't feel it. These are just simple new magic APIs for them.

----------------------------------------------------------------
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 #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

wu-sheng commented on a change in pull request #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#discussion_r396329503
 
 

 ##########
 File path: docs/en/setup/service-agent/java-agent/README.md
 ##########
 @@ -160,6 +162,7 @@ Now, we have the following known bootstrap plugins.
 [log4j2](Application-toolkit-log4j-2.x.md), [logback](Application-toolkit-logback-1.x.md)
     * If you want to use annotations or SkyWalking native APIs to read context, try [SkyWalking manual APIs](Application-toolkit-trace.md)
 
 Review comment:
   ```suggestion
       * If you want your codes interact with SkyWalking agent, including `getting trace id`, `set tags`, etc.. Try [SkyWalking manual APIs](Application-toolkit-trace.md)
   ```

----------------------------------------------------------------
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 #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

wu-sheng commented on a change in pull request #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#discussion_r396955244
 
 

 ##########
 File path: apm-application-toolkit/apm-toolkit-trace/src/main/java/org/apache/skywalking/apm/toolkit/trace/CorrelationContext.java
 ##########
 @@ -0,0 +1,45 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.skywalking.apm.toolkit.trace;
+
+import java.util.Optional;
+
+/**
+ * CorrelationContext is the interactive API for end user to put/set custom data.
+ */
+public class CorrelationContext {
+
+    /**
+     * Try to get the custom value from trace context.
+     *
+     * @return custom data value.
+     */
+    public static Optional<String> get(String key) {
+        return Optional.empty();
+    }
+
+    /**
+     * Setting the custom key/value into trace context.
+     *
+     * @return previous value if it exists.
+     */
+    public static Optional<String> set(String key, String value) {
+        return Optional.empty();
+    }
 
 Review comment:
   As `Optional`, the point is avoiding NPE explicitly for API user. We could guarantee there is no EMPTY_STRING as the value, as we discussed above. Then user doesn't have `doublt-check` case. We also don't ask user to do `.orElse("")` and then `Strings.isNullOrEmpty`. Your demo codes are written by yourself in this way.
   
   I am a little confusing about your conclusion. 

----------------------------------------------------------------
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 #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

wu-sheng commented on a change in pull request #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#discussion_r396871715
 
 

 ##########
 File path: docs/en/setup/service-agent/java-agent/Application-toolkit-trace.md
 ##########
 @@ -53,3 +53,13 @@ public User methodYouWantToTrace(String param1, String param2) {
 }
 ```
 
+* Use `CorrelationContext.set()` API to setting custom data in tracing context. Please follow [the agent configuration](README.md#table-of-agent-configuration-properties) to get more setting limit.
+```java
+optional<String> previous = CorrelationContext.set("customKey", "customValue");
+```
+
+* Use `CorrelationContext.get()` API to get custom data.
+```java
+optional<String> value = CorrelationContext.get("customKey");
+```
+
 
 Review comment:
   ```suggestion
   CorrelationContext configuration descriptions could be found in the [the agent configuration](README.md#table-of-agent-configuration-properties) documentation, with `correlation.` as the prefix.
   ```

----------------------------------------------------------------
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 #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

kezhenxu94 commented on a change in pull request #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#discussion_r396902387
 
 

 ##########
 File path: apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/CorrelationContext.java
 ##########
 @@ -0,0 +1,150 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.skywalking.apm.agent.core.context;
+
+import org.apache.skywalking.apm.agent.core.base64.Base64;
+import org.apache.skywalking.apm.agent.core.conf.Config;
+import org.apache.skywalking.apm.util.StringUtil;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Optional;
+import java.util.stream.Collectors;
+
+/**
+ * Correlation context, use to propagation user custom data.
+ * Working on the protocol and delegate set/get method.
+ */
+public class CorrelationContext {
+
+    private final Map<String, String> data;
+
+    public CorrelationContext() {
+        this.data = new HashMap<>(Config.Correlation.ELEMENT_MAX_NUMBER);
+    }
+
+    public Optional<String> set(String key, String value) {
+        // key must not null
+        if (key == null) {
+            return Optional.empty();
+        }
+        if (value == null) {
+            value = "";
+        }
 
 Review comment:
   How do I remove a key if you consider a `null` as `""`. For example, if I already have 3 items in the context and I want to remove one of them and put another one (different `key`), there's no way to do so

----------------------------------------------------------------
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 #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

wu-sheng commented on a change in pull request #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#discussion_r396328779
 
 

 ##########
 File path: docs/en/setup/service-agent/java-agent/README.md
 ##########
 @@ -160,6 +162,7 @@ Now, we have the following known bootstrap plugins.
 [log4j2](Application-toolkit-log4j-2.x.md), [logback](Application-toolkit-logback-1.x.md)
     * If you want to use annotations or SkyWalking native APIs to read context, try [SkyWalking manual APIs](Application-toolkit-trace.md)
     * If you want to continue traces across thread manually, use [across thread solution APIs](Application-toolkit-trace-cross-thread.md).
+    * If you want to cross process correlation your custom data, use [Correlation solution APIs](Application-toolkit-correlation.md).
 
 Review comment:
   ```suggestion
       * If you want to propagate custom data by using tracing context channel, use [Correlation solution APIs](Application-toolkit-correlation.md).
   ```

----------------------------------------------------------------
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 #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

codecov-io edited a comment on issue #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#issuecomment-602475846
 
 
   # [Codecov](https://codecov.io/gh/apache/skywalking/pull/4555?src=pr&el=h1) Report
   > Merging [#4555](https://codecov.io/gh/apache/skywalking/pull/4555?src=pr&el=desc) into [master](https://codecov.io/gh/apache/skywalking/commit/954eede0ec727222d14cdb41ab584e6354776c49&el=desc) will **increase** coverage by `0.10%`.
   > The diff coverage is `52.57%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/skywalking/pull/4555/graphs/tree.svg?width=650&height=150&src=pr&token=qrILxY5yA8)](https://codecov.io/gh/apache/skywalking/pull/4555?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master    #4555      +/-   ##
   ==========================================
   + Coverage   25.57%   25.67%   +0.10%     
   ==========================================
     Files        1246     1251       +5     
     Lines       28975    29070      +95     
     Branches     3974     3987      +13     
   ==========================================
   + Hits         7410     7464      +54     
   - Misses      20873    20917      +44     
   + Partials      692      689       -3     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/skywalking/pull/4555?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [...walking/apm/agent/core/context/ContextManager.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29udGV4dC9Db250ZXh0TWFuYWdlci5qYXZh) | `50.00% <0.00%> (-2.39%)` | :arrow_down: |
   | [...alking/apm/agent/core/context/ContextSnapshot.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29udGV4dC9Db250ZXh0U25hcHNob3QuamF2YQ==) | `0.00% <0.00%> (ø)` | |
   | [...ng/apm/agent/core/context/MockContextSnapshot.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLXRlc3QtdG9vbHMvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29udGV4dC9Nb2NrQ29udGV4dFNuYXBzaG90LmphdmE=) | `0.00% <0.00%> (ø)` | |
   | [...activation/trace/CorrelationContextActivation.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLXRvb2xraXQtYWN0aXZhdGlvbi9hcG0tdG9vbGtpdC10cmFjZS1hY3RpdmF0aW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9za3l3YWxraW5nL2FwbS90b29sa2l0L2FjdGl2YXRpb24vdHJhY2UvQ29ycmVsYXRpb25Db250ZXh0QWN0aXZhdGlvbi5qYXZh) | `0.00% <0.00%> (ø)` | |
   | [...vation/trace/CorrelationContextGetInterceptor.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLXRvb2xraXQtYWN0aXZhdGlvbi9hcG0tdG9vbGtpdC10cmFjZS1hY3RpdmF0aW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9za3l3YWxraW5nL2FwbS90b29sa2l0L2FjdGl2YXRpb24vdHJhY2UvQ29ycmVsYXRpb25Db250ZXh0R2V0SW50ZXJjZXB0b3IuamF2YQ==) | `0.00% <0.00%> (ø)` | |
   | [...vation/trace/CorrelationContextSetInterceptor.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLXRvb2xraXQtYWN0aXZhdGlvbi9hcG0tdG9vbGtpdC10cmFjZS1hY3RpdmF0aW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9za3l3YWxraW5nL2FwbS90b29sa2l0L2FjdGl2YXRpb24vdHJhY2UvQ29ycmVsYXRpb25Db250ZXh0U2V0SW50ZXJjZXB0b3IuamF2YQ==) | `0.00% <0.00%> (ø)` | |
   | [...g/apm/agent/core/context/IgnoredTracerContext.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29udGV4dC9JZ25vcmVkVHJhY2VyQ29udGV4dC5qYXZh) | `71.42% <40.00%> (ø)` | |
   | [...walking/apm/agent/core/context/TracingContext.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29udGV4dC9UcmFjaW5nQ29udGV4dC5qYXZh) | `61.66% <60.00%> (+0.22%)` | :arrow_up: |
   | [...ing/apm/agent/core/context/CorrelationContext.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29udGV4dC9Db3JyZWxhdGlvbkNvbnRleHQuamF2YQ==) | `76.59% <76.59%> (ø)` | |
   | [.../apache/skywalking/apm/agent/core/conf/Config.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29uZi9Db25maWcuamF2YQ==) | `70.17% <100.00%> (+1.08%)` | :arrow_up: |
   | ... and [13 more](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/skywalking/pull/4555?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/4555?src=pr&el=footer). Last update [954eede...71d076b](https://codecov.io/gh/apache/skywalking/pull/4555?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] mrproliu commented on a change in pull request #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

mrproliu commented on a change in pull request #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#discussion_r396532066
 
 

 ##########
 File path: docs/en/setup/service-agent/java-agent/README.md
 ##########
 @@ -160,6 +162,7 @@ Now, we have the following known bootstrap plugins.
 [log4j2](Application-toolkit-log4j-2.x.md), [logback](Application-toolkit-logback-1.x.md)
     * If you want to use annotations or SkyWalking native APIs to read context, try [SkyWalking manual APIs](Application-toolkit-trace.md)
     * If you want to continue traces across thread manually, use [across thread solution APIs](Application-toolkit-trace-cross-thread.md).
+    * If you want to cross process correlation your custom data, use [Correlation solution APIs](Application-toolkit-correlation.md).
 
 Review comment:
   Delete this file. 

----------------------------------------------------------------
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 #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

wu-sheng commented on a change in pull request #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#discussion_r396951984
 
 

 ##########
 File path: apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/CorrelationContext.java
 ##########
 @@ -0,0 +1,150 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.skywalking.apm.agent.core.context;
+
+import org.apache.skywalking.apm.agent.core.base64.Base64;
+import org.apache.skywalking.apm.agent.core.conf.Config;
+import org.apache.skywalking.apm.util.StringUtil;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Optional;
+import java.util.stream.Collectors;
+
+/**
+ * Correlation context, use to propagation user custom data.
+ * Working on the protocol and delegate set/get method.
+ */
+public class CorrelationContext {
+
+    private final Map<String, String> data;
+
+    public CorrelationContext() {
+        this.data = new HashMap<>(Config.Correlation.ELEMENT_MAX_NUMBER);
+    }
+
+    public Optional<String> set(String key, String value) {
+        // key must not null
+        if (key == null) {
+            return Optional.empty();
+        }
+        if (value == null) {
+            value = "";
+        }
+
+        // check value length
+        if (value.length() > Config.Correlation.VALUE_MAX_LENGTH) {
+            return Optional.empty();
+        }
+
+        // already contain key
+        if (data.containsKey(key)) {
+            final String previousValue = data.put(key, value);
+            return Optional.of(previousValue);
+        }
+
+        // check keys count
+        if (data.size() >= Config.Correlation.ELEMENT_MAX_NUMBER) {
+            return Optional.empty();
+        }
+
+        // setting
+        data.put(key, value);
+        return Optional.empty();
+    }
+
+    public Optional<String> get(String key) {
+        if (key == null) {
+            return Optional.empty();
+        }
+
+        return Optional.ofNullable(data.get(key));
+    }
+
+    /**
+     * Serialize this {@link CorrelationContext} to a {@link String}
+     *
+     * @return the serialization string.
+     */
+    String serialize() {
+        if (data.isEmpty()) {
+            return "";
+        }
+
+        return data.entrySet().stream()
+            .map(entry -> Base64.encode(entry.getKey()) + ":" + Base64.encode(entry.getValue()))
+            .collect(Collectors.joining(","));
+    }
+
+    /**
+     * Deserialize data from {@link String}
+     */
+    void deserialize(String value) {
+        if (StringUtil.isEmpty(value)) {
+            return;
+        }
+
+        for (String perData : value.split(",")) {
+            final String[] parts = perData.split(":");
+            String perDataKey = parts[0];
+            String perDataValue = parts.length > 1 ? parts[1] : "";
+            data.put(Base64.decode2UTFString(perDataKey), Base64.decode2UTFString(perDataValue));
+        }
+    }
+
+    /**
+     * Prepare for the cross-process propagation. Inject the {@link #data} into {@link ContextCarrier#getCorrelationContext()}
+     */
+    void inject(ContextCarrier carrier) {
+        carrier.getCorrelationContext().data.putAll(this.data);
 
 Review comment:
   This is using `putAll`, I am feeling we have a memory leak point. If user to inject a header with more elements than our limitation, `inject` and `deserialize` both wouldn't reject or ignore part of them, then basically, the config of this agent is useless.

----------------------------------------------------------------
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 #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

wu-sheng commented on a change in pull request #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#discussion_r397918749
 
 

 ##########
 File path: docs/en/setup/service-agent/java-agent/Application-toolkit-trace.md
 ##########
 @@ -53,3 +53,14 @@ public User methodYouWantToTrace(String param1, String param2) {
 }
 ```
 
+* Use `TraceContext.getCorrelation()` API to put custom data in tracing context. 
+```java
+Optional<String> previous = TraceContext.getCorrelation("customKey", "customValue");
+```
+CorrelationContext will remove the item when the value is `null` or empty.
+
+* Use `TraceContext.putCorrelation()` API to get custom data.
+```java
+Optional<String> value = TraceContext.putCorrelation("customKey");
 
 Review comment:
   Another typo here.

----------------------------------------------------------------
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 #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

wu-sheng commented on a change in pull request #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#discussion_r396875248
 
 

 ##########
 File path: apm-application-toolkit/apm-toolkit-trace/src/main/java/org/apache/skywalking/apm/toolkit/trace/CorrelationContext.java
 ##########
 @@ -0,0 +1,45 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.skywalking.apm.toolkit.trace;
+
+import java.util.Optional;
+
+/**
+ * Propagate the custom data in the tracing context.
 
 Review comment:
   ```suggestion
    * CorrelationContext is the interactive API for end user to put/set custom data. 
   ```

----------------------------------------------------------------
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 #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

codecov-io edited a comment on issue #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#issuecomment-602475846
 
 
   # [Codecov](https://codecov.io/gh/apache/skywalking/pull/4555?src=pr&el=h1) Report
   > Merging [#4555](https://codecov.io/gh/apache/skywalking/pull/4555?src=pr&el=desc) into [master](https://codecov.io/gh/apache/skywalking/commit/7c9b6cf054f8e534afcc5366e01858b384fd7e8d&el=desc) will **increase** coverage by `0.12%`.
   > The diff coverage is `54.83%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/skywalking/pull/4555/graphs/tree.svg?width=650&height=150&src=pr&token=qrILxY5yA8)](https://codecov.io/gh/apache/skywalking/pull/4555?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master    #4555      +/-   ##
   ==========================================
   + Coverage   25.54%   25.67%   +0.12%     
   ==========================================
     Files        1246     1251       +5     
     Lines       28975    29066      +91     
     Branches     3974     3987      +13     
   ==========================================
   + Hits         7403     7464      +61     
   - Misses      20880    20913      +33     
   + Partials      692      689       -3     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/skywalking/pull/4555?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [...walking/apm/agent/core/context/ContextManager.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29udGV4dC9Db250ZXh0TWFuYWdlci5qYXZh) | `50.00% <0.00%> (-2.39%)` | :arrow_down: |
   | [...alking/apm/agent/core/context/ContextSnapshot.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29udGV4dC9Db250ZXh0U25hcHNob3QuamF2YQ==) | `0.00% <0.00%> (ø)` | |
   | [...ng/apm/agent/core/context/MockContextSnapshot.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLXRlc3QtdG9vbHMvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29udGV4dC9Nb2NrQ29udGV4dFNuYXBzaG90LmphdmE=) | `0.00% <0.00%> (ø)` | |
   | [...activation/trace/CorrelationContextActivation.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLXRvb2xraXQtYWN0aXZhdGlvbi9hcG0tdG9vbGtpdC10cmFjZS1hY3RpdmF0aW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9za3l3YWxraW5nL2FwbS90b29sa2l0L2FjdGl2YXRpb24vdHJhY2UvQ29ycmVsYXRpb25Db250ZXh0QWN0aXZhdGlvbi5qYXZh) | `0.00% <0.00%> (ø)` | |
   | [...vation/trace/CorrelationContextGetInterceptor.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLXRvb2xraXQtYWN0aXZhdGlvbi9hcG0tdG9vbGtpdC10cmFjZS1hY3RpdmF0aW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9za3l3YWxraW5nL2FwbS90b29sa2l0L2FjdGl2YXRpb24vdHJhY2UvQ29ycmVsYXRpb25Db250ZXh0R2V0SW50ZXJjZXB0b3IuamF2YQ==) | `0.00% <0.00%> (ø)` | |
   | [...vation/trace/CorrelationContextSetInterceptor.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLXRvb2xraXQtYWN0aXZhdGlvbi9hcG0tdG9vbGtpdC10cmFjZS1hY3RpdmF0aW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9za3l3YWxraW5nL2FwbS90b29sa2l0L2FjdGl2YXRpb24vdHJhY2UvQ29ycmVsYXRpb25Db250ZXh0U2V0SW50ZXJjZXB0b3IuamF2YQ==) | `0.00% <0.00%> (ø)` | |
   | [...g/apm/agent/core/context/IgnoredTracerContext.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29udGV4dC9JZ25vcmVkVHJhY2VyQ29udGV4dC5qYXZh) | `71.42% <40.00%> (ø)` | |
   | [...walking/apm/agent/core/context/TracingContext.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29udGV4dC9UcmFjaW5nQ29udGV4dC5qYXZh) | `61.66% <60.00%> (+0.22%)` | :arrow_up: |
   | [...ing/apm/agent/core/context/CorrelationContext.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29udGV4dC9Db3JyZWxhdGlvbkNvbnRleHQuamF2YQ==) | `76.59% <76.59%> (ø)` | |
   | [.../apache/skywalking/apm/agent/core/conf/Config.java](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29uZi9Db25maWcuamF2YQ==) | `70.17% <100.00%> (+1.08%)` | :arrow_up: |
   | ... and [12 more](https://codecov.io/gh/apache/skywalking/pull/4555/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/skywalking/pull/4555?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/4555?src=pr&el=footer). Last update [7c9b6cf...b2316ae](https://codecov.io/gh/apache/skywalking/pull/4555?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] mrproliu commented on a change in pull request #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

mrproliu commented on a change in pull request #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#discussion_r396915718
 
 

 ##########
 File path: apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/CorrelationContext.java
 ##########
 @@ -0,0 +1,150 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.skywalking.apm.agent.core.context;
+
+import org.apache.skywalking.apm.agent.core.base64.Base64;
+import org.apache.skywalking.apm.agent.core.conf.Config;
+import org.apache.skywalking.apm.util.StringUtil;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Optional;
+import java.util.stream.Collectors;
+
+/**
+ * Correlation context, use to propagation user custom data.
+ * Working on the protocol and delegate set/get method.
+ */
+public class CorrelationContext {
+
+    private final Map<String, String> data;
+
+    public CorrelationContext() {
+        this.data = new HashMap<>(Config.Correlation.ELEMENT_MAX_NUMBER);
+    }
+
+    public Optional<String> set(String key, String value) {
+        // key must not null
+        if (key == null) {
+            return Optional.empty();
+        }
+        if (value == null) {
+            value = "";
+        }
 
 Review comment:
   Do you mean delete the key when the value is null? Should I add a new method to implement the `remove` 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.
 
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 #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

wu-sheng commented on a change in pull request #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#discussion_r396872086
 
 

 ##########
 File path: docs/en/setup/service-agent/java-agent/README.md
 ##########
 @@ -125,6 +125,8 @@ property key | Description | Default |
 `plugin.tomcat.collect_http_params`| This config item controls that whether the Tomcat plugin should collect the parameters of the request. Also, activate implicitly in the profiled trace. | `false` |
 `plugin.springmvc.collect_http_params`| This config item controls that whether the SpringMVC plugin should collect the parameters of the request, when your Spring application is based on Tomcat, consider only setting either `plugin.tomcat.collect_http_params` or `plugin.springmvc.collect_http_params`. Also, activate implicitly in the profiled trace. | `false` |
 `plugin.http.http_params_length_threshold`| When `COLLECT_HTTP_PARAMS` is enabled, how many characters to keep and send to the OAP backend, use negative values to keep and send the complete parameters, NB. this config item is added for the sake of performance.  | `1024` |
+`correlation.element_max_number`|Correlation max element count in the correlation context.|`3`|
+`correlation.value_max_length`|Correlation max value length in each key.|`128`|
 
 Review comment:
   ```suggestion
   `correlation.value_max_length`|Max value length of correlation context element.|`128`|
   ```

----------------------------------------------------------------
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] mrproliu commented on a change in pull request #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

mrproliu commented on a change in pull request #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#discussion_r396429881
 
 

 ##########
 File path: docs/en/setup/service-agent/java-agent/Application-toolkit-correlation.md
 ##########
 @@ -0,0 +1,33 @@
+## Cross Process Correlation
+
+## Introduce
+This plugin is help user to transport custom data in the tracing context. [Follow this to get protocol description.](../../../protocols/Skywalking-Cross-Process-Correlation-Headers-Protocol-v1.md)
+
+## How to use it
+* Dependency the toolkit, such as using maven or gradle
+```xml
+   <dependency>
+      <groupId>org.apache.skywalking</groupId>
+      <artifactId>apm-toolkit-trace</artifactId>
+      <version>${skywalking.version}</version>
+   </dependency>
+```
+
+* Use `CorrelationContext.set()` API to setting custom data.
+```java
+CorrelationSettingResult settingResult = CorrelationContext.set("customKey", "customValue");
 
 Review comment:
   I think users can record this setting result, such as log this setting result,to confirm whether the setting is successful. I not pretty sure do we need that? any suggestion?

----------------------------------------------------------------
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 #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

wu-sheng commented on a change in pull request #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#discussion_r396849997
 
 

 ##########
 File path: apm-application-toolkit/apm-toolkit-trace/src/main/java/org/apache/skywalking/apm/toolkit/trace/CorrelationContext.java
 ##########
 @@ -0,0 +1,45 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.skywalking.apm.toolkit.trace;
+
+import java.util.Optional;
+
+/**
+ * Propagate the custom data in the tracing context.
+ */
+public class CorrelationContext {
+
+    /**
+     * Try to get the custom value from trace context.
+     *
+     * @return custom data value.
+     */
+    public static Optional<String> get(String key) {
+        return Optional.empty();
+    }
+
+    /**
+     * Setting the custom key/value into trace context.
+     *
+     * @return not empty when override the previous value.
 
 Review comment:
   ```suggestion
        * @return previous value if it exists.
   ```

----------------------------------------------------------------
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] mrproliu commented on issue #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

mrproliu commented on issue #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#issuecomment-603820006
 
 
   > It was best to commit one pull request for one feature
   
   This pull request is only used to provide the Java implementation of the correlation context. I don't understand what you mean?

----------------------------------------------------------------
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 #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

wu-sheng commented on a change in pull request #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#discussion_r396853676
 
 

 ##########
 File path: apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/CorrelationContext.java
 ##########
 @@ -0,0 +1,150 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.skywalking.apm.agent.core.context;
+
+import org.apache.skywalking.apm.agent.core.base64.Base64;
+import org.apache.skywalking.apm.agent.core.conf.Config;
+import org.apache.skywalking.apm.util.StringUtil;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Optional;
+import java.util.stream.Collectors;
+
+/**
+ * Correlation context, use to propagation user custom data.
+ * Working on the protocol and delegate set/get method.
+ */
+public class CorrelationContext {
+
+    private final Map<String, String> data;
+
+    public CorrelationContext() {
+        this.data = new HashMap<>(0);
 
 Review comment:
   0 should be replaced by `ELEMENT_MAX_NUMBER` to avoid resize for most 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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [skywalking] mrproliu commented on a change in pull request #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

mrproliu commented on a change in pull request #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#discussion_r396957075
 
 

 ##########
 File path: apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/CorrelationContext.java
 ##########
 @@ -0,0 +1,150 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.skywalking.apm.agent.core.context;
+
+import org.apache.skywalking.apm.agent.core.base64.Base64;
+import org.apache.skywalking.apm.agent.core.conf.Config;
+import org.apache.skywalking.apm.util.StringUtil;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Optional;
+import java.util.stream.Collectors;
+
+/**
+ * Correlation context, use to propagation user custom data.
+ * Working on the protocol and delegate set/get method.
+ */
+public class CorrelationContext {
+
+    private final Map<String, String> data;
+
+    public CorrelationContext() {
+        this.data = new HashMap<>(Config.Correlation.ELEMENT_MAX_NUMBER);
+    }
+
+    public Optional<String> set(String key, String value) {
+        // key must not null
+        if (key == null) {
+            return Optional.empty();
+        }
+        if (value == null) {
+            value = "";
+        }
+
+        // check value length
+        if (value.length() > Config.Correlation.VALUE_MAX_LENGTH) {
+            return Optional.empty();
+        }
+
+        // already contain key
+        if (data.containsKey(key)) {
+            final String previousValue = data.put(key, value);
+            return Optional.of(previousValue);
+        }
+
+        // check keys count
+        if (data.size() >= Config.Correlation.ELEMENT_MAX_NUMBER) {
+            return Optional.empty();
+        }
+
+        // setting
+        data.put(key, value);
+        return Optional.empty();
+    }
+
+    public Optional<String> get(String key) {
+        if (key == null) {
+            return Optional.empty();
+        }
+
+        return Optional.ofNullable(data.get(key));
+    }
+
+    /**
+     * Serialize this {@link CorrelationContext} to a {@link String}
+     *
+     * @return the serialization string.
+     */
+    String serialize() {
+        if (data.isEmpty()) {
+            return "";
+        }
+
+        return data.entrySet().stream()
+            .map(entry -> Base64.encode(entry.getKey()) + ":" + Base64.encode(entry.getValue()))
+            .collect(Collectors.joining(","));
+    }
+
+    /**
+     * Deserialize data from {@link String}
+     */
+    void deserialize(String value) {
+        if (StringUtil.isEmpty(value)) {
+            return;
+        }
+
+        for (String perData : value.split(",")) {
+            final String[] parts = perData.split(":");
+            String perDataKey = parts[0];
+            String perDataValue = parts.length > 1 ? parts[1] : "";
+            data.put(Base64.decode2UTFString(perDataKey), Base64.decode2UTFString(perDataValue));
+        }
+    }
+
+    /**
+     * Prepare for the cross-process propagation. Inject the {@link #data} into {@link ContextCarrier#getCorrelationContext()}
+     */
+    void inject(ContextCarrier carrier) {
+        carrier.getCorrelationContext().data.putAll(this.data);
 
 Review comment:
   That's true. If another agent approves to setting more than element count or value length. So we will delete their data? 

----------------------------------------------------------------
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 #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

kezhenxu94 commented on a change in pull request #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#discussion_r396896536
 
 

 ##########
 File path: docs/en/setup/service-agent/java-agent/Application-toolkit-trace.md
 ##########
 @@ -53,3 +53,13 @@ public User methodYouWantToTrace(String param1, String param2) {
 }
 ```
 
+* Use `CorrelationContext.set()` API to setting custom data in tracing context. 
+```java
+optional<String> previous = CorrelationContext.set("customKey", "customValue");
+```
+
+* Use `CorrelationContext.get()` API to get custom data.
+```java
+optional<String> value = CorrelationContext.get("customKey");
+```
+CorrelationContext configuration descriptions could be found in the [the agent configuration](README.md#table-of-agent-configuration-properties) documentation, with `correlation.` as the prefix.
 
 Review comment:
   ```suggestion
   CorrelationContext configuration descriptions could be found in [the agent configuration](README.md#table-of-agent-configuration-properties) documentation, with `correlation.` as the prefix.
   ```

----------------------------------------------------------------
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 #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

wu-sheng commented on a change in pull request #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#discussion_r396434085
 
 

 ##########
 File path: docs/en/setup/service-agent/java-agent/Application-toolkit-correlation.md
 ##########
 @@ -0,0 +1,33 @@
+## Cross Process Correlation
+
+## Introduce
+This plugin is help user to transport custom data in the tracing context. [Follow this to get protocol description.](../../../protocols/Skywalking-Cross-Process-Correlation-Headers-Protocol-v1.md)
+
+## How to use it
+* Dependency the toolkit, such as using maven or gradle
+```xml
+   <dependency>
+      <groupId>org.apache.skywalking</groupId>
+      <artifactId>apm-toolkit-trace</artifactId>
+      <version>${skywalking.version}</version>
+   </dependency>
+```
+
+* Use `CorrelationContext.set()` API to setting custom data.
+```java
+CorrelationSettingResult settingResult = CorrelationContext.set("customKey", "customValue");
 
 Review comment:
   The point is, I am not sure who(users) will set interactive codes with an observability API? And why do they do this? 
   From my understanding, they just expect we wouldn't throw exception. An error seem very confusing. Same principle in `getTraceId()`. We never report the reason why `trace id` is N/A or empty string.

----------------------------------------------------------------
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 #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

kezhenxu94 commented on a change in pull request #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#discussion_r396900648
 
 

 ##########
 File path: apm-sniffer/apm-toolkit-activation/apm-toolkit-trace-activation/src/main/java/org/apache/skywalking/apm/toolkit/activation/trace/CorrelationContextGetInterceptor.java
 ##########
 @@ -0,0 +1,49 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.skywalking.apm.toolkit.activation.trace;
+
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import org.apache.skywalking.apm.agent.core.logging.api.ILog;
+import org.apache.skywalking.apm.agent.core.logging.api.LogManager;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.MethodInterceptResult;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.StaticMethodsAroundInterceptor;
+
+import java.lang.reflect.Method;
+import java.util.Optional;
+
+public class CorrelationContextGetInterceptor implements StaticMethodsAroundInterceptor {
+
+    private ILog logger = LogManager.getLogger(CorrelationContextGetInterceptor.class);
 
 Review comment:
   remove 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] wu-sheng commented on a change in pull request #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

wu-sheng commented on a change in pull request #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#discussion_r396959494
 
 

 ##########
 File path: apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/CorrelationContext.java
 ##########
 @@ -0,0 +1,150 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.skywalking.apm.agent.core.context;
+
+import org.apache.skywalking.apm.agent.core.base64.Base64;
+import org.apache.skywalking.apm.agent.core.conf.Config;
+import org.apache.skywalking.apm.util.StringUtil;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Optional;
+import java.util.stream.Collectors;
+
+/**
+ * Correlation context, use to propagation user custom data.
+ * Working on the protocol and delegate set/get method.
+ */
+public class CorrelationContext {
+
+    private final Map<String, String> data;
+
+    public CorrelationContext() {
+        this.data = new HashMap<>(Config.Correlation.ELEMENT_MAX_NUMBER);
+    }
+
+    public Optional<String> set(String key, String value) {
+        // key must not null
+        if (key == null) {
+            return Optional.empty();
+        }
+        if (value == null) {
+            value = "";
+        }
+
+        // check value length
+        if (value.length() > Config.Correlation.VALUE_MAX_LENGTH) {
+            return Optional.empty();
+        }
+
+        // already contain key
+        if (data.containsKey(key)) {
+            final String previousValue = data.put(key, value);
+            return Optional.of(previousValue);
+        }
+
+        // check keys count
+        if (data.size() >= Config.Correlation.ELEMENT_MAX_NUMBER) {
+            return Optional.empty();
+        }
+
+        // setting
+        data.put(key, value);
+        return Optional.empty();
+    }
+
+    public Optional<String> get(String key) {
+        if (key == null) {
+            return Optional.empty();
+        }
+
+        return Optional.ofNullable(data.get(key));
+    }
+
+    /**
+     * Serialize this {@link CorrelationContext} to a {@link String}
+     *
+     * @return the serialization string.
+     */
+    String serialize() {
+        if (data.isEmpty()) {
+            return "";
+        }
+
+        return data.entrySet().stream()
+            .map(entry -> Base64.encode(entry.getKey()) + ":" + Base64.encode(entry.getValue()))
+            .collect(Collectors.joining(","));
+    }
+
+    /**
+     * Deserialize data from {@link String}
+     */
+    void deserialize(String value) {
+        if (StringUtil.isEmpty(value)) {
+            return;
+        }
+
+        for (String perData : value.split(",")) {
+            final String[] parts = perData.split(":");
+            String perDataKey = parts[0];
+            String perDataValue = parts.length > 1 ? parts[1] : "";
+            data.put(Base64.decode2UTFString(perDataKey), Base64.decode2UTFString(perDataValue));
+        }
+    }
+
+    /**
+     * Prepare for the cross-process propagation. Inject the {@link #data} into {@link ContextCarrier#getCorrelationContext()}
+     */
+    void inject(ContextCarrier carrier) {
+        carrier.getCorrelationContext().data.putAll(this.data);
 
 Review comment:
   I think so. We should keep the memory safe as 1st priority.

----------------------------------------------------------------
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 #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

kezhenxu94 commented on a change in pull request #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#discussion_r396961004
 
 

 ##########
 File path: apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/CorrelationContext.java
 ##########
 @@ -0,0 +1,150 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.skywalking.apm.agent.core.context;
+
+import org.apache.skywalking.apm.agent.core.base64.Base64;
+import org.apache.skywalking.apm.agent.core.conf.Config;
+import org.apache.skywalking.apm.util.StringUtil;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Optional;
+import java.util.stream.Collectors;
+
+/**
+ * Correlation context, use to propagation user custom data.
+ * Working on the protocol and delegate set/get method.
+ */
+public class CorrelationContext {
+
+    private final Map<String, String> data;
+
+    public CorrelationContext() {
+        this.data = new HashMap<>(Config.Correlation.ELEMENT_MAX_NUMBER);
+    }
+
+    public Optional<String> set(String key, String value) {
+        // key must not null
+        if (key == null) {
+            return Optional.empty();
+        }
+        if (value == null) {
+            value = "";
+        }
 
 Review comment:
   @mrproliu please perform a removal when `value == null || value.isEmpty()`, and document it, `remove` is unnecessary then (I tend to keep the core API as simple as possible)

----------------------------------------------------------------
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 #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

wu-sheng commented on a change in pull request #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#discussion_r396871171
 
 

 ##########
 File path: docs/en/setup/service-agent/java-agent/Application-toolkit-trace.md
 ##########
 @@ -53,3 +53,13 @@ public User methodYouWantToTrace(String param1, String param2) {
 }
 ```
 
+* Use `CorrelationContext.set()` API to setting custom data in tracing context. Please follow [the agent configuration](README.md#table-of-agent-configuration-properties) to get more setting limit.
 
 Review comment:
   ```suggestion
   * Use `CorrelationContext.set()` API to setting custom data in tracing context. 
   ```

----------------------------------------------------------------
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 #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

wu-sheng commented on a change in pull request #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#discussion_r396416362
 
 

 ##########
 File path: apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/CorrelationContext.java
 ##########
 @@ -0,0 +1,164 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.skywalking.apm.agent.core.context;
+
+import org.apache.skywalking.apm.agent.core.base64.Base64;
+import org.apache.skywalking.apm.agent.core.conf.Config;
+import org.apache.skywalking.apm.util.StringUtil;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Objects;
+import java.util.stream.Collectors;
+
+/**
+ * Correlation context, use to propagation user custom data.
+ * Working on the protocol and delegate set/get method.
+ */
+public class CorrelationContext {
+
+    private volatile Map<String, String> data;
+
+    public CorrelationContext() {
+        this.data = new HashMap<>(0);
+    }
+
+    public SettingResult set(String key, String value) {
+        // key must not null
+        if (key == null) {
+            return SettingResult.buildWithSettingError("Key Cannot be null");
+        }
+        if (value == null) {
+            value = "";
+        }
+
+        // check value length
+        if (value.length() > Config.Correlation.VALUE_LENGTH) {
+            return SettingResult.buildWithSettingError("Out out correlation value length limit");
+        }
+
+        // already contain key
+        if (data.containsKey(key)) {
+            final String previousValue = data.put(key, value);
+            return SettingResult.buildWithSuccess(previousValue);
+        }
+
+        // check keys count
+        if (data.size() >= Config.Correlation.KEY_COUNT) {
+            return SettingResult.buildWithSettingError("Out out correlation key count limit");
+        }
+
+        // setting
+        data.put(key, value);
+        return SettingResult.buildWithSuccess(null);
+    }
+
+    public String get(String key) {
+        if (key == null) {
+            return "";
+        }
+
+        final String value = data.get(key);
+        return value == null ? "" : value;
+    }
+
+    /**
+     * Serialize this {@link CorrelationContext} to a {@link String}
+     *
+     * @return the serialization string.
+     */
+    String serialize() {
+        if (data.isEmpty()) {
+            return "";
+        }
+
+        return data.entrySet().stream()
+            .map(entry -> Base64.encode(entry.getKey()) + ":" + Base64.encode(entry.getValue()))
+            .collect(Collectors.joining(","));
+    }
+
+    /**
+     * Deserialize data from {@link String}
+     */
+    void deserialize(String value) {
+        if (StringUtil.isEmpty(value)) {
+            return;
+        }
+
+        for (String perData : value.split(",")) {
+            final String[] parts = perData.split(":");
+            String perDataKey = parts[0];
+            String perDataValue = parts.length > 1 ? parts[1] : "";
+            data.put(Base64.decode2UTFString(perDataKey), Base64.decode2UTFString(perDataValue));
+        }
+    }
+
+    /**
+     * Reset correlation context from other context
+     */
+    public void resetFrom(CorrelationContext context) {
 
 Review comment:
   Why there is `reset` here? Why does a context require `reset`?

----------------------------------------------------------------
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] mrproliu commented on a change in pull request #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

mrproliu commented on a change in pull request #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#discussion_r396426834
 
 

 ##########
 File path: docs/en/setup/service-agent/java-agent/Application-toolkit-correlation.md
 ##########
 @@ -0,0 +1,33 @@
+## Cross Process Correlation
+
+## Introduce
+This plugin is help user to transport custom data in the tracing context. [Follow this to get protocol description.](../../../protocols/Skywalking-Cross-Process-Correlation-Headers-Protocol-v1.md)
+
+## How to use it
+* Dependency the toolkit, such as using maven or gradle
+```xml
+   <dependency>
+      <groupId>org.apache.skywalking</groupId>
+      <artifactId>apm-toolkit-trace</artifactId>
+      <version>${skywalking.version}</version>
 
 Review comment:
   Sure, I put this into `Application-toolkit-trace.md`.

----------------------------------------------------------------
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 #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

wu-sheng commented on a change in pull request #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#discussion_r396968976
 
 

 ##########
 File path: apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/CorrelationContext.java
 ##########
 @@ -0,0 +1,150 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.skywalking.apm.agent.core.context;
+
+import org.apache.skywalking.apm.agent.core.base64.Base64;
+import org.apache.skywalking.apm.agent.core.conf.Config;
+import org.apache.skywalking.apm.util.StringUtil;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Optional;
+import java.util.stream.Collectors;
+
+/**
+ * Correlation context, use to propagation user custom data.
+ * Working on the protocol and delegate set/get method.
+ */
+public class CorrelationContext {
+
+    private final Map<String, String> data;
+
+    public CorrelationContext() {
+        this.data = new HashMap<>(Config.Correlation.ELEMENT_MAX_NUMBER);
+    }
+
+    public Optional<String> set(String key, String value) {
+        // key must not null
+        if (key == null) {
+            return Optional.empty();
+        }
+        if (value == null) {
+            value = "";
+        }
+
+        // check value length
+        if (value.length() > Config.Correlation.VALUE_MAX_LENGTH) {
+            return Optional.empty();
+        }
+
+        // already contain key
+        if (data.containsKey(key)) {
+            final String previousValue = data.put(key, value);
+            return Optional.of(previousValue);
+        }
+
+        // check keys count
+        if (data.size() >= Config.Correlation.ELEMENT_MAX_NUMBER) {
+            return Optional.empty();
+        }
+
+        // setting
+        data.put(key, value);
+        return Optional.empty();
+    }
+
+    public Optional<String> get(String key) {
+        if (key == null) {
+            return Optional.empty();
+        }
+
+        return Optional.ofNullable(data.get(key));
+    }
+
+    /**
+     * Serialize this {@link CorrelationContext} to a {@link String}
+     *
+     * @return the serialization string.
+     */
+    String serialize() {
+        if (data.isEmpty()) {
+            return "";
+        }
+
+        return data.entrySet().stream()
+            .map(entry -> Base64.encode(entry.getKey()) + ":" + Base64.encode(entry.getValue()))
+            .collect(Collectors.joining(","));
+    }
+
+    /**
+     * Deserialize data from {@link String}
+     */
+    void deserialize(String value) {
+        if (StringUtil.isEmpty(value)) {
+            return;
+        }
+
+        for (String perData : value.split(",")) {
+            final String[] parts = perData.split(":");
+            String perDataKey = parts[0];
+            String perDataValue = parts.length > 1 ? parts[1] : "";
+            data.put(Base64.decode2UTFString(perDataKey), Base64.decode2UTFString(perDataValue));
+        }
+    }
+
+    /**
+     * Prepare for the cross-process propagation. Inject the {@link #data} into {@link ContextCarrier#getCorrelationContext()}
+     */
+    void inject(ContextCarrier carrier) {
+        carrier.getCorrelationContext().data.putAll(this.data);
 
 Review comment:
   My mistake, this comment should target `extract` and `deserialize`, not `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 #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

wu-sheng commented on a change in pull request #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#discussion_r396870792
 
 

 ##########
 File path: apm-sniffer/apm-toolkit-activation/apm-toolkit-trace-activation/src/main/java/org/apache/skywalking/apm/toolkit/activation/trace/CorrelationContextSetInterceptor.java
 ##########
 @@ -0,0 +1,51 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.skywalking.apm.toolkit.activation.trace;
+
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import org.apache.skywalking.apm.agent.core.logging.api.ILog;
+import org.apache.skywalking.apm.agent.core.logging.api.LogManager;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.MethodInterceptResult;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.StaticMethodsAroundInterceptor;
+
+import java.lang.reflect.Method;
+import java.util.Optional;
+
+public class CorrelationContextSetInterceptor implements StaticMethodsAroundInterceptor {
+
+    private ILog logger = LogManager.getLogger(CorrelationContextSetInterceptor.class);
+
+    @Override
+    public void beforeMethod(Class clazz, Method method, Object[] allArguments, Class<?>[] parameterTypes, MethodInterceptResult result) {
+        final String key = (String) allArguments[0];
+        final String value = (String) allArguments[1];
+        final Optional<String> previous = ContextManager.getCorrelationContext().set(key, value);
+
+        result.defineReturnValue(previous);
+    }
+
+    @Override
+    public Object afterMethod(Class clazz, Method method, Object[] allArguments, Class<?>[] parameterTypes, Object ret) {
+        return ret;
+    }
+
+    @Override
+    public void handleMethodException(Class clazz, Method method, Object[] allArguments, Class<?>[] parameterTypes, Throwable t) {
+        logger.error("Failed to setting correlation.", t);
 
 Review comment:
   Same here.

----------------------------------------------------------------
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 #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

wu-sheng commented on a change in pull request #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#discussion_r397144548
 
 

 ##########
 File path: apm-application-toolkit/apm-toolkit-trace/src/main/java/org/apache/skywalking/apm/toolkit/trace/CorrelationContext.java
 ##########
 @@ -0,0 +1,45 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.skywalking.apm.toolkit.trace;
+
+import java.util.Optional;
+
+/**
+ * CorrelationContext is the interactive API for end user to put/set custom data.
+ */
+public class CorrelationContext {
 
 Review comment:
   There are two `CorrelationContext` in your codes. Seems strange. In the old APIs, we have `TraceContext`(for users) and `ContextManager`/`TracingContext` for internal. I suggest we fix the duplicated naming issue.
   
   Such as for users, we could have `TraceContext#putCorrelation`. @kezhenxu94 @mrproliu WDYT?

----------------------------------------------------------------
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 #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

wu-sheng commented on a change in pull request #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#discussion_r396326398
 
 

 ##########
 File path: docs/en/setup/service-agent/java-agent/Application-toolkit-correlation.md
 ##########
 @@ -0,0 +1,33 @@
+## Cross Process Correlation
 
 Review comment:
   Why this is not a part of current toolkit document?

----------------------------------------------------------------
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 #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

wu-sheng commented on a change in pull request #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#discussion_r396870758
 
 

 ##########
 File path: apm-sniffer/apm-toolkit-activation/apm-toolkit-trace-activation/src/main/java/org/apache/skywalking/apm/toolkit/activation/trace/CorrelationContextGetInterceptor.java
 ##########
 @@ -0,0 +1,50 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.skywalking.apm.toolkit.activation.trace;
+
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import org.apache.skywalking.apm.agent.core.logging.api.ILog;
+import org.apache.skywalking.apm.agent.core.logging.api.LogManager;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.MethodInterceptResult;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.StaticMethodsAroundInterceptor;
+
+import java.lang.reflect.Method;
+import java.util.Optional;
+
+public class CorrelationContextGetInterceptor implements StaticMethodsAroundInterceptor {
+
+    private ILog logger = LogManager.getLogger(CorrelationContextGetInterceptor.class);
+
+    @Override
+    public void beforeMethod(Class clazz, Method method, Object[] allArguments, Class<?>[] parameterTypes, MethodInterceptResult result) {
+        final String key = (String) allArguments[0];
+        final Optional<String> data = ContextManager.getCorrelationContext().get(key);
+
+        result.defineReturnValue(data);
+    }
+
+    @Override
+    public Object afterMethod(Class clazz, Method method, Object[] allArguments, Class<?>[] parameterTypes, Object ret) {
+        return ret;
+    }
+
+    @Override
+    public void handleMethodException(Class clazz, Method method, Object[] allArguments, Class<?>[] parameterTypes, Throwable t) {
+        logger.error("Failed to get correlation value.", t);
 
 Review comment:
   Remove this, we don't log unless this is a serious issue.  Can't see the case this will happen.

----------------------------------------------------------------
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 #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

kezhenxu94 merged pull request #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555
 
 
   

----------------------------------------------------------------
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 #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

kezhenxu94 commented on a change in pull request #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#discussion_r396919616
 
 

 ##########
 File path: apm-application-toolkit/apm-toolkit-trace/src/main/java/org/apache/skywalking/apm/toolkit/trace/CorrelationContext.java
 ##########
 @@ -0,0 +1,45 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.skywalking.apm.toolkit.trace;
+
+import java.util.Optional;
+
+/**
+ * CorrelationContext is the interactive API for end user to put/set custom data.
+ */
+public class CorrelationContext {
+
+    /**
+     * Try to get the custom value from trace context.
+     *
+     * @return custom data value.
+     */
+    public static Optional<String> get(String key) {
+        return Optional.empty();
+    }
+
+    /**
+     * Setting the custom key/value into trace context.
+     *
+     * @return previous value if it exists.
+     */
+    public static Optional<String> set(String key, String value) {
+        return Optional.empty();
+    }
 
 Review comment:
   > You're saying to unwrap the `Optional<String>`, so we don't need to use `get`(or `orElse`) and `Strings.isNullOrEmpty` to double-check
   
   Exactly

----------------------------------------------------------------
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] mrproliu commented on a change in pull request #4555: Correlation protocol implement

Posted by GitBox <gi...@apache.org>.

mrproliu commented on a change in pull request #4555: Correlation protocol implement
URL: https://github.com/apache/skywalking/pull/4555#discussion_r397924073
 
 

 ##########
 File path: docs/en/setup/service-agent/java-agent/Application-toolkit-trace.md
 ##########
 @@ -53,3 +53,14 @@ public User methodYouWantToTrace(String param1, String param2) {
 }
 ```
 
+* Use `TraceContext.getCorrelation()` API to put custom data in tracing context. 
+```java
+Optional<String> previous = TraceContext.getCorrelation("customKey", "customValue");
 
 Review comment:
   My mistake. Write in the wrong place.

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