[fix] Open a new span when wrapping with a new trace #49
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
carterkozak
reviewed
Dec 21, 2018
| return delegate.call(); | ||
| } finally { | ||
| Tracer.completeSpan(); |
carterkozak
reviewed
Dec 21, 2018
| delegate.run(); | ||
| } finally { | ||
| Tracer.completeSpan(); |
carterkozak
reviewed
Dec 21, 2018
| @@ -148,8 +149,10 @@ public static ScheduledExecutorService wrapWithNewTrace(ScheduledExecutorService | |||
|
|
|||
There was a problem hiding this comment.
Looks like there's an existing bug where wrapping a callable will leave an empty trace with no root span on the thread if there's not already an active trace.
Tracer.getAndClearTrace should probably return Optional<Trace>
Not related to this change though.
API rev isn't required for this change, but I don't think it solves the linked ticket. This definitely fixes a bug (thanks!), but it's still possible to invoke |
carterkozak
approved these changes
Dec 21, 2018
pkoenig10
changed the title
bulldozer-bot bot
pushed a commit
that referenced
this pull request
Feb 6, 2019
…new trace (#59) ## Before this PR When calling any of the `wrapWithNewTrace` methods, the operation of the initial span would be set to "root". This makes it difficult to determine the origin of traces if `wrapWithNewTrace` is used in more than one place. ## After this PR `Tracers` now provides variants of the `wrapWithNewTrace` methods that allow callers to explicitly set the initial span operation. Follow up to #49.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Before this PR
Creating a new trace does not create a new root span. Thus all top-level calls in this trace will have no parent span. This breaks assumptions of trace log consumers who assume that there is only one root span for a given trace.
This also seems counter to the
parentIdspecification in the Zipkin API:After this PR
Creating a new trace will create a new root span.
The issue is part of the
Major API revisionmilestone, but this fix does not break the API. Is a major API revision required for this change?