Add InstrumentationLibrary to Tracer #693
Conversation
Codecov Report
@@ Coverage Diff @@
## main #693 +/- ##
==========================================
+ Coverage 94.79% 94.81% +0.01%
==========================================
Files 212 214 +2
Lines 9707 9744 +37
==========================================
+ Hits 9202 9239 +37
Misses 505 505
|
ThomsonTan
changed the title
There was a problem hiding this comment.
Change looks good assuming two things:
- We create new
Tracers for different libraries/versions (you have Support multiple tracers in TracerProvider #694) for that. - We fill out
InstrumentationLibraryin the OTLP exporter. Is there a bug open to track that work or do we need to open one?
Created #695 for this. |
Create new |
sdk/src/trace/tracer_provider.cc
Outdated
|
|
||
| const std::lock_guard<std::mutex> guard(lock_); | ||
|
|
||
| auto lib = InstrumentationLibrary::create(library_name, library_version); |
There was a problem hiding this comment.
If GetTracer were to be used in an instrumentation to retrieve a tracer when creating spans, this potentially creates 3 allocations and deallocs on a hot path.
Would it make sense to avoid heap allocations here for the usual case when a tracer already exists?
I mean using GetTracer() every time could perhaps be considered bad use of the API as opposed to caching a tracer in the instrumentation, but I think it might be too easy too misuse the API and just default to GetTracer()
There was a problem hiding this comment.
@seemk thanks. I think this is a valid concern. I updated the PR and delayed the heap allocation to the time when creating new Tracer is necessary.
Fixes #567 and #694.
Changes
Add
InstrumentationLibraryto SDK and return a new tracer for a differentInstruementationLibrary.For significant contributions please make sure you have completed the following items:
CHANGELOG.mdupdated for non-trivial changes