Skip to content
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

Merged
merged 2 commits into from
Nov 24, 2020

Conversation

anuraaga
Copy link
Contributor

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

Segment segment = beginSegment();
...
public void methodMakingBackendCall() {
  segment.run(() -> {
    var response = dynamoDbClient.execute(request);
  }
}

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.

* @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
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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

willarmiros
willarmiros previously approved these changes Nov 24, 2020
@willarmiros willarmiros dismissed their stale review November 24, 2020 16:58

Pending review from other teammates

Copy link
Contributor

@willarmiros willarmiros left a 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

@willarmiros willarmiros merged commit ad6b333 into aws:master Nov 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants