Populate InstrumentationLibrary to exporters #700
Conversation
Codecov Report
@@ Coverage Diff @@
## main #700 +/- ##
=======================================
Coverage 94.80% 94.81%
=======================================
Files 214 214
Lines 9744 9750 +6
=======================================
+ Hits 9238 9244 +6
Misses 506 506
|
exporters/zipkin/src/recordable.cc
Outdated
| &&instrumentation_library) noexcept | ||
| { | ||
| span_["otel.library.name"] = instrumentation_library->GetName(); | ||
| span_["otel.library.version"] = instrumentation_library->GetVersion(); |
There was a problem hiding this comment.
As per the specs these should be reported in tags:
OpenTelemetry Span's InstrumentationLibrary MUST be reported as tags to Zipkin using the following mapping.
Similar to what is done for status:
There was a problem hiding this comment.
Thanks for the catch. I misread the tags part in the spec. Fixed it.
| * Set the instrumentation library of the span. | ||
| * @param instrumentation_library the instrumentation library to set | ||
| */ | ||
| virtual void SetInstrumentationLibrary( |
There was a problem hiding this comment.
Thinking out loud - should we be instead passing nostd::string_view for name and version - to ensure that exporter(s) won't store this shared_ptr reference locally?
There was a problem hiding this comment.
Yes, this is a good thought. I was trying to avoid dispersing the internal structure of InstruemntationLibrary to places other than the class itself as the spec mentioned it could be expected in future.
I tried define the pointer as std::wake_ptr in the exporter to avoid taking ownership. I could make this change if it looks good to you.
There was a problem hiding this comment.
If we want to keep the structure intact, it would be much easier to pass const reference to InstruemntationLibrary, as Tracer::GetInstrumentationLibrary() already returns that.
There was a problem hiding this comment.
Take const reference could cause use after free if the tracer provider is destructed before the exporter, or the exporter needs to make a copy of the InstrumentationLibrary which I'd like to avoid.
There was a problem hiding this comment.
I don't think that would happen. As the shutdown is propagated from TracerProvider -> Processor -> Exporter.
There was a problem hiding this comment.
This looks reasonable. Could there be case like the exporter runs asynchronously then it is still working after the call of TracerProvider::Shutdown?
Updated the PR to pass const reference of InstrumentationLibrary to exporters.
There was a problem hiding this comment.
That shouldn't happen if the exporter is implemented correctly. Exporter::Shutdown() should ensure all cleanups ( that includes pending/ongoing synchronous/asynchronous calls ) are completed before returning. And TracerProvider::Shutdown() would return only after.
There was a problem hiding this comment.
But TracerProvicer::Shutdown() could return fail or timeout as the spec mentioned, will TracerProvider continue to destruct in these cases?
There was a problem hiding this comment.
LGTM.
We can see if there are comments from @jsuereth, @pyohannes, and others before merging.
Fixes #695
Changes
Added
SetInstrumentationLibrarytoRecordableinterface which is used to populate instrumentation library to exporters.For significant contributions please make sure you have completed the following items:
CHANGELOG.mdupdated for non-trivial changes