tracing: clean up receivers #56045
Merged
tracing: clean up receivers #56045
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
Release note: None
When `Span` and `Tracer` were exposed only via the `opentracing` interfaces, we weren't able to freely add methods to them, which led to a number of methods being added to the tracing package that took an `opentracing.Span` or `opentracing.Tracer` and internally unwrapped the underlying type. Now that we're not using these interfaces any more, we can be more idiomatic and put all methods on their proper receivers. This is done on this commit as a prelude to being more opinionated about our APIs. Release note: None
This is one of the immediate useful outcomes of moving off the `opentracing.Span` interface: we now get to decide what `(nil).Finish()` does, removing the need for workarounds. Release note: None
It was a testing crutch that is no longer necessary now that we've moved off the opentracing interfaces. Release note: None
tbg
requested review from
pbardea,
knz and
irfansharif
and removed request for
a team
|
This change is |
tbg
force-pushed
the
tracing-more-cleanup
branch
from
b00e688 to
a0dc6a5
Compare
RaduBerinde
approved these changes
Oct 29, 2020
There was a problem hiding this comment.
Very nice, it's great that we can do this now!
Reviewable status:
complete! 1 of 0 LGTMs obtained (waiting on @irfansharif, @knz, and @tbg)
pkg/util/tracing/span.go, line 148 at r5 (raw file):
} // otSpan is an span for an external opentracing compatible tracer
[nit] a span
pkg/util/tracing/span.go, line 153 at r5 (raw file):
// shadowTr is the shadowTracer this span was created from. We need // to hold on to it separately because shadowSpan.Tracer() returns // the wrapper tracer directly and we lose the ability to find out
[nit] maybe remove "directly", I found it confusing
Release note: None
tbg
force-pushed
the
tracing-more-cleanup
branch
from
a0dc6a5 to
b25946e
Compare
tbg
commented
Oct 29, 2020
There was a problem hiding this comment.
TFTR!
bors r=RaduBerinde
Reviewable status:
|
Build succeeded: |
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.
Assorted cleanups that are possible now, that we have moved off the opentracing
interfaces.
There will be more cleanups.