-
Notifications
You must be signed in to change notification settings - Fork 20
Support attaching DetachedSpan state to a thread without creating children #769
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
Conversation
Generate changelog in
|
@smoorpal This is the piece that should enable the capability you were looking for in dialogue -- this way we can decouple the concerns around retaining trace state from span creation. |
* completed separately. | ||
*/ | ||
@MustBeClosed | ||
CloseableSpan attach(); |
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.
The CloseableSpan
type is a little bit odd here because it's not actually creating a span. I played with a new (though functionally identical) CloseableContext type to make the naming cleaner, but it also made the api larger, so I've pushed reusing the existing type.
assertThat(observed.get(0)).extracting(Span::getOperation).isEqualTo(operationName + " initial"); | ||
assertThat(observed.get(1)).extracting(Span::getOperation).isEqualTo(operationName); | ||
assertThat(observed).hasSize(1); | ||
assertThat(observed.get(0)).extracting(Span::getOperation).isEqualTo(operationName); |
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.
Everyone using the Tracers.wrapListenableFuture
functionality will get cleaner tracing spans (no more "initial" span) when they upgrade.
==COMMIT_MSG==
Support attaching DetachedSpan state to a thread without creating children
==COMMIT_MSG==
Potential Downsides
Previously we tried to couple thread state transitions/associations directly with named spans to more easily discover and remediate tracing state leak. Using
@MustBeClosed
should make it harder to create asymmetric start/complete tracing structures, and we're clearly pushing more spans than we'd like. I'd rather relax our opinions in this project than further reduce tracing rate while pushing unhelpful data.