-
Notifications
You must be signed in to change notification settings - Fork 118
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Kotlin extension - coroutines example #188
Kotlin extension - coroutines example #188
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it. Seems like a clear example to me. Just offered a few suggestions.
val finishedSpanItems = inMemoryExporter.finishedSpanItems | ||
println("First child's trace id: ${finishedSpanItems[1].traceId}") // The same as the root span's. | ||
println("Second child's trace id: ${finishedSpanItems[2].traceId}") // The same as the root span's. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You would have a restructure a little bit, but what about moving this to the main method after the latch is awaited? That feels a bit more "finalized" to me and also more clearly shows that this is the demonstrated example output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also add the thread.id
as an attribute to the spans and print it here? I guess there's no simple way to force the execution to be on a different thread, but it would be a nice visual verification that it works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I've made some changes to it to clean it up a bit and to move the verification part outside of the coroutine block. I also like the idea of printing the thread.id
, so that's added now as well.
// Bonus: If we wanted to launch a coroutine in a separate thread (not IO, which is the one set in the scope) while | ||
// keeping the Span context too, we could do so like this: | ||
// scope.launch(Dispatchers.Main + Context.current().asContextElement()) | ||
scope.launch(Context.current().asContextElement()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a kotlin dev so sorry if I have misunderstood it, but couldn't you just use the Job
returned from scope.launch
and call join
on it to wait for the coroutine to complete? Then you could move everything that comes after createChildrenSpans
out of the coroutine. Closing a scope on a thread different from the one that opened it doesn't really work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a great point, thanks. I wanted to make it easy to read even for non kotlin devs, though it certainly looked a bit strange so I just applied your suggestion and it looks cleaner now.
…wing the same format for other modules
…ify that they were created in different thread and still got the same trace id
/easycla |
This is an example of how to use the
Context.asContextElement
extension function provided in the OTel Java Kotlin extension library in order to preserve a Span context across a coroutine, even if its thread changes.