otelhttp: get tracer from current context if not set in constructor #873
Conversation
tonistiigi
requested review from
Aneurysm9,
dashpole,
evantorrie,
jmacd,
MadVikingGod,
MrAlias,
paivagustavo,
pellared and
XSAM
as code owners
There was a problem hiding this comment.
I am not sure if there was any issue or discussion before this PR was created, please link it if there is some. I imagine that if we would like to support such behavior then we should probably create some helper in SDK and then use it in all applicable instrumentation packages (you already repeated the functionality in #874). This would deserve an issue at least for tracking purposes.
| instrumentationName, | ||
| trace.WithInstrumentationVersion(contrib.SemVersion()), | ||
| ) | ||
| // Tracer is only initialized if manually specified. Otherwise, can be passed with the tracing context. |
There was a problem hiding this comment.
a comment in code like this is not enough for documenting the behavior
it should be at least noted in "entry" point functions/types e.g. NewHandler and NewTransport
Moreover the new feature should be noted in CHANGELOG.md
There was a problem hiding this comment.
This is not documenting behavior but explaining why the .Meter and .Tracer are handled differently to the reader. This switch could be in RoundTrip() as well.
There was a problem hiding this comment.
Added changelog item to the "Added" section. Could have been in "Fixed" as well as if the global tracer and the parent span tracer would not have matched previously I think it would have lead to some bad behavior. Lmk if you want to change that.
There was a problem hiding this comment.
particularly while the question of even getting a
TracerProviderfrom aSpanis still outstanding in the spec.
For sure it should not be in "Fixed".
If we accept such behavior as acceptable, then, in my opinion, it should be explicitly described in the godoc.
tonistiigi
force-pushed
the
otelhttp-context
branch
2 times, most recently
from
3a7013e
to
e00caaa
Compare
Codecov Report
@@ Coverage Diff @@
## main #873 +/- ##
=====================================
Coverage 69.2% 69.3%
=====================================
Files 126 127 +1
Lines 5480 5496 +16
=====================================
+ Hits 3797 3810 +13
- Misses 1536 1538 +2
- Partials 147 148 +1
|
tonistiigi
force-pushed
the
otelhttp-context
branch
from
e00caaa
to
f1f7321
Compare
|
#879 was merged, so this needs another rebase @tonistiigi |
tonistiigi
force-pushed
the
otelhttp-context
branch
from
f1f7321
to
eced414
Compare
|
The base of this PR was completely refactored while it was stuck in review 😞 . The refactoring was merged in a matter of hours. |
|
Looks like this needs a re-approval to get CI running; @pellared @Aneurysm9 are you able to help getting this one moving? |
|
@tonistiigi needs a rebase (again) 😞 |
tonistiigi
force-pushed
the
otelhttp-context
branch
from
eced414
to
5e863f0
Compare
|
Can someone approve CI run please? Thanks! |
|
Looks to be green ✅ |
|
And needs a rebase again (conflict in CHANGELOG.md) 😞 @XSAM @pellared @Aneurysm9 are you able to help getting this one moving? Trying to get rid of a (what was expected to be a short-lived) fork of this code. |
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
tonistiigi
force-pushed
the
otelhttp-context
branch
from
5e863f0
to
9710f2d
Compare
When
otelhttpis used as aTransportthe traces should go to the parent span tracer by default. If customTracerProvideris set then it is still used and if none is set then global one is used like it was before.@Aneurysm9
Signed-off-by: Tonis Tiigi tonistiigi@gmail.com