-
Notifications
You must be signed in to change notification settings - Fork 98
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
Add a method to run a block of code with a segment mounted as the cur… #240
Conversation
* @deprecated Use {@link Entity#run(Runnable)} or methods in {@link SegmentContextExecutors} instead of directly setting | ||
* the trace entity so it can be restored correctly. | ||
*/ | ||
@Deprecated |
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.
Is deprecating this the path forward we're certain we want to take? It is certainly a sharp edge and I definitely agree with recommending using Entity.run()
over this. But IMO customers who know their own architecture should still be able to use these low-level APIs if it suits their app and they're comfortable with the risks. Especially if they've implemented a custom SegmentContext
interface that overrides the default setTraceEntity()
behavior.
All this being said, if the team disagrees and we move forward with this path, should we go ahead and also deprecate clearTraceEntity
and getTraceEntity
?
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.
get
is required since you definitely need to be able to get the current entity. It's also safe.
clear
I debated - but even in OTel it's important in request handlers as a safeguard against leaks. For maximum safety we always clear before creating a root span (segment) since parenting never makes sense. So I think it's ok to keep - it's basically safe (can lead to broken traces potentially/rarely, but definitely not leaks).
set
is bad since it requires the restoration maneuvering - it's never better than a Closeable
or wrapping a lambda, just more tedious. We haven't added a closeable in this PR yet but I'm still comfortable removing set
, it can never provide a good experience I think.
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.
Ok, thanks for the explainers. Here's to safer usage going forward :)
Pending review from other teammates
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.
LGTM & approved by team
…rent entity.
Description of changes: To reduce the machinery needed to manage threadlocal context for users when propagating a segment using manual code, this adds
Segment.run
to take care of it in the background.SegmentContextExecutors.newSegmentContextExecutor
and registering the resulting executor in an async framework should still be the preferred pattern for propagating segments.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.