Don't default the tracer name. #3153
Conversation
And, make sure that the standard exporters can handle a null InstrumentationLibraryInfo name.
jkwatson
requested review from
anuraaga,
arminru,
bogdandrutu,
carlosalberto,
Oberon00,
pavolloffay,
thisthat and
tylerbenson
as code owners
| @@ -48,6 +45,7 @@ public static InstrumentationLibraryInfo empty() { | |||
| * | |||
| * @return the name of the instrumentation library. | |||
| */ | |||
| @Nullable | |||
There was a problem hiding this comment.
Hmm - going from non-null to Nullable isn't technically a safe change, as can be seen with the amount of null checks we had to add everywhere (any custom exporter out there would have the same problem on a minor version bump). Since the spec says SHOULD preserve original value, but we know preserving null in Java is generally a bad idea, can we just skip this change?
There was a problem hiding this comment.
It's a fine question. @bogdandrutu was the one who pushed for this in the specs. Opinions?
Honestly, I also think this is a bad change. I would much prefer to have a fallback name, so that exporters don't all have to guard against this case.
Can we maybe use empty string as the default, rather than keeping nulls?
There was a problem hiding this comment.
Empty string is still a better default than keeping nulls indeed.
There was a problem hiding this comment.
I'd definitely vote against setting this nullable as the value will propagate, causing lots of extra null-checks (as documented in this PR).
There was a problem hiding this comment.
Please add your opinions to open-telemetry/opentelemetry-specification#1619 :)
|
Turned this into a draft, pending the outcome of the related spec issue. |
|
closing, as we're going with #3170 |
And, make sure that the standard exporters can handle a null InstrumentationLibraryInfo name.
The spec changed in v1.2.0 to say we should no longer have a default: open-telemetry/opentelemetry-specification#1534