You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by GitBox <gi...@apache.org> on 2020/01/02 15:14:15 UTC
[GitHub] [skywalking] devkanro opened a new pull request #4165: Add kotlin
coroutine plugin
devkanro opened a new pull request #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165
# Skywalking with Kotlin coroutine
This PR provided an auto instrument support plugin for Kotlin coroutine based on context snapshot.
## Checklist
You can merge this PR after all check is done.
- [x] Implementation
- [ ] Test [WIP]
- [ ] Review
## Description
We have known there are some limits with skywalking and coroutine, because of the trace context based on `ThreadLocal`.
But skywalking provided context snapshot for cross-thread tracing, I create this plugin for resolving context losing in Kotlin coroutine.
## Implementation principle
As we know, Kotlin coroutine switches the execution thread by `CoroutineDispatcher`.
01. Create a snapshot of the current context before dispatch the continuation.
02. Then create a coroutine span after thread switched, mark the span continued with the snapshot.
03. Every new span which created in the new thread will be a child of this coroutine span. So we can link those span together in a tracing.
04. After the original runnable executed, we need to stop the coroutine span for cleaning thread state.
----------------------------------------------------------------
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] devkanro edited a comment on issue #4165: Add kotlin
coroutine plugin
Posted by GitBox <gi...@apache.org>.
devkanro edited a comment on issue #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#issuecomment-570762748
> One question to all, does this plugin suppose to optional or default. How wide does routine use in kotlin? Could it have side effect as default for all coroutine?
@wu-sheng
I think this can be divided into four cases.
### Pure Java
Pure java will not exist the interception point, the plugin will not be activated, maybe no additional cost with this case, just 1 classes be loaded.
### Kotlin but no coroutine
Kotlin is becoming popular in the back-end stack, because of Kotlin's practical and straightforward grammar and powerful stdlib. But most people maybe use Kotlin to instead of Java, but no more coroutines or other adv features.
But if you don't have dependent on `org.jetbrains.kotlinx:kotlinx-coroutines-core`, the interception point will not exist too.
### Kotlin with coroutine in business code
Maybe sometimes, people will use coroutine in their business code, and don't care about the tracing in the coroutine.
It will cause some performance loss, about create two context snapshots cost.
### Kotlin with coroutine stack
This is my use case. I'm using a full coroutine stack framework, and it is necessary to make the links traceable.
It will cost about create two context snapshots, a span, a `TracingRunnable` object.
----------------------------------------------------------------
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 #4165: Add kotlin
coroutine plugin
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#issuecomment-570608160
# [Codecov](https://codecov.io/gh/apache/skywalking/pull/4165?src=pr&el=h1) Report
> Merging [#4165](https://codecov.io/gh/apache/skywalking/pull/4165?src=pr&el=desc) into [master](https://codecov.io/gh/apache/skywalking/commit/c237385d33d116864fd9770cabc5cd3052e5e778?src=pr&el=desc) will **decrease** coverage by `0.02%`.
> The diff coverage is `6.45%`.
[](https://codecov.io/gh/apache/skywalking/pull/4165?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #4165 +/- ##
==========================================
- Coverage 26.83% 26.81% -0.03%
==========================================
Files 1163 1163
Lines 25429 25428 -1
Branches 3691 3691
==========================================
- Hits 6825 6819 -6
- Misses 17994 18000 +6
+ Partials 610 609 -1
```
| [Impacted Files](https://codecov.io/gh/apache/skywalking/pull/4165?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [.../apache/skywalking/apm/agent/core/conf/Config.java](https://codecov.io/gh/apache/skywalking/pull/4165/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29uZi9Db25maWcuamF2YQ==) | `66.66% <0%> (-1.34%)` | :arrow_down: |
| [...g/apm/plugin/kotlin/coroutine/TracingRunnable.java](https://codecov.io/gh/apache/skywalking/pull/4165/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvb3B0aW9uYWwtcGx1Z2lucy9rb3RsaW4tY29yb3V0aW5lLXBsdWdpbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2t5d2Fsa2luZy9hcG0vcGx1Z2luL2tvdGxpbi9jb3JvdXRpbmUvVHJhY2luZ1J1bm5hYmxlLmphdmE=) | `0% <0%> (ø)` | :arrow_up: |
| [...commons/interceptor/AbstractMethodInterceptor.java](https://codecov.io/gh/apache/skywalking/pull/4165/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLXNkay1wbHVnaW4vc3ByaW5nLXBsdWdpbnMvbXZjLWFubm90YXRpb24tY29tbW9ucy9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2t5d2Fsa2luZy9hcG0vcGx1Z2luL3NwcmluZy9tdmMvY29tbW9ucy9pbnRlcmNlcHRvci9BYnN0cmFjdE1ldGhvZEludGVyY2VwdG9yLmphdmE=) | `0% <0%> (ø)` | :arrow_up: |
| [...skywalking/apm/agent/core/util/CollectionUtil.java](https://codecov.io/gh/apache/skywalking/pull/4165/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvdXRpbC9Db2xsZWN0aW9uVXRpbC5qYXZh) | `0% <0%> (ø)` | :arrow_up: |
| [...he/skywalking/apm/agent/core/context/tag/Tags.java](https://codecov.io/gh/apache/skywalking/pull/4165/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29udGV4dC90YWcvVGFncy5qYXZh) | `100% <100%> (ø)` | :arrow_up: |
| [.../apm/plugin/tomcat78x/TomcatInvokeInterceptor.java](https://codecov.io/gh/apache/skywalking/pull/4165/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLXNkay1wbHVnaW4vdG9tY2F0LTcueC04LngtcGx1Z2luL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9za3l3YWxraW5nL2FwbS9wbHVnaW4vdG9tY2F0Nzh4L1RvbWNhdEludm9rZUludGVyY2VwdG9yLmphdmE=) | `58.97% <7.14%> (ø)` | :arrow_up: |
| [...apm/agent/core/remote/GRPCStreamServiceStatus.java](https://codecov.io/gh/apache/skywalking/pull/4165/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvcmVtb3RlL0dSUENTdHJlYW1TZXJ2aWNlU3RhdHVzLmphdmE=) | `29.16% <0%> (-25.01%)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/skywalking/pull/4165?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/4165?src=pr&el=footer). Last update [c237385...dcbfaf3](https://codecov.io/gh/apache/skywalking/pull/4165?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] devkanro commented on a change in pull request #4165:
Add kotlin coroutine plugin
Posted by GitBox <gi...@apache.org>.
devkanro commented on a change in pull request #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#discussion_r362513887
##########
File path: apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/trace/SpanLayer.java
##########
@@ -26,7 +26,8 @@
RPC_FRAMEWORK(2),
HTTP(3),
MQ(4),
- CACHE(5);
+ CACHE(5),
+ COROUTINE(6);
Review comment:
Got it, I create it for other incoming coroutine plugins, like goroutine etc.
----------------------------------------------------------------
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 #4165: Add kotlin
coroutine plugin
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#issuecomment-570608160
# [Codecov](https://codecov.io/gh/apache/skywalking/pull/4165?src=pr&el=h1) Report
> Merging [#4165](https://codecov.io/gh/apache/skywalking/pull/4165?src=pr&el=desc) into [master](https://codecov.io/gh/apache/skywalking/commit/99100ed635522fb5142230e7a7c03d8011c2f8a1?src=pr&el=desc) will **increase** coverage by `0.02%`.
> The diff coverage is `0%`.
[](https://codecov.io/gh/apache/skywalking/pull/4165?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #4165 +/- ##
==========================================
+ Coverage 26.78% 26.81% +0.02%
==========================================
Files 1161 1163 +2
Lines 25410 25428 +18
Branches 3689 3691 +2
==========================================
+ Hits 6807 6819 +12
- Misses 17997 18000 +3
- Partials 606 609 +3
```
| [Impacted Files](https://codecov.io/gh/apache/skywalking/pull/4165?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...plugin/kotlin/coroutine/DispatcherInterceptor.java](https://codecov.io/gh/apache/skywalking/pull/4165/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvb3B0aW9uYWwtcGx1Z2lucy9rb3RsaW4tY29yb3V0aW5lLXBsdWdpbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2t5d2Fsa2luZy9hcG0vcGx1Z2luL2tvdGxpbi9jb3JvdXRpbmUvRGlzcGF0Y2hlckludGVyY2VwdG9yLmphdmE=) | `0% <0%> (ø)` | |
| [.../apm/network/trace/component/ComponentsDefine.java](https://codecov.io/gh/apache/skywalking/pull/4165/diff?src=pr&el=tree#diff-YXBtLXByb3RvY29sL2FwbS1uZXR3b3JrL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9za3l3YWxraW5nL2FwbS9uZXR3b3JrL3RyYWNlL2NvbXBvbmVudC9Db21wb25lbnRzRGVmaW5lLmphdmE=) | `0% <0%> (ø)` | :arrow_up: |
| [...g/apm/plugin/kotlin/coroutine/TracingRunnable.java](https://codecov.io/gh/apache/skywalking/pull/4165/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvb3B0aW9uYWwtcGx1Z2lucy9rb3RsaW4tY29yb3V0aW5lLXBsdWdpbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2t5d2Fsa2luZy9hcG0vcGx1Z2luL2tvdGxpbi9jb3JvdXRpbmUvVHJhY2luZ1J1bm5hYmxlLmphdmE=) | `0% <0%> (ø)` | |
| [.../core/remote/ServiceAndEndpointRegisterClient.java](https://codecov.io/gh/apache/skywalking/pull/4165/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvcmVtb3RlL1NlcnZpY2VBbmRFbmRwb2ludFJlZ2lzdGVyQ2xpZW50LmphdmE=) | `44.94% <0%> (+13.48%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/skywalking/pull/4165?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/4165?src=pr&el=footer). Last update [99100ed...b891f1f](https://codecov.io/gh/apache/skywalking/pull/4165?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 #4165:
Add kotlin coroutine plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#discussion_r363029663
##########
File path: docs/en/setup/service-agent/java-agent/agent-optional-plugins/Kotlin-Coroutine-plugin.md
##########
@@ -0,0 +1,30 @@
+# Skywalking with Kotlin coroutine
+This PR provided an auto instrument support plugin for Kotlin coroutine based on context snapshot.
Review comment:
Provided to provides
----------------------------------------------------------------
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 #4165:
Add kotlin coroutine plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#discussion_r363029658
##########
File path: docs/en/setup/service-agent/java-agent/README.md
##########
@@ -135,6 +135,7 @@ Now, we have the following known optional plugins.
* [Customize enhance](Customize-enhance-trace.md) Trace methods based on description files, rather than write plugin or change source codes.
* Plugin of Spring Cloud Gateway 2.1.x in optional plugin folder. Please only active this plugin when you install agent in Spring Gateway. spring-cloud-gateway-2.x-plugin and spring-webflux-5.x-plugin are both required.
* Plugin of Spring Transaction in optional plugin folder. The reason of being optional plugin is, many local span are generated, which also spend more CPU, memory and network.
+* [Plugin of Kotlin coroutine](agent-optional-plugins/Kotlin-Coroutine-plugin.md)
Review comment:
We need to add the reason of optional plugin.
Plugin of Kotlin coroutine provides the tracing across coroutines automatically. As it will add local spans to all across routines scenarios, Please assess the performance impact.
----------------------------------------------------------------
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] devkanro commented on issue #4165: Add kotlin
coroutine plugin
Posted by GitBox <gi...@apache.org>.
devkanro commented on issue #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#issuecomment-570518793
@kezhenxu94
Plugin def file missing the license header, already add it.
It will pass `mvnw checkstyle:check apache-rat:check` checks.
----------------------------------------------------------------
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] devkanro commented on issue #4165: Add kotlin
coroutine plugin
Posted by GitBox <gi...@apache.org>.
devkanro commented on issue #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#issuecomment-570511605
> You should run `./mvnw clean package` locally and passed first.
@wu-sheng
I think it should pass with `mvnw package -DskipTests`?
----------------------------------------------------------------
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 #4165:
Add kotlin coroutine plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#discussion_r363029409
##########
File path: docs/en/setup/service-agent/java-agent/Supported-list.md
##########
@@ -86,6 +86,8 @@
* [Spring @Async](https://github.com/spring-projects/spring-framework) 4.x and 5.x
* Cache
* [Ehcache](https://www.ehcache.org/) 2.x
+* Kotlin
+ * [Coroutine](https://kotlinlang.org/docs/reference/coroutines-overview.html) 1.3.x (Optional²)
Review comment:
You miss this update.
----------------------------------------------------------------
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 #4165:
Add kotlin coroutine plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#discussion_r363029735
##########
File path: docs/en/setup/service-agent/java-agent/agent-optional-plugins/Kotlin-Coroutine-plugin.md
##########
@@ -0,0 +1,30 @@
+# Skywalking with Kotlin coroutine
+This PR provided an auto instrument support plugin for Kotlin coroutine based on context snapshot.
+
+## Description
Review comment:
Description official document should like
SkyWalking provide tracing context propagation inside thread. In order to support Kotlin Coroutine, we provide this additional plugin.
----------------------------------------------------------------
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 #4165:
Add kotlin coroutine plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#discussion_r363029514
##########
File path: docs/en/setup/service-agent/java-agent/agent-optional-plugins/Kotlin-Coroutine-plugin.md
##########
@@ -0,0 +1,30 @@
+# Skywalking with Kotlin coroutine
+This PR provided an auto instrument support plugin for Kotlin coroutine based on context snapshot.
+
+## Description
+We have known there are some limits with skywalking and coroutine, because of the trace context based on `ThreadLocal`.
+
+But skywalking provided context snapshot for cross-thread tracing, I create this plugin for resolving context losing in Kotlin coroutine.
+
+## Implementation principle
+As we know, Kotlin coroutine switches the execution thread by `CoroutineDispatcher`.
+
+01. Create a snapshot of the current context before dispatch the continuation.
+02. Then create a coroutine span after thread switched, mark the span continued with the snapshot.
+03. Every new span which created in the new thread will be a child of this coroutine span. So we can link those span together in a tracing.
+04. After the original runnable executed, we need to stop the coroutine span for cleaning thread state.
+
+## Some screenshots
+### Run without the plugin
+We run a Kotlin coroutine based gRPC server without this coroutine plugin.
+You can find, the one call (client -> server1 -> server2) has been split two tracing paths.
+
+01. Server1 without exit span and server2 tracing path.
+
+02. Server2 tracing path.
+
+
+### Run with the plugin
+With no business code changed, just install the plugin. We can find the tracing paths be connected together. We can get all info of one client call.
Review comment:
1. With no -> Without
1. Business code changed -> changing codes manually.
1. Tracing path -> spans
----------------------------------------------------------------
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 #4165: Add kotlin
coroutine plugin
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#issuecomment-570608160
# [Codecov](https://codecov.io/gh/apache/skywalking/pull/4165?src=pr&el=h1) Report
> Merging [#4165](https://codecov.io/gh/apache/skywalking/pull/4165?src=pr&el=desc) into [master](https://codecov.io/gh/apache/skywalking/commit/99100ed635522fb5142230e7a7c03d8011c2f8a1?src=pr&el=desc) will **increase** coverage by `0.02%`.
> The diff coverage is `0%`.
[](https://codecov.io/gh/apache/skywalking/pull/4165?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #4165 +/- ##
==========================================
+ Coverage 26.78% 26.81% +0.02%
==========================================
Files 1161 1163 +2
Lines 25410 25428 +18
Branches 3689 3691 +2
==========================================
+ Hits 6807 6819 +12
- Misses 17997 18000 +3
- Partials 606 609 +3
```
| [Impacted Files](https://codecov.io/gh/apache/skywalking/pull/4165?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...plugin/kotlin/coroutine/DispatcherInterceptor.java](https://codecov.io/gh/apache/skywalking/pull/4165/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvb3B0aW9uYWwtcGx1Z2lucy9rb3RsaW4tY29yb3V0aW5lLXBsdWdpbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2t5d2Fsa2luZy9hcG0vcGx1Z2luL2tvdGxpbi9jb3JvdXRpbmUvRGlzcGF0Y2hlckludGVyY2VwdG9yLmphdmE=) | `0% <0%> (ø)` | |
| [.../apm/network/trace/component/ComponentsDefine.java](https://codecov.io/gh/apache/skywalking/pull/4165/diff?src=pr&el=tree#diff-YXBtLXByb3RvY29sL2FwbS1uZXR3b3JrL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9za3l3YWxraW5nL2FwbS9uZXR3b3JrL3RyYWNlL2NvbXBvbmVudC9Db21wb25lbnRzRGVmaW5lLmphdmE=) | `0% <0%> (ø)` | :arrow_up: |
| [...g/apm/plugin/kotlin/coroutine/TracingRunnable.java](https://codecov.io/gh/apache/skywalking/pull/4165/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvb3B0aW9uYWwtcGx1Z2lucy9rb3RsaW4tY29yb3V0aW5lLXBsdWdpbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2t5d2Fsa2luZy9hcG0vcGx1Z2luL2tvdGxpbi9jb3JvdXRpbmUvVHJhY2luZ1J1bm5hYmxlLmphdmE=) | `0% <0%> (ø)` | |
| [.../core/remote/ServiceAndEndpointRegisterClient.java](https://codecov.io/gh/apache/skywalking/pull/4165/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvcmVtb3RlL1NlcnZpY2VBbmRFbmRwb2ludFJlZ2lzdGVyQ2xpZW50LmphdmE=) | `44.94% <0%> (+13.48%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/skywalking/pull/4165?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/4165?src=pr&el=footer). Last update [99100ed...b891f1f](https://codecov.io/gh/apache/skywalking/pull/4165?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] devkanro commented on a change in pull request #4165:
Add kotlin coroutine plugin
Posted by GitBox <gi...@apache.org>.
devkanro commented on a change in pull request #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#discussion_r363026020
##########
File path: docs/en/setup/service-agent/java-agent/agent-optional-plugins/Kotlin-Coroutine-plugin.md
##########
@@ -0,0 +1,30 @@
+# Skywalking with Kotlin coroutine
+This PR provided an auto instrument support plugin for Kotlin coroutine based on context snapshot.
+
+## Description
+We have known there are some limits with skywalking and coroutine, because of the trace context based on `ThreadLocal`.
+
+But skywalking provided context snapshot for cross-thread tracing, I create this plugin for resolving context losing in Kotlin coroutine.
+
+## Implementation principle
+As we know, Kotlin coroutine switches the execution thread by `CoroutineDispatcher`.
+
+01. Create a snapshot of the current context before dispatch the continuation.
+02. Then create a coroutine span after thread switched, mark the span continued with the snapshot.
+03. Every new span which created in the new thread will be a child of this coroutine span. So we can link those span together in a tracing.
+04. After the original runnable executed, we need to stop the coroutine span for cleaning thread state.
+
+## Some screenshots
+### Run without the plugin
+We run a Kotlin coroutine based gRPC server without this coroutine plugin.
+You can find, the one call (client -> server1 -> server2) has been split two tracing paths.
+
+01. Server1 without exit span and server2 tracing path.
+
+02. Server2 tracing path.
+
Review comment:
PR created [#76](https://github.com/apache/skywalking-website/pull/76).
----------------------------------------------------------------
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] devkanro commented on issue #4165: Add kotlin
coroutine plugin
Posted by GitBox <gi...@apache.org>.
devkanro commented on issue #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#issuecomment-570242815
@wu-sheng
Is here some strong reason for not accepting `.kt`?
Kotlin is 100% compatible with pure java.
And if you need this plugin, you must be use it in a Kotlin project.
----------------------------------------------------------------
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] arugal commented on issue #4165: Add kotlin coroutine
plugin
Posted by GitBox <gi...@apache.org>.
arugal commented on issue #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#issuecomment-570783236
Good for 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] kezhenxu94 commented on a change in pull request
#4165: Add kotlin coroutine plugin
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on a change in pull request #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#discussion_r363018871
##########
File path: test/plugin/scenarios/kotlin-coroutine-scenario/config/expectedData.yaml
##########
@@ -0,0 +1,121 @@
+# 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.
+registryItems:
+ applications:
+ - {kotlin-coroutine: nq 0}
Review comment:
```suggestion
- {kotlin-coroutine-scenario: nq 0}
```
----------------------------------------------------------------
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] devkanro commented on a change in pull request #4165:
Add kotlin coroutine plugin
Posted by GitBox <gi...@apache.org>.
devkanro commented on a change in pull request #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#discussion_r363025338
##########
File path: test/plugin/scenarios/kotlin-coroutine-scenario/support-version.list
##########
@@ -0,0 +1,16 @@
+# 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
+# "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.
+1.3.0
Review comment:
Maybe 1.0+, Should I list all versions in this file?
Like:
```
1.3.3
1.3.2
1.3.1
1.3.0
# versions
1.1.0
1.0.1
1.0.0
```
Or just major version
```
1.3.0
1.2.0
1.1.0
1.0.0
```
----------------------------------------------------------------
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] devkanro edited a comment on issue #4165: Add kotlin
coroutine plugin
Posted by GitBox <gi...@apache.org>.
devkanro edited a comment on issue #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#issuecomment-570762748
> One question to all, does this plugin suppose to optional or default. How wide does routine use in kotlin? Could it have side effect as default for all coroutine?
@wu-sheng
I think this can be divided into four cases.
### Pure Java
Pure java will not exist the interception point, the plugin will not be activated, maybe no additional cost with this case, just 1 class be loaded.
### Kotlin but no coroutine
Kotlin is becoming popular in the back-end stack, because of Kotlin's practical and straightforward grammar and powerful stdlib. But most people maybe use Kotlin to instead of Java, but no more coroutines or other adv features.
But if you don't have dependent on `org.jetbrains.kotlinx:kotlinx-coroutines-core`, the interception point will not exist too.
### Kotlin with coroutine in business code
Maybe sometimes, people will use coroutine in their business code, and don't care about the tracing in the coroutine.
If you have no activated tracing context, it just case `ContextManager.isActive()` cost.
If you have activated tracing context in the current thread, it will cause some performance loss, about create two context snapshots, a span, and a `TracingRunnable` object.
### Kotlin with coroutine stack
This is my use case. I'm using a full coroutine stack framework, and it is necessary to make the links traceable.
It will cost about create two context snapshots, a span, a `TracingRunnable` object.
----------------------------------------------------------------
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 #4165: Add kotlin
coroutine plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#issuecomment-570512749
And please notice, the plugin test case is required in the plugin pull request. Please finish that part.
----------------------------------------------------------------
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
#4165: Add kotlin coroutine plugin
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on a change in pull request #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#discussion_r362520581
##########
File path: apm-sniffer/bootstrap-plugins/kotlin-coroutine-plugin/pom.xml
##########
@@ -0,0 +1,78 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ ~ 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.
+ ~
+ -->
+
+<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+ <modelVersion>4.0.0</modelVersion>
+ <parent>
+ <groupId>org.apache.skywalking</groupId>
+ <artifactId>bootstrap-plugins</artifactId>
Review comment:
Obviously it’s not bootstrap plugin
----------------------------------------------------------------
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] devkanro commented on a change in pull request #4165:
Add kotlin coroutine plugin
Posted by GitBox <gi...@apache.org>.
devkanro commented on a change in pull request #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#discussion_r362520309
##########
File path: apm-sniffer/bootstrap-plugins/pom.xml
##########
@@ -43,6 +43,7 @@
<modules>
<module>jdk-http-plugin</module>
<module>jdk-threading-plugin</module>
+ <module>kotlin-coroutine-plugin</module>
Review comment:
I have another implementation for this plugin, it based on `ThreadContextElement` which is Kotlin coroutine context for syncing thread context, it needs to create interceptor point in `newCoroutineContext`, but I got some bugs with creating the key object of `ThreadContextElement`(singleton be created twice), I change this plugin into bootstrap plugin, It works fine, I will do more test for it, and I will change it to a normal plugin after those test.
----------------------------------------------------------------
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] devkanro commented on issue #4165: Add kotlin
coroutine plugin
Posted by GitBox <gi...@apache.org>.
devkanro commented on issue #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#issuecomment-570593644
> CI passed. Then you need the plugin test case.
I'm thinking of how to write tests for it.
In my case, I use this plugin with a custom gRPC plugin and a Kotlin coroutine based gRPC framework(Screenshot in the description).
The gRPC plugin and gRPC framework are still developing now, and I can't use it in current tests.
I consider using WebFlux and coroutine based Http Client, and I need test compatibility for Kotlin + WebFlux + Coroutine.
----------------------------------------------------------------
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 #4165:
Add kotlin coroutine plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#discussion_r363005495
##########
File path: apm-sniffer/optional-plugins/kotlin-coroutine-plugin/src/main/java/org/apache/skywalking/apm/plugin/kotlin/coroutine/TracingRunnable.java
##########
@@ -0,0 +1,89 @@
+/*
+ * 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.plugin.kotlin.coroutine;
+
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import org.apache.skywalking.apm.agent.core.context.ContextSnapshot;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.network.trace.component.ComponentsDefine;
+
+/**
+ * {@link Runnable} wrapper with trace context snapshot, it will create span
+ * with context snapshot around {@link Runnable} runs.
+ * <p>
+ * A class implementation will be cheaper cost than lambda with captured
+ * variables implementation.
+ */
+class TracingRunnable implements Runnable {
+ private static final String COROUTINE = "/Kotlin/Coroutine";
+
+ private ContextSnapshot snapshot;
+ private Runnable delegate;
+ private String tracingId;
+
+ private TracingRunnable(ContextSnapshot snapshot, Runnable delegate, String tracingId) {
+ this.snapshot = snapshot;
+ this.delegate = delegate;
+ this.tracingId = tracingId;
+ }
+
+ private TracingRunnable(ContextSnapshot snapshot, Runnable delegate) {
+ this(snapshot, delegate, ContextManager.getGlobalTraceId());
+ }
+
+ /**
+ * Wrap {@link Runnable} by {@link TracingRunnable} if active trace context
+ * existed.
+ *
+ * @param delegate {@link Runnable} to wrap.
+ *
+ * @return Wrapped {@link TracingRunnable} or original {@link Runnable} if
+ * trace context not existed.
+ */
+ public static Runnable wrapOrNot(Runnable delegate) {
+ // Just wrap continuation with active trace context
+ if (ContextManager.isActive()) {
+ return new TracingRunnable(ContextManager.capture(), delegate);
+ } else {
+ return delegate;
+ }
+ }
+
+ @Override
+ public void run() {
+ if (ContextManager.getGlobalTraceId().equals(tracingId)) {
+ // Trace id same with before dispatching, skip restore snapshot.
+ delegate.run();
+ return;
+ }
Review comment:
Please use core API to do this check. `snapshot#isFromCurrent()`. Segment id is more accurate to do thread-switch check
----------------------------------------------------------------
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] devkanro commented on a change in pull request #4165:
Add kotlin coroutine plugin
Posted by GitBox <gi...@apache.org>.
devkanro commented on a change in pull request #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#discussion_r363027906
##########
File path: test/plugin/scenarios/kotlin-coroutine-scenario/support-version.list
##########
@@ -0,0 +1,16 @@
+# 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
+# "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.
+1.3.0
Review comment:
Test passed, it seems to look fine
----------------------------------------------------------------
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 #4165: Add kotlin
coroutine plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#issuecomment-570257436
And, as all thread related plugin, kotlin routine plugin should be in the optional plugin list
----------------------------------------------------------------
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 #4165: Add kotlin
coroutine plugin
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on issue #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#issuecomment-570758800
> what should I fill in
@devkanro how many versions to test, the effective lines in your `test/plugin/scenarios/kotlin-coroutine-scenario/support-version.list`
----------------------------------------------------------------
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] devkanro commented on issue #4165: Add kotlin
coroutine plugin
Posted by GitBox <gi...@apache.org>.
devkanro commented on issue #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#issuecomment-570759065
I am developing on Windows, it seems the plugin test scripts can't run on it.
I have tried to run in WSL, but some mount problems([#2151](https://github.com/docker/for-win/issues/2151)) case it can't run.
----------------------------------------------------------------
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
#4165: Add kotlin coroutine plugin
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on a change in pull request #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#discussion_r363018874
##########
File path: test/plugin/scenarios/kotlin-coroutine-scenario/config/expectedData.yaml
##########
@@ -0,0 +1,121 @@
+# 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.
+registryItems:
+ applications:
+ - {kotlin-coroutine: nq 0}
+ instances:
+ - {kotlin-coroutine: 1}
Review comment:
```suggestion
- {kotlin-coroutine-scenario: 1}
```
----------------------------------------------------------------
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 edited a comment on issue #4165: Add kotlin
coroutine plugin
Posted by GitBox <gi...@apache.org>.
kezhenxu94 edited a comment on issue #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#issuecomment-570406048
@devkanro please add license header in newly-added files, and have you finished the test 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] devkanro commented on issue #4165: Add kotlin
coroutine plugin
Posted by GitBox <gi...@apache.org>.
devkanro commented on issue #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#issuecomment-570515223
> And please notice, the plugin test case is required in the plugin pull request. Please finish that part.
@wu-sheng
Yes, it has been in my checklist.
----------------------------------------------------------------
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 #4165:
Add kotlin coroutine plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#discussion_r362516804
##########
File path: apm-sniffer/bootstrap-plugins/pom.xml
##########
@@ -43,6 +43,7 @@
<modules>
<module>jdk-http-plugin</module>
<module>jdk-threading-plugin</module>
+ <module>kotlin-coroutine-plugin</module>
Review comment:
Why is kotlin coroutine plugin as bootstrap plugin? From my understanding, Kotlin still run inside JVM, and compiled as `.class` files. Bootstrap plugins should be rt.jar level injection. Does koltin plugin require 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 issue #4165: Add kotlin
coroutine plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#issuecomment-570251739
> And if I create some test for this plugin, I must write it in Kotlin, I can't use Kotlin coroutine in Java code.
This is OK, you could write that in the test project.
----------------------------------------------------------------
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 #4165: Add kotlin
coroutine plugin
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on issue #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#issuecomment-570406048
@devkanro please add license header in newly-added files, and have you finish the test 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] devkanro commented on issue #4165: Add kotlin
coroutine plugin
Posted by GitBox <gi...@apache.org>.
devkanro commented on issue #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#issuecomment-570244279
@wu-sheng
And if I create some test for this plugin, I must write it in Kotlin, I can't use Kotlin coroutine in Java code.
----------------------------------------------------------------
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] devkanro commented on issue #4165: Add kotlin
coroutine plugin
Posted by GitBox <gi...@apache.org>.
devkanro commented on issue #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#issuecomment-570758478
> Please modify the plugin test Within action YAML file to make it run on the CI process.
What is the mean of serial in test name in `plugin_test.yml`?
```Yml
SpringAsync_gRPC_KotlinCoroutine: # Is it appropriate to test here?
runs-on: ubuntu-18.04
timeout-minutes: 90
strategy:
fail-fast: true
steps:
- uses: actions/checkout@v1
with:
submodules: true
- uses: actions/cache@v1
with:
path: ~/.m2/repository
key: ${{ runner.os }}-maven-${{ hashFiles('**/pom.xml') }}
restore-keys: |
${{ runner.os }}-maven-
- uses: actions/setup-java@v1
with:
java-version: 8
- name: Build SkyWalking Agent
run: ./mvnw clean package -DskipTests -Pagent >/dev/null
- name: Build the Docker image
run: ./mvnw -f test/plugin/pom.xml clean package -DskipTests docker:build -DBUILD_NO=local >/dev/null
- name: Run spring async 4.3.x-5.1.x (35)
run: bash test/plugin/run.sh spring-async-scenario
- name: Run grpc 1.6.0-1.25.0 (22)
run: bash test/plugin/run.sh grpc-scenario
- name: Run kotlin coroutine (?) # what should I fill in
run: bash test/plugin/run.sh kotlin-coroutine-scenario
```
----------------------------------------------------------------
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 #4165: Add kotlin
coroutine plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#issuecomment-570769206
The four cases look good enough. @kezhenxu94 @arugal @dmsolr What do you think?
----------------------------------------------------------------
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 #4165:
Add kotlin coroutine plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#discussion_r363024593
##########
File path: test/plugin/scenarios/kotlin-coroutine-scenario/support-version.list
##########
@@ -0,0 +1,16 @@
+# 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
+# "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.
+1.3.0
Review comment:
Is only 1.3.0 supported? Or 1.3.x, or 1.0+?
According to the central repo, there are many coroutine releases, https://mvnrepository.com/artifact/org.jetbrains.kotlinx/kotlinx-coroutines-core. We should set the boundary clear.
----------------------------------------------------------------
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] devkanro edited a comment on issue #4165: Add kotlin
coroutine plugin
Posted by GitBox <gi...@apache.org>.
devkanro edited a comment on issue #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#issuecomment-570762748
> One question to all, does this plugin suppose to optional or default. How wide does routine use in kotlin? Could it have side effect as default for all coroutine?
@wu-sheng
I think this can be divided into four cases.
### Pure Java
Pure java will not exist the interception point, the plugin will not be activated, maybe no additional cost with this case, just 1 classes be loaded.
### Kotlin but no coroutine
Kotlin is becoming popular in the back-end stack, because of Kotlin's practical and straightforward grammar and powerful stdlib. But most people maybe use Kotlin to instead of Java, no more coroutines or other adv features.
But if you don't have dependent on `org.jetbrains.kotlinx:kotlinx-coroutines-core`, the interception point will not exist too.
### Kotlin with coroutine in business code
Maybe sometimes, people will use coroutine in their business code, and don't care about the tracing in the coroutine.
It will cause some performance loss, about create two context snapshots cost.
### Kotliun with coroutine stack
This is my use case. I'm using a full coroutine stack framework, and it is necessary to make the links traceable.
It will cost about create two context snapshots, a span, a `TracingRunnable` object.
----------------------------------------------------------------
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 #4165: Add kotlin
coroutine plugin
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#issuecomment-570608160
# [Codecov](https://codecov.io/gh/apache/skywalking/pull/4165?src=pr&el=h1) Report
> Merging [#4165](https://codecov.io/gh/apache/skywalking/pull/4165?src=pr&el=desc) into [master](https://codecov.io/gh/apache/skywalking/commit/6be84cfa784d47ce7b5e3a22dccd1d2c6fd1e105?src=pr&el=desc) will **decrease** coverage by `0.01%`.
> The diff coverage is `0%`.
[](https://codecov.io/gh/apache/skywalking/pull/4165?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #4165 +/- ##
==========================================
- Coverage 26.78% 26.76% -0.02%
==========================================
Files 1161 1163 +2
Lines 25410 25428 +18
Branches 3689 3691 +2
==========================================
Hits 6807 6807
- Misses 17997 18015 +18
Partials 606 606
```
| [Impacted Files](https://codecov.io/gh/apache/skywalking/pull/4165?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...plugin/kotlin/coroutine/DispatcherInterceptor.java](https://codecov.io/gh/apache/skywalking/pull/4165/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvb3B0aW9uYWwtcGx1Z2lucy9rb3RsaW4tY29yb3V0aW5lLXBsdWdpbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2t5d2Fsa2luZy9hcG0vcGx1Z2luL2tvdGxpbi9jb3JvdXRpbmUvRGlzcGF0Y2hlckludGVyY2VwdG9yLmphdmE=) | `0% <0%> (ø)` | |
| [.../apm/network/trace/component/ComponentsDefine.java](https://codecov.io/gh/apache/skywalking/pull/4165/diff?src=pr&el=tree#diff-YXBtLXByb3RvY29sL2FwbS1uZXR3b3JrL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9za3l3YWxraW5nL2FwbS9uZXR3b3JrL3RyYWNlL2NvbXBvbmVudC9Db21wb25lbnRzRGVmaW5lLmphdmE=) | `0% <0%> (ø)` | :arrow_up: |
| [...g/apm/plugin/kotlin/coroutine/TracingRunnable.java](https://codecov.io/gh/apache/skywalking/pull/4165/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvb3B0aW9uYWwtcGx1Z2lucy9rb3RsaW4tY29yb3V0aW5lLXBsdWdpbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2t5d2Fsa2luZy9hcG0vcGx1Z2luL2tvdGxpbi9jb3JvdXRpbmUvVHJhY2luZ1J1bm5hYmxlLmphdmE=) | `0% <0%> (ø)` | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/skywalking/pull/4165?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/4165?src=pr&el=footer). Last update [6be84cf...5abf643](https://codecov.io/gh/apache/skywalking/pull/4165?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 #4165:
Add kotlin coroutine plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#discussion_r363005806
##########
File path: apm-sniffer/optional-plugins/kotlin-coroutine-plugin/src/main/java/org/apache/skywalking/apm/plugin/kotlin/coroutine/TracingRunnable.java
##########
@@ -0,0 +1,89 @@
+/*
+ * 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.plugin.kotlin.coroutine;
+
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import org.apache.skywalking.apm.agent.core.context.ContextSnapshot;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.network.trace.component.ComponentsDefine;
+
+/**
+ * {@link Runnable} wrapper with trace context snapshot, it will create span
+ * with context snapshot around {@link Runnable} runs.
+ * <p>
+ * A class implementation will be cheaper cost than lambda with captured
+ * variables implementation.
+ */
+class TracingRunnable implements Runnable {
+ private static final String COROUTINE = "/Kotlin/Coroutine";
+
+ private ContextSnapshot snapshot;
+ private Runnable delegate;
+ private String tracingId;
+
+ private TracingRunnable(ContextSnapshot snapshot, Runnable delegate, String tracingId) {
+ this.snapshot = snapshot;
+ this.delegate = delegate;
+ this.tracingId = tracingId;
+ }
+
+ private TracingRunnable(ContextSnapshot snapshot, Runnable delegate) {
+ this(snapshot, delegate, ContextManager.getGlobalTraceId());
+ }
+
+ /**
+ * Wrap {@link Runnable} by {@link TracingRunnable} if active trace context
+ * existed.
+ *
+ * @param delegate {@link Runnable} to wrap.
+ *
+ * @return Wrapped {@link TracingRunnable} or original {@link Runnable} if
+ * trace context not existed.
+ */
+ public static Runnable wrapOrNot(Runnable delegate) {
+ // Just wrap continuation with active trace context
+ if (ContextManager.isActive()) {
+ return new TracingRunnable(ContextManager.capture(), delegate);
+ } else {
+ return delegate;
+ }
+ }
+
+ @Override
+ public void run() {
+ if (ContextManager.getGlobalTraceId().equals(tracingId)) {
+ // Trace id same with before dispatching, skip restore snapshot.
+ delegate.run();
+ return;
+ }
Review comment:
Before doing this, you need to do `ContextManager#isActive`, making sure there is a context activated in the current thread.
----------------------------------------------------------------
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 #4165: Add kotlin
coroutine plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#issuecomment-570512275
CI indicates very clear, still have license header check issue. You could read the logs by yourself.
----------------------------------------------------------------
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 #4165: Add kotlin
coroutine plugin
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on issue #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#issuecomment-570516266
> > You should run `./mvnw clean package` locally and passed first.
>
> @wu-sheng
> I think it should pass with `mvnw package -DskipTests`?
If you think running tests and packaging locally is too time-consuming, there's a minimum requirement that you **MUST** check locally first before committing and pushing:
```shell
./mvnw checkstyle:check apache-rat:check
```
----------------------------------------------------------------
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] devkanro edited a comment on issue #4165: Add kotlin
coroutine plugin
Posted by GitBox <gi...@apache.org>.
devkanro edited a comment on issue #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#issuecomment-570515223
> And please notice, the plugin test case is required in the plugin pull request. Please finish that part.
@wu-sheng
Yes, it has been on my checklist.
----------------------------------------------------------------
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] devkanro commented on issue #4165: Add kotlin
coroutine plugin
Posted by GitBox <gi...@apache.org>.
devkanro commented on issue #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#issuecomment-570512481
> CI indicates very clear, still have license header check issue. You could read the logs by yourself.
@wu-sheng
I will check it later
----------------------------------------------------------------
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 #4165: Add kotlin
coroutine plugin
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#issuecomment-570608160
# [Codecov](https://codecov.io/gh/apache/skywalking/pull/4165?src=pr&el=h1) Report
> Merging [#4165](https://codecov.io/gh/apache/skywalking/pull/4165?src=pr&el=desc) into [master](https://codecov.io/gh/apache/skywalking/commit/99100ed635522fb5142230e7a7c03d8011c2f8a1?src=pr&el=desc) will **decrease** coverage by `0.01%`.
> The diff coverage is `0%`.
[](https://codecov.io/gh/apache/skywalking/pull/4165?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #4165 +/- ##
==========================================
- Coverage 26.78% 26.76% -0.02%
==========================================
Files 1161 1163 +2
Lines 25410 25428 +18
Branches 3689 3691 +2
==========================================
Hits 6807 6807
- Misses 17997 18015 +18
Partials 606 606
```
| [Impacted Files](https://codecov.io/gh/apache/skywalking/pull/4165?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...plugin/kotlin/coroutine/DispatcherInterceptor.java](https://codecov.io/gh/apache/skywalking/pull/4165/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvb3B0aW9uYWwtcGx1Z2lucy9rb3RsaW4tY29yb3V0aW5lLXBsdWdpbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2t5d2Fsa2luZy9hcG0vcGx1Z2luL2tvdGxpbi9jb3JvdXRpbmUvRGlzcGF0Y2hlckludGVyY2VwdG9yLmphdmE=) | `0% <0%> (ø)` | |
| [.../apm/network/trace/component/ComponentsDefine.java](https://codecov.io/gh/apache/skywalking/pull/4165/diff?src=pr&el=tree#diff-YXBtLXByb3RvY29sL2FwbS1uZXR3b3JrL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9za3l3YWxraW5nL2FwbS9uZXR3b3JrL3RyYWNlL2NvbXBvbmVudC9Db21wb25lbnRzRGVmaW5lLmphdmE=) | `0% <0%> (ø)` | :arrow_up: |
| [...g/apm/plugin/kotlin/coroutine/TracingRunnable.java](https://codecov.io/gh/apache/skywalking/pull/4165/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvb3B0aW9uYWwtcGx1Z2lucy9rb3RsaW4tY29yb3V0aW5lLXBsdWdpbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2t5d2Fsa2luZy9hcG0vcGx1Z2luL2tvdGxpbi9jb3JvdXRpbmUvVHJhY2luZ1J1bm5hYmxlLmphdmE=) | `0% <0%> (ø)` | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/skywalking/pull/4165?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/4165?src=pr&el=footer). Last update [99100ed...b891f1f](https://codecov.io/gh/apache/skywalking/pull/4165?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] devkanro commented on issue #4165: Add kotlin
coroutine plugin
Posted by GitBox <gi...@apache.org>.
devkanro commented on issue #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#issuecomment-570762748
> One question to all, does this plugin suppose to optional or default. How wide does routine use in kotlin? Could it have side effect as default for all coroutine?
@wu-sheng
I think this can be divided into four cases.
### Pure Java
Pure java will not exist the interception point, the plugin will not be activated, maybe no additional cost with this case, just 3 classes be loaded.
### Kotlin but no coroutine
Kotlin is becoming popular in the back-end stack, because of Kotlin's practical and straightforward grammar and powerful stdlib. But most people maybe use Kotlin to instead of Java, no more coroutines or other adv features.
But if you don't have dependent on `org.jetbrains.kotlinx:kotlinx-coroutines-core`, the interception point will not exist too.
### Kotlin with coroutine in business code
Maybe sometimes, people will use coroutine in their business code, and don't care about the tracing in the coroutine.
It will cause some performance loss, about create two context snapshots cost.
### Kotliun with coroutine stack
This is my use case. I'm using a full coroutine stack framework, and it is necessary to make the links traceable.
It will cost about create two context snapshots, a span, a `TracingRunnable` object.
----------------------------------------------------------------
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 #4165: Add kotlin
coroutine plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#issuecomment-570759238
Sorry, the scripts support Linux and Mac only. FYI, @dmsolr
----------------------------------------------------------------
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] devkanro commented on issue #4165: Add kotlin
coroutine plugin
Posted by GitBox <gi...@apache.org>.
devkanro commented on issue #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#issuecomment-570761742
> Some changes are suggested inline, and after fixing those, the tests still failed
@kezhenxu94
I have changed the check data `expectedData.yaml`, maybe it could match the result.
And I have a question about the 'parentTraceSegmentId' in ref.
The `[1]` in `${kotlin-coroutine-scenario[1]}` could be defined segment in my expectedData.yaml or the actual segments?
----------------------------------------------------------------
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] devkanro commented on a change in pull request #4165:
Add kotlin coroutine plugin
Posted by GitBox <gi...@apache.org>.
devkanro commented on a change in pull request #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#discussion_r362523593
##########
File path: apm-sniffer/bootstrap-plugins/kotlin-coroutine-plugin/pom.xml
##########
@@ -0,0 +1,78 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ ~ 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.
+ ~
+ -->
+
+<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+ <modelVersion>4.0.0</modelVersion>
+ <parent>
+ <groupId>org.apache.skywalking</groupId>
+ <artifactId>bootstrap-plugins</artifactId>
+ <version>7.0.0-SNAPSHOT</version>
+ </parent>
+
+ <artifactId>apm-kotlin-coroutine-plugin</artifactId>
+ <name>kotlin-coroutine-plugin</name>
+ <packaging>jar</packaging>
+
+ <properties>
+ <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
+ <kotlin.version>1.3.61</kotlin.version>
+ <kotlinx.coroutine.version>1.3.3</kotlinx.coroutine.version>
+ <kotlin.compiler.incremental>true</kotlin.compiler.incremental>
+ </properties>
+
+ <dependencies>
+ <dependency>
+ <groupId>org.jetbrains.kotlin</groupId>
+ <artifactId>kotlin-stdlib</artifactId>
Review comment:
Yeah, you are right, I assumed people who will use this plugin must be a Kotlin project, I will try to rewrite it in java.
----------------------------------------------------------------
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 #4165: Add kotlin
coroutine plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#issuecomment-570594635
This is a very simple test, you are testing `coroutine`, so basically, if a Tomcat serves HTTP, and call coroutine API to do a DB(H2) access. Then, you have your scenario, right?
----------------------------------------------------------------
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 #4165: Add kotlin
coroutine plugin
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on issue #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#issuecomment-570246219
> @wu-sheng
> Is here some strong reason for not accepting `.kt`?
> Kotlin is 100% compatible with pure java.
Yes it’s 100% interactive with Java, but the standard lib of kotlin is not built-in in JDK, so it breaks the plugins when users are using pure Java.
> And if you need this plugin, you must be use it in a Kotlin project.
I’m afraid not, they’re both JVM language, and are compiled to byte codes, and bytebuddy is manipulating the byte codes, maybe you can just try using pure Java to rewrite the plugin, and use Kotlin to write the test codes, we don’t have strong limitations on test codes
> I have some experience in Kotlin programming and I’d like to follow this PR @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] wu-sheng commented on a change in pull request #4165:
Add kotlin coroutine plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#discussion_r363029418
##########
File path: docs/en/setup/service-agent/java-agent/agent-optional-plugins/Kotlin-Coroutine-plugin.md
##########
@@ -0,0 +1,30 @@
+# Skywalking with Kotlin coroutine
+This PR provided an auto instrument support plugin for Kotlin coroutine based on context snapshot.
Review comment:
This plugin, not PR
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] codecov-io edited a comment on issue #4165: Add kotlin
coroutine plugin
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#issuecomment-570608160
# [Codecov](https://codecov.io/gh/apache/skywalking/pull/4165?src=pr&el=h1) Report
> Merging [#4165](https://codecov.io/gh/apache/skywalking/pull/4165?src=pr&el=desc) into [master](https://codecov.io/gh/apache/skywalking/commit/99100ed635522fb5142230e7a7c03d8011c2f8a1?src=pr&el=desc) will **increase** coverage by `0.02%`.
> The diff coverage is `0%`.
[](https://codecov.io/gh/apache/skywalking/pull/4165?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #4165 +/- ##
==========================================
+ Coverage 26.78% 26.81% +0.02%
==========================================
Files 1161 1163 +2
Lines 25410 25428 +18
Branches 3689 3691 +2
==========================================
+ Hits 6807 6819 +12
- Misses 17997 18000 +3
- Partials 606 609 +3
```
| [Impacted Files](https://codecov.io/gh/apache/skywalking/pull/4165?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...plugin/kotlin/coroutine/DispatcherInterceptor.java](https://codecov.io/gh/apache/skywalking/pull/4165/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvb3B0aW9uYWwtcGx1Z2lucy9rb3RsaW4tY29yb3V0aW5lLXBsdWdpbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2t5d2Fsa2luZy9hcG0vcGx1Z2luL2tvdGxpbi9jb3JvdXRpbmUvRGlzcGF0Y2hlckludGVyY2VwdG9yLmphdmE=) | `0% <0%> (ø)` | |
| [.../apm/network/trace/component/ComponentsDefine.java](https://codecov.io/gh/apache/skywalking/pull/4165/diff?src=pr&el=tree#diff-YXBtLXByb3RvY29sL2FwbS1uZXR3b3JrL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9za3l3YWxraW5nL2FwbS9uZXR3b3JrL3RyYWNlL2NvbXBvbmVudC9Db21wb25lbnRzRGVmaW5lLmphdmE=) | `0% <0%> (ø)` | :arrow_up: |
| [...g/apm/plugin/kotlin/coroutine/TracingRunnable.java](https://codecov.io/gh/apache/skywalking/pull/4165/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvb3B0aW9uYWwtcGx1Z2lucy9rb3RsaW4tY29yb3V0aW5lLXBsdWdpbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2t5d2Fsa2luZy9hcG0vcGx1Z2luL2tvdGxpbi9jb3JvdXRpbmUvVHJhY2luZ1J1bm5hYmxlLmphdmE=) | `0% <0%> (ø)` | |
| [.../core/remote/ServiceAndEndpointRegisterClient.java](https://codecov.io/gh/apache/skywalking/pull/4165/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvcmVtb3RlL1NlcnZpY2VBbmRFbmRwb2ludFJlZ2lzdGVyQ2xpZW50LmphdmE=) | `44.94% <0%> (+13.48%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/skywalking/pull/4165?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/4165?src=pr&el=footer). Last update [99100ed...dcbfaf3](https://codecov.io/gh/apache/skywalking/pull/4165?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] devkanro commented on a change in pull request #4165:
Add kotlin coroutine plugin
Posted by GitBox <gi...@apache.org>.
devkanro commented on a change in pull request #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#discussion_r362742533
##########
File path: apm-sniffer/optional-plugins/kotlin-coroutine-plugin/src/main/java/org/apache/skywalking/apm/plugin/kotlin/coroutine/TracingRunnable.java
##########
@@ -0,0 +1,89 @@
+/*
+ * 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.plugin.kotlin.coroutine;
+
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import org.apache.skywalking.apm.agent.core.context.ContextSnapshot;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.network.trace.component.ComponentsDefine;
+
+/**
+ * {@link Runnable} wrapper with trace context snapshot, it will create span
+ * with context snapshot around {@link Runnable} runs.
+ * <p>
+ * A class implementation will be cheaper cost than lambda with captured
+ * variables implementation.
+ */
+class TracingRunnable implements Runnable {
+ private static final String COROUTINE = "/Kotlin/Coroutine";
+
+ private ContextSnapshot snapshot;
+ private Runnable delegate;
+ private String tracingId;
+
+ private TracingRunnable(ContextSnapshot snapshot, Runnable delegate, String tracingId) {
+ this.snapshot = snapshot;
+ this.delegate = delegate;
+ this.tracingId = tracingId;
+ }
+
+ private TracingRunnable(ContextSnapshot snapshot, Runnable delegate) {
+ this(snapshot, delegate, ContextManager.getGlobalTraceId());
+ }
+
+ /**
+ * Wrap {@link Runnable} by {@link TracingRunnable} if active trace context
+ * existed.
+ *
+ * @param delegate {@link Runnable} to wrap.
+ *
+ * @return Wrapped {@link TracingRunnable} or original {@link Runnable} if
+ * trace context not existed.
+ */
+ public static Runnable wrapOrNot(Runnable delegate) {
+ // Just wrap continuation with active trace context
+ if (ContextManager.isActive()) {
+ return new TracingRunnable(ContextManager.capture(), delegate);
+ } else {
+ return delegate;
+ }
+ }
+
+ @Override
+ public void run() {
+ if (ContextManager.getGlobalTraceId().equals(tracingId)) {
+ // Trace id same with before dispatching, skip restore snapshot.
+ delegate.run();
+ return;
+ }
Review comment:
Sometimes coroutine or suspend function will not switch the thread, we can skip creating a "/Kotlin/Coroutine" span for simplifying tracing.
----------------------------------------------------------------
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 #4165: Add kotlin
coroutine plugin
Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#issuecomment-570608160
# [Codecov](https://codecov.io/gh/apache/skywalking/pull/4165?src=pr&el=h1) Report
> Merging [#4165](https://codecov.io/gh/apache/skywalking/pull/4165?src=pr&el=desc) into [master](https://codecov.io/gh/apache/skywalking/commit/99100ed635522fb5142230e7a7c03d8011c2f8a1?src=pr&el=desc) will **increase** coverage by `0.02%`.
> The diff coverage is `0%`.
[](https://codecov.io/gh/apache/skywalking/pull/4165?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #4165 +/- ##
==========================================
+ Coverage 26.78% 26.81% +0.02%
==========================================
Files 1161 1163 +2
Lines 25410 25430 +20
Branches 3689 3691 +2
==========================================
+ Hits 6807 6819 +12
- Misses 17997 18002 +5
- Partials 606 609 +3
```
| [Impacted Files](https://codecov.io/gh/apache/skywalking/pull/4165?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...plugin/kotlin/coroutine/DispatcherInterceptor.java](https://codecov.io/gh/apache/skywalking/pull/4165/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvb3B0aW9uYWwtcGx1Z2lucy9rb3RsaW4tY29yb3V0aW5lLXBsdWdpbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2t5d2Fsa2luZy9hcG0vcGx1Z2luL2tvdGxpbi9jb3JvdXRpbmUvRGlzcGF0Y2hlckludGVyY2VwdG9yLmphdmE=) | `0% <0%> (ø)` | |
| [.../apm/network/trace/component/ComponentsDefine.java](https://codecov.io/gh/apache/skywalking/pull/4165/diff?src=pr&el=tree#diff-YXBtLXByb3RvY29sL2FwbS1uZXR3b3JrL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9za3l3YWxraW5nL2FwbS9uZXR3b3JrL3RyYWNlL2NvbXBvbmVudC9Db21wb25lbnRzRGVmaW5lLmphdmE=) | `0% <0%> (ø)` | :arrow_up: |
| [...g/apm/plugin/kotlin/coroutine/TracingRunnable.java](https://codecov.io/gh/apache/skywalking/pull/4165/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvb3B0aW9uYWwtcGx1Z2lucy9rb3RsaW4tY29yb3V0aW5lLXBsdWdpbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2t5d2Fsa2luZy9hcG0vcGx1Z2luL2tvdGxpbi9jb3JvdXRpbmUvVHJhY2luZ1J1bm5hYmxlLmphdmE=) | `0% <0%> (ø)` | |
| [.../core/remote/ServiceAndEndpointRegisterClient.java](https://codecov.io/gh/apache/skywalking/pull/4165/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvcmVtb3RlL1NlcnZpY2VBbmRFbmRwb2ludFJlZ2lzdGVyQ2xpZW50LmphdmE=) | `44.94% <0%> (+13.48%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/skywalking/pull/4165?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/4165?src=pr&el=footer). Last update [99100ed...6927733](https://codecov.io/gh/apache/skywalking/pull/4165?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
#4165: Add kotlin coroutine plugin
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on a change in pull request #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#discussion_r363018858
##########
File path: test/plugin/scenarios/kotlin-coroutine-scenario/pom.xml
##########
@@ -0,0 +1,157 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ ~ 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.
+ ~
+ -->
+<project xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xmlns="http://maven.apache.org/POM/4.0.0"
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+
+ <groupId>org.apache.skywalking.apm.testcase</groupId>
+ <artifactId>kotlin-coroutine-scenario</artifactId>
+ <version>1.0.0</version>
+ <packaging>jar</packaging>
+
+ <modelVersion>4.0.0</modelVersion>
+
+ <properties>
+ <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
+ <compiler.version>1.8</compiler.version>
+ <os-maven-plugin.version>1.6.2</os-maven-plugin.version>
+
+ <spring-boot-version>2.1.6.RELEASE</spring-boot-version>
+ <kotlin.version>1.3.61</kotlin.version>
+ <kotlinx.coroutine.version>1.3.3</kotlinx.coroutine.version>
+ </properties>
+
+ <name>skywalking-kotlin-coroutine-scenario</name>
+
+ <dependencyManagement>
+ <dependencies>
+ <dependency>
+ <groupId>org.springframework.boot</groupId>
+ <artifactId>spring-boot-dependencies</artifactId>
+ <version>${spring-boot-version}</version>
+ <type>pom</type>
+ <scope>import</scope>
+ </dependency>
+ </dependencies>
+ </dependencyManagement>
+
+ <dependencies>
+ <dependency>
+ <groupId>org.springframework.boot</groupId>
+ <artifactId>spring-boot-starter-web</artifactId>
+ <exclusions>
+ <exclusion>
+ <groupId>org.springframework.boot</groupId>
+ <artifactId>spring-boot-starter-logging</artifactId>
+ </exclusion>
+ </exclusions>
+ </dependency>
+ <dependency>
+ <groupId>org.springframework.boot</groupId>
+ <artifactId>spring-boot-starter-log4j2</artifactId>
+ <exclusions>
+ <exclusion>
+ <artifactId>jul-to-slf4j</artifactId>
+ <groupId>org.slf4j</groupId>
+ </exclusion>
+ </exclusions>
+ </dependency>
+ <dependency>
+ <groupId>com.h2database</groupId>
+ <artifactId>h2</artifactId>
+ <version>1.4.196</version>
+ </dependency>
+ <dependency>
+ <groupId>commons-dbcp</groupId>
+ <artifactId>commons-dbcp</artifactId>
+ <version>1.4</version>
+ </dependency>
+
+ <dependency>
+ <groupId>org.jetbrains.kotlin</groupId>
+ <artifactId>kotlin-stdlib</artifactId>
+ <version>${kotlin.version}</version>
+ </dependency>
+ <dependency>
+ <groupId>org.jetbrains.kotlinx</groupId>
+ <artifactId>kotlinx-coroutines-core</artifactId>
+ <version>${kotlinx.coroutine.version}</version>
+ </dependency>
+ </dependencies>
+
+ <build>
+ <finalName>kotlin-coroutine-scenario</finalName>
+ <sourceDirectory>${project.basedir}/src/main/kotlin</sourceDirectory>
+ <plugins>
+ <plugin>
+ <groupId>kr.motd.maven</groupId>
+ <artifactId>os-maven-plugin</artifactId>
+ <version>${os-maven-plugin.version}</version>
+ <executions>
+ <execution>
+ <phase>initialize</phase>
+ <goals>
+ <goal>detect</goal>
+ </goals>
+ </execution>
+ </executions>
+ </plugin>
Review comment:
You don't need this plugin, instead, you need this, otherwise the packaged jar is not executable:
```xml
<plugin>
<groupId>org.springframework.boot</groupId>
<artifactId>spring-boot-maven-plugin</artifactId>
<version>${spring-boot-version}</version>
<executions>
<execution>
<goals>
<goal>repackage</goal>
</goals>
</execution>
</executions>
</plugin>
```
----------------------------------------------------------------
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] devkanro commented on issue #4165: Add kotlin
coroutine plugin
Posted by GitBox <gi...@apache.org>.
devkanro commented on issue #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#issuecomment-570760165
> @devkanro Considering that you're not able to test locally, I can help on test and fix the issues if you need my help :)
@kezhenxu94
Thanks, I really need your help
----------------------------------------------------------------
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 #4165:
Add kotlin coroutine plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#discussion_r362512880
##########
File path: apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/trace/SpanLayer.java
##########
@@ -26,7 +26,8 @@
RPC_FRAMEWORK(2),
HTTP(3),
MQ(4),
- CACHE(5);
+ CACHE(5),
+ COROUTINE(6);
Review comment:
This should not be added. `COROUTINE` is a local span. Should not be `Layer`.
----------------------------------------------------------------
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] devkanro commented on a change in pull request #4165:
Add kotlin coroutine plugin
Posted by GitBox <gi...@apache.org>.
devkanro commented on a change in pull request #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#discussion_r363026285
##########
File path: test/plugin/scenarios/kotlin-coroutine-scenario/support-version.list
##########
@@ -0,0 +1,16 @@
+# 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
+# "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.
+1.3.0
Review comment:
I am not sure about is lastest Kotlin version compatible with every Kotlin Coroutine 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] codecov-io edited a comment on issue #4165: Add kotlin
coroutine plugin
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#issuecomment-570608160
# [Codecov](https://codecov.io/gh/apache/skywalking/pull/4165?src=pr&el=h1) Report
> Merging [#4165](https://codecov.io/gh/apache/skywalking/pull/4165?src=pr&el=desc) into [master](https://codecov.io/gh/apache/skywalking/commit/6be84cfa784d47ce7b5e3a22dccd1d2c6fd1e105?src=pr&el=desc) will **decrease** coverage by `0.02%`.
> The diff coverage is `0%`.
[](https://codecov.io/gh/apache/skywalking/pull/4165?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #4165 +/- ##
==========================================
- Coverage 26.78% 26.76% -0.03%
==========================================
Files 1161 1163 +2
Lines 25410 25428 +18
Branches 3689 3691 +2
==========================================
- Hits 6807 6805 -2
- Misses 17997 18017 +20
Partials 606 606
```
| [Impacted Files](https://codecov.io/gh/apache/skywalking/pull/4165?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...plugin/kotlin/coroutine/DispatcherInterceptor.java](https://codecov.io/gh/apache/skywalking/pull/4165/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvb3B0aW9uYWwtcGx1Z2lucy9rb3RsaW4tY29yb3V0aW5lLXBsdWdpbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2t5d2Fsa2luZy9hcG0vcGx1Z2luL2tvdGxpbi9jb3JvdXRpbmUvRGlzcGF0Y2hlckludGVyY2VwdG9yLmphdmE=) | `0% <0%> (ø)` | |
| [.../apm/network/trace/component/ComponentsDefine.java](https://codecov.io/gh/apache/skywalking/pull/4165/diff?src=pr&el=tree#diff-YXBtLXByb3RvY29sL2FwbS1uZXR3b3JrL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9za3l3YWxraW5nL2FwbS9uZXR3b3JrL3RyYWNlL2NvbXBvbmVudC9Db21wb25lbnRzRGVmaW5lLmphdmE=) | `0% <0%> (ø)` | :arrow_up: |
| [...g/apm/plugin/kotlin/coroutine/TracingRunnable.java](https://codecov.io/gh/apache/skywalking/pull/4165/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvb3B0aW9uYWwtcGx1Z2lucy9rb3RsaW4tY29yb3V0aW5lLXBsdWdpbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2t5d2Fsa2luZy9hcG0vcGx1Z2luL2tvdGxpbi9jb3JvdXRpbmUvVHJhY2luZ1J1bm5hYmxlLmphdmE=) | `0% <0%> (ø)` | |
| [...ache/skywalking/apm/agent/core/jvm/JVMService.java](https://codecov.io/gh/apache/skywalking/pull/4165/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvanZtL0pWTVNlcnZpY2UuamF2YQ==) | `73.77% <0%> (-3.28%)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/skywalking/pull/4165?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/4165?src=pr&el=footer). Last update [6be84cf...5abf643](https://codecov.io/gh/apache/skywalking/pull/4165?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] devkanro edited a comment on issue #4165: Add kotlin
coroutine plugin
Posted by GitBox <gi...@apache.org>.
devkanro edited a comment on issue #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#issuecomment-570762748
> One question to all, does this plugin suppose to optional or default. How wide does routine use in kotlin? Could it have side effect as default for all coroutine?
@wu-sheng
I think this can be divided into four cases.
### Pure Java
Pure java will not exist the interception point, the plugin will not be activated, maybe no additional cost with this case, just 1 classes be loaded.
### Kotlin but no coroutine
Kotlin is becoming popular in the back-end stack, because of Kotlin's practical and straightforward grammar and powerful stdlib. But most people maybe use Kotlin to instead of Java, but no more coroutines or other adv features.
But if you don't have dependent on `org.jetbrains.kotlinx:kotlinx-coroutines-core`, the interception point will not exist too.
### Kotlin with coroutine in business code
Maybe sometimes, people will use coroutine in their business code, and don't care about the tracing in the coroutine.
It will cause some performance loss, about create two context snapshots cost.
### Kotliun with coroutine stack
This is my use case. I'm using a full coroutine stack framework, and it is necessary to make the links traceable.
It will cost about create two context snapshots, a span, a `TracingRunnable` object.
----------------------------------------------------------------
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 #4165: Add kotlin
coroutine plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#issuecomment-570575756
CI passed. Then you need the plugin test case.
----------------------------------------------------------------
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
#4165: Add kotlin coroutine plugin
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on a change in pull request #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#discussion_r362520279
##########
File path: apm-sniffer/bootstrap-plugins/kotlin-coroutine-plugin/pom.xml
##########
@@ -0,0 +1,78 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ ~ 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.
+ ~
+ -->
+
+<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+ <modelVersion>4.0.0</modelVersion>
+ <parent>
+ <groupId>org.apache.skywalking</groupId>
+ <artifactId>bootstrap-plugins</artifactId>
+ <version>7.0.0-SNAPSHOT</version>
+ </parent>
+
+ <artifactId>apm-kotlin-coroutine-plugin</artifactId>
+ <name>kotlin-coroutine-plugin</name>
+ <packaging>jar</packaging>
+
+ <properties>
+ <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
+ <kotlin.version>1.3.61</kotlin.version>
+ <kotlinx.coroutine.version>1.3.3</kotlinx.coroutine.version>
+ <kotlin.compiler.incremental>true</kotlin.compiler.incremental>
+ </properties>
+
+ <dependencies>
+ <dependency>
+ <groupId>org.jetbrains.kotlin</groupId>
+ <artifactId>kotlin-stdlib</artifactId>
Review comment:
I meant this, although the name is “stdlib”, but for Java it is not really standard, all the classes/functions in it will cause ClassNotFoundException when users are using vanilla Java
----------------------------------------------------------------
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] devkanro commented on issue #4165: Add kotlin
coroutine plugin
Posted by GitBox <gi...@apache.org>.
devkanro commented on issue #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#issuecomment-570760220
Paste errors and results here, I will try to resolve it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
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
#4165: Add kotlin coroutine plugin
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on a change in pull request #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#discussion_r363018367
##########
File path: test/plugin/scenarios/kotlin-coroutine-scenario/configuration.yml
##########
@@ -0,0 +1,22 @@
+# 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.
+
+type: jvm
+entryService: http://localhost:8080/kotlin-coroutine-scenario/case/h2
+healthCheck: http://localhost:8080/kotlin-coroutine-scenario/case/healthCheck
+framework: kt-coroutine
+runningMode: with_optional
+withPlugins: apm-kotlin-coroutine-plugin-*.jar
Review comment:
missed `startScript: ./bin/startup.sh`
----------------------------------------------------------------
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 #4165:
Add kotlin coroutine plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#discussion_r363024368
##########
File path: docs/en/setup/service-agent/java-agent/agent-optional-plugins/Kotlin-Coroutine-plugin.md
##########
@@ -0,0 +1,30 @@
+# Skywalking with Kotlin coroutine
+This PR provided an auto instrument support plugin for Kotlin coroutine based on context snapshot.
+
+## Description
+We have known there are some limits with skywalking and coroutine, because of the trace context based on `ThreadLocal`.
+
+But skywalking provided context snapshot for cross-thread tracing, I create this plugin for resolving context losing in Kotlin coroutine.
+
+## Implementation principle
+As we know, Kotlin coroutine switches the execution thread by `CoroutineDispatcher`.
+
+01. Create a snapshot of the current context before dispatch the continuation.
+02. Then create a coroutine span after thread switched, mark the span continued with the snapshot.
+03. Every new span which created in the new thread will be a child of this coroutine span. So we can link those span together in a tracing.
+04. After the original runnable executed, we need to stop the coroutine span for cleaning thread state.
+
+## Some screenshots
+### Run without the plugin
+We run a Kotlin coroutine based gRPC server without this coroutine plugin.
+You can find, the one call (client -> server1 -> server2) has been split two tracing paths.
+
+01. Server1 without exit span and server2 tracing path.
+
+02. Server2 tracing path.
+
Review comment:
If you want to provide screenshots, you need to host the images in Apache repo. Should create a folder named `7.0.0/kotlin/coroutine` at here https://github.com/apache/skywalking-website/tree/master/docs/.vuepress/public/screenshots
----------------------------------------------------------------
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 #4165:
Add kotlin coroutine plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#discussion_r363029761
##########
File path: docs/en/setup/service-agent/java-agent/agent-optional-plugins/Kotlin-Coroutine-plugin.md
##########
@@ -0,0 +1,30 @@
+# Skywalking with Kotlin coroutine
+This PR provided an auto instrument support plugin for Kotlin coroutine based on context snapshot.
+
+## Description
Review comment:
Your description is PR 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 issue #4165: Add kotlin
coroutine plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#issuecomment-570489156
You should run `./mvnw clean package` locally and passed first.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] dmsolr commented on a change in pull request #4165:
Add kotlin coroutine plugin
Posted by GitBox <gi...@apache.org>.
dmsolr commented on a change in pull request #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#discussion_r362721134
##########
File path: apm-sniffer/optional-plugins/kotlin-coroutine-plugin/src/main/java/org/apache/skywalking/apm/plugin/kotlin/coroutine/TracingRunnable.java
##########
@@ -0,0 +1,89 @@
+/*
+ * 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.plugin.kotlin.coroutine;
+
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import org.apache.skywalking.apm.agent.core.context.ContextSnapshot;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.network.trace.component.ComponentsDefine;
+
+/**
+ * {@link Runnable} wrapper with trace context snapshot, it will create span
+ * with context snapshot around {@link Runnable} runs.
+ * <p>
+ * A class implementation will be cheaper cost than lambda with captured
+ * variables implementation.
+ */
+class TracingRunnable implements Runnable {
+ private static final String COROUTINE = "/Kotlin/Coroutine";
+
+ private ContextSnapshot snapshot;
+ private Runnable delegate;
+ private String tracingId;
+
+ private TracingRunnable(ContextSnapshot snapshot, Runnable delegate, String tracingId) {
+ this.snapshot = snapshot;
+ this.delegate = delegate;
+ this.tracingId = tracingId;
+ }
+
+ private TracingRunnable(ContextSnapshot snapshot, Runnable delegate) {
+ this(snapshot, delegate, ContextManager.getGlobalTraceId());
+ }
+
+ /**
+ * Wrap {@link Runnable} by {@link TracingRunnable} if active trace context
+ * existed.
+ *
+ * @param delegate {@link Runnable} to wrap.
+ *
+ * @return Wrapped {@link TracingRunnable} or original {@link Runnable} if
+ * trace context not existed.
+ */
+ public static Runnable wrapOrNot(Runnable delegate) {
+ // Just wrap continuation with active trace context
+ if (ContextManager.isActive()) {
+ return new TracingRunnable(ContextManager.capture(), delegate);
+ } else {
+ return delegate;
+ }
+ }
+
+ @Override
+ public void run() {
+ if (ContextManager.getGlobalTraceId().equals(tracingId)) {
+ // Trace id same with before dispatching, skip restore snapshot.
+ delegate.run();
+ return;
+ }
Review comment:
Do we need it?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
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 #4165:
Add kotlin coroutine plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#discussion_r363025503
##########
File path: test/plugin/scenarios/kotlin-coroutine-scenario/support-version.list
##########
@@ -0,0 +1,16 @@
+# 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
+# "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.
+1.3.0
Review comment:
I think at least for the latest of every major release version, such as 1.3.3 for 1.3.x
----------------------------------------------------------------
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 #4165: Add kotlin
coroutine plugin
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on issue #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#issuecomment-570760082
@devkanro Considering that you're not able to test locally, I can help on test and fix the issues if you need my help :)
----------------------------------------------------------------
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] devkanro commented on issue #4165: Add kotlin
coroutine plugin
Posted by GitBox <gi...@apache.org>.
devkanro commented on issue #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#issuecomment-570595588
> This is a very simple test, you are testing `coroutine`, so basically, if a Tomcat serves HTTP, and call coroutine API to do a DB(H2) access. Then, you have your scenario, right?
You are right, I think too complicated.
----------------------------------------------------------------
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 #4165: Add kotlin
coroutine plugin
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on issue #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#issuecomment-570776642
> The four cases look good enough. @kezhenxu94 @arugal @dmsolr What do you think?
Looks good to me enough
> Do we move this into the default? Or stay in the option for safe.
I'd say let's make it optional, since coroutine is to Kotlin as what threading is to Java, IMO, and there should be many third party Kotlin libs are using it, so it should be more safe to make it optional
----------------------------------------------------------------
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 #4165: Add kotlin
coroutine plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#issuecomment-570250980
> Is here some strong reason for not accepting .kt?
The key question is, is this necessary to accept the kt files? I think this plugin could be written by Java too, unless there is some class can't be defined by Java, which is very rare.
----------------------------------------------------------------
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] devkanro edited a comment on issue #4165: Add kotlin
coroutine plugin
Posted by GitBox <gi...@apache.org>.
devkanro edited a comment on issue #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#issuecomment-570762748
> One question to all, does this plugin suppose to optional or default. How wide does routine use in kotlin? Could it have side effect as default for all coroutine?
@wu-sheng
I think this can be divided into four cases.
### Pure Java
Pure java will not exist the interception point, the plugin will not be activated, maybe no additional cost with this case, just 1 classes be loaded.
### Kotlin but no coroutine
Kotlin is becoming popular in the back-end stack, because of Kotlin's practical and straightforward grammar and powerful stdlib. But most people maybe use Kotlin to instead of Java, but no more coroutines or other adv features.
But if you don't have dependent on `org.jetbrains.kotlinx:kotlinx-coroutines-core`, the interception point will not exist too.
### Kotlin with coroutine in business code
Maybe sometimes, people will use coroutine in their business code, and don't care about the tracing in the coroutine.
If you have no activated tracing context, it just case `ContextManager.isActive()` cost.
If you have activated tracing context in the current thread, it will cause some performance loss, about create two context snapshots, a span, and a `TracingRunnable` object.
### Kotlin with coroutine stack
This is my use case. I'm using a full coroutine stack framework, and it is necessary to make the links traceable.
It will cost about create two context snapshots, a span, a `TracingRunnable` object.
----------------------------------------------------------------
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 #4165: Add kotlin
coroutine plugin
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#issuecomment-570608160
# [Codecov](https://codecov.io/gh/apache/skywalking/pull/4165?src=pr&el=h1) Report
> Merging [#4165](https://codecov.io/gh/apache/skywalking/pull/4165?src=pr&el=desc) into [master](https://codecov.io/gh/apache/skywalking/commit/99100ed635522fb5142230e7a7c03d8011c2f8a1?src=pr&el=desc) will **increase** coverage by `0.02%`.
> The diff coverage is `0%`.
[](https://codecov.io/gh/apache/skywalking/pull/4165?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #4165 +/- ##
==========================================
+ Coverage 26.78% 26.81% +0.02%
==========================================
Files 1161 1163 +2
Lines 25410 25428 +18
Branches 3689 3691 +2
==========================================
+ Hits 6807 6819 +12
- Misses 17997 18000 +3
- Partials 606 609 +3
```
| [Impacted Files](https://codecov.io/gh/apache/skywalking/pull/4165?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...plugin/kotlin/coroutine/DispatcherInterceptor.java](https://codecov.io/gh/apache/skywalking/pull/4165/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvb3B0aW9uYWwtcGx1Z2lucy9rb3RsaW4tY29yb3V0aW5lLXBsdWdpbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2t5d2Fsa2luZy9hcG0vcGx1Z2luL2tvdGxpbi9jb3JvdXRpbmUvRGlzcGF0Y2hlckludGVyY2VwdG9yLmphdmE=) | `0% <0%> (ø)` | |
| [.../apm/network/trace/component/ComponentsDefine.java](https://codecov.io/gh/apache/skywalking/pull/4165/diff?src=pr&el=tree#diff-YXBtLXByb3RvY29sL2FwbS1uZXR3b3JrL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9za3l3YWxraW5nL2FwbS9uZXR3b3JrL3RyYWNlL2NvbXBvbmVudC9Db21wb25lbnRzRGVmaW5lLmphdmE=) | `0% <0%> (ø)` | :arrow_up: |
| [...g/apm/plugin/kotlin/coroutine/TracingRunnable.java](https://codecov.io/gh/apache/skywalking/pull/4165/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvb3B0aW9uYWwtcGx1Z2lucy9rb3RsaW4tY29yb3V0aW5lLXBsdWdpbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2t5d2Fsa2luZy9hcG0vcGx1Z2luL2tvdGxpbi9jb3JvdXRpbmUvVHJhY2luZ1J1bm5hYmxlLmphdmE=) | `0% <0%> (ø)` | |
| [.../core/remote/ServiceAndEndpointRegisterClient.java](https://codecov.io/gh/apache/skywalking/pull/4165/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvcmVtb3RlL1NlcnZpY2VBbmRFbmRwb2ludFJlZ2lzdGVyQ2xpZW50LmphdmE=) | `44.94% <0%> (+13.48%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/skywalking/pull/4165?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/4165?src=pr&el=footer). Last update [99100ed...8d0e4ef](https://codecov.io/gh/apache/skywalking/pull/4165?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 merged pull request #4165: Add kotlin
coroutine plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng merged pull request #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165
----------------------------------------------------------------
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 #4165: Add kotlin
coroutine plugin
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#issuecomment-570608160
# [Codecov](https://codecov.io/gh/apache/skywalking/pull/4165?src=pr&el=h1) Report
> Merging [#4165](https://codecov.io/gh/apache/skywalking/pull/4165?src=pr&el=desc) into [master](https://codecov.io/gh/apache/skywalking/commit/99100ed635522fb5142230e7a7c03d8011c2f8a1?src=pr&el=desc) will **increase** coverage by `0.02%`.
> The diff coverage is `0%`.
[](https://codecov.io/gh/apache/skywalking/pull/4165?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #4165 +/- ##
==========================================
+ Coverage 26.78% 26.81% +0.02%
==========================================
Files 1161 1163 +2
Lines 25410 25428 +18
Branches 3689 3691 +2
==========================================
+ Hits 6807 6819 +12
- Misses 17997 18000 +3
- Partials 606 609 +3
```
| [Impacted Files](https://codecov.io/gh/apache/skywalking/pull/4165?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...plugin/kotlin/coroutine/DispatcherInterceptor.java](https://codecov.io/gh/apache/skywalking/pull/4165/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvb3B0aW9uYWwtcGx1Z2lucy9rb3RsaW4tY29yb3V0aW5lLXBsdWdpbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2t5d2Fsa2luZy9hcG0vcGx1Z2luL2tvdGxpbi9jb3JvdXRpbmUvRGlzcGF0Y2hlckludGVyY2VwdG9yLmphdmE=) | `0% <0%> (ø)` | |
| [.../apm/network/trace/component/ComponentsDefine.java](https://codecov.io/gh/apache/skywalking/pull/4165/diff?src=pr&el=tree#diff-YXBtLXByb3RvY29sL2FwbS1uZXR3b3JrL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9za3l3YWxraW5nL2FwbS9uZXR3b3JrL3RyYWNlL2NvbXBvbmVudC9Db21wb25lbnRzRGVmaW5lLmphdmE=) | `0% <0%> (ø)` | :arrow_up: |
| [...g/apm/plugin/kotlin/coroutine/TracingRunnable.java](https://codecov.io/gh/apache/skywalking/pull/4165/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvb3B0aW9uYWwtcGx1Z2lucy9rb3RsaW4tY29yb3V0aW5lLXBsdWdpbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2t5d2Fsa2luZy9hcG0vcGx1Z2luL2tvdGxpbi9jb3JvdXRpbmUvVHJhY2luZ1J1bm5hYmxlLmphdmE=) | `0% <0%> (ø)` | |
| [.../core/remote/ServiceAndEndpointRegisterClient.java](https://codecov.io/gh/apache/skywalking/pull/4165/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvcmVtb3RlL1NlcnZpY2VBbmRFbmRwb2ludFJlZ2lzdGVyQ2xpZW50LmphdmE=) | `44.94% <0%> (+13.48%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/skywalking/pull/4165?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/4165?src=pr&el=footer). Last update [99100ed...b891f1f](https://codecov.io/gh/apache/skywalking/pull/4165?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
#4165: Add kotlin coroutine plugin
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on a change in pull request #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#discussion_r362520279
##########
File path: apm-sniffer/bootstrap-plugins/kotlin-coroutine-plugin/pom.xml
##########
@@ -0,0 +1,78 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ ~ 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.
+ ~
+ -->
+
+<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+ <modelVersion>4.0.0</modelVersion>
+ <parent>
+ <groupId>org.apache.skywalking</groupId>
+ <artifactId>bootstrap-plugins</artifactId>
+ <version>7.0.0-SNAPSHOT</version>
+ </parent>
+
+ <artifactId>apm-kotlin-coroutine-plugin</artifactId>
+ <name>kotlin-coroutine-plugin</name>
+ <packaging>jar</packaging>
+
+ <properties>
+ <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
+ <kotlin.version>1.3.61</kotlin.version>
+ <kotlinx.coroutine.version>1.3.3</kotlinx.coroutine.version>
+ <kotlin.compiler.incremental>true</kotlin.compiler.incremental>
+ </properties>
+
+ <dependencies>
+ <dependency>
+ <groupId>org.jetbrains.kotlin</groupId>
+ <artifactId>kotlin-stdlib</artifactId>
Review comment:
I meant this, although the name is “stdlib”, but for Java it is not really standard, all the classes/functions in it will course ClassNotFoundException when users are using vanilla Java
----------------------------------------------------------------
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] devkanro commented on issue #4165: Add kotlin
coroutine plugin
Posted by GitBox <gi...@apache.org>.
devkanro commented on issue #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#issuecomment-570766742
@wu-sheng @kezhenxu94
It seems like all CI passed
----------------------------------------------------------------
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] devkanro commented on a change in pull request #4165:
Add kotlin coroutine plugin
Posted by GitBox <gi...@apache.org>.
devkanro commented on a change in pull request #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#discussion_r362520309
##########
File path: apm-sniffer/bootstrap-plugins/pom.xml
##########
@@ -43,6 +43,7 @@
<modules>
<module>jdk-http-plugin</module>
<module>jdk-threading-plugin</module>
+ <module>kotlin-coroutine-plugin</module>
Review comment:
I have another implementation for this plugin, it based on `ThreadContextElement` which is Kotlin coroutine context for syncing thread context, it needs to create interceptor point in `newCoroutineContext`, I got some bugs with creating the key object of `ThreadContextElement`(singleton be created twice), I change this plugin into bootstrap plugin, It works fine, I will do more test for it, and I will change it to a normal plugin after those test.
----------------------------------------------------------------
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 #4165:
Add kotlin coroutine plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#discussion_r363024398
##########
File path: docs/en/setup/service-agent/java-agent/agent-optional-plugins/Kotlin-Coroutine-plugin.md
##########
@@ -0,0 +1,30 @@
+# Skywalking with Kotlin coroutine
Review comment:
As an optional plugin, this doc should be linked from https://github.com/apache/skywalking/blob/master/docs/en/setup/service-agent/java-agent/README.md#optional-plugins
----------------------------------------------------------------
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 #4165: Add kotlin
coroutine plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#issuecomment-570758723
Just a brief. What you want to change seems right. Please go ahead.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] wu-sheng edited a comment on issue #4165: Add kotlin
coroutine plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng edited a comment on issue #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#issuecomment-570769206
The four cases look good enough. @kezhenxu94 @arugal @dmsolr What do you think?
Do we move this into the default? Or stay in the option for safe.
----------------------------------------------------------------
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 #4165: Add kotlin
coroutine plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#issuecomment-570251484
You don't need a Koltin plugin, the core concept of Java plugin is to intercept the target classes. So, everything you did should write in Java easily.
----------------------------------------------------------------
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 #4165: Add kotlin
coroutine plugin
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#issuecomment-570608160
# [Codecov](https://codecov.io/gh/apache/skywalking/pull/4165?src=pr&el=h1) Report
> Merging [#4165](https://codecov.io/gh/apache/skywalking/pull/4165?src=pr&el=desc) into [master](https://codecov.io/gh/apache/skywalking/commit/c237385d33d116864fd9770cabc5cd3052e5e778?src=pr&el=desc) will **decrease** coverage by `0.02%`.
> The diff coverage is `6.45%`.
[](https://codecov.io/gh/apache/skywalking/pull/4165?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #4165 +/- ##
==========================================
- Coverage 26.83% 26.81% -0.03%
==========================================
Files 1163 1163
Lines 25429 25428 -1
Branches 3691 3691
==========================================
- Hits 6825 6819 -6
- Misses 17994 18000 +6
+ Partials 610 609 -1
```
| [Impacted Files](https://codecov.io/gh/apache/skywalking/pull/4165?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...g/apm/plugin/kotlin/coroutine/TracingRunnable.java](https://codecov.io/gh/apache/skywalking/pull/4165/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvb3B0aW9uYWwtcGx1Z2lucy9rb3RsaW4tY29yb3V0aW5lLXBsdWdpbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2t5d2Fsa2luZy9hcG0vcGx1Z2luL2tvdGxpbi9jb3JvdXRpbmUvVHJhY2luZ1J1bm5hYmxlLmphdmE=) | `0% <0%> (ø)` | :arrow_up: |
| [...commons/interceptor/AbstractMethodInterceptor.java](https://codecov.io/gh/apache/skywalking/pull/4165/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLXNkay1wbHVnaW4vc3ByaW5nLXBsdWdpbnMvbXZjLWFubm90YXRpb24tY29tbW9ucy9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2t5d2Fsa2luZy9hcG0vcGx1Z2luL3NwcmluZy9tdmMvY29tbW9ucy9pbnRlcmNlcHRvci9BYnN0cmFjdE1ldGhvZEludGVyY2VwdG9yLmphdmE=) | `0% <0%> (ø)` | :arrow_up: |
| [.../apache/skywalking/apm/agent/core/conf/Config.java](https://codecov.io/gh/apache/skywalking/pull/4165/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29uZi9Db25maWcuamF2YQ==) | `66.66% <0%> (-1.34%)` | :arrow_down: |
| [...skywalking/apm/agent/core/util/CollectionUtil.java](https://codecov.io/gh/apache/skywalking/pull/4165/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvdXRpbC9Db2xsZWN0aW9uVXRpbC5qYXZh) | `0% <0%> (ø)` | :arrow_up: |
| [...he/skywalking/apm/agent/core/context/tag/Tags.java](https://codecov.io/gh/apache/skywalking/pull/4165/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29udGV4dC90YWcvVGFncy5qYXZh) | `100% <100%> (ø)` | :arrow_up: |
| [.../apm/plugin/tomcat78x/TomcatInvokeInterceptor.java](https://codecov.io/gh/apache/skywalking/pull/4165/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLXNkay1wbHVnaW4vdG9tY2F0LTcueC04LngtcGx1Z2luL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9za3l3YWxraW5nL2FwbS9wbHVnaW4vdG9tY2F0Nzh4L1RvbWNhdEludm9rZUludGVyY2VwdG9yLmphdmE=) | `58.97% <7.14%> (ø)` | :arrow_up: |
| [...apm/agent/core/remote/GRPCStreamServiceStatus.java](https://codecov.io/gh/apache/skywalking/pull/4165/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvcmVtb3RlL0dSUENTdHJlYW1TZXJ2aWNlU3RhdHVzLmphdmE=) | `29.16% <0%> (-25.01%)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/skywalking/pull/4165?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/4165?src=pr&el=footer). Last update [c237385...415436d](https://codecov.io/gh/apache/skywalking/pull/4165?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] devkanro commented on a change in pull request #4165:
Add kotlin coroutine plugin
Posted by GitBox <gi...@apache.org>.
devkanro commented on a change in pull request #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#discussion_r362522290
##########
File path: apm-sniffer/bootstrap-plugins/kotlin-coroutine-plugin/pom.xml
##########
@@ -0,0 +1,78 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ ~ 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.
+ ~
+ -->
+
+<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+ <modelVersion>4.0.0</modelVersion>
+ <parent>
+ <groupId>org.apache.skywalking</groupId>
+ <artifactId>bootstrap-plugins</artifactId>
Review comment:
@wu-sheng has commented it too, I have reply to him, check it.
>I have another implementation for this plugin, it based on `ThreadContextElement` which is Kotlin coroutine context for syncing thread context, it needs to create interceptor point in `newCoroutineContext`, but I got some bugs with creating the key object of `ThreadContextElement`(singleton be created twice), I change this plugin into bootstrap plugin, It works fine, I will do more test for it, and I will change it to a normal plugin after those test.
----------------------------------------------------------------
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] devkanro commented on issue #4165: Add kotlin
coroutine plugin
Posted by GitBox <gi...@apache.org>.
devkanro commented on issue #4165: Add kotlin coroutine plugin
URL: https://github.com/apache/skywalking/pull/4165#issuecomment-570264387
@wu-sheng @kezhenxu94
Rewritten it in Java, check it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services