-
Notifications
You must be signed in to change notification settings - Fork 451
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SDK] Add tracer scope configurator #3137
Conversation
✅ Deploy Preview for opentelemetry-cpp-api-docs canceled.
|
6e15bc3
to
75d84d8
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3137 +/- ##
==========================================
+ Coverage 87.80% 87.91% +0.12%
==========================================
Files 198 201 +3
Lines 6324 6385 +61
==========================================
+ Hits 5552 5613 +61
Misses 772 772
|
dd70641
to
0370722
Compare
b82ea68
to
f6ff444
Compare
93950bf
to
c77065e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the patch, this is making good progress.
See another round comments on the code and tests.
b382755
to
8511e80
Compare
5b72b3e
to
8ef20b2
Compare
fix dangling pointer warnings & valgrind memcheck warnings
8ef20b2
to
98ec24d
Compare
Hi @marcalff, |
sdk/include/opentelemetry/sdk/instrumentationscope/scope_configurator.h
Outdated
Show resolved
Hide resolved
sdk/include/opentelemetry/sdk/instrumentationscope/scope_configurator.h
Outdated
Show resolved
Hide resolved
Update string_view -> string for use within the function scope to ensure scope lifetime. Update push_back -> emplace_back to construct directly within the vector container. Added a constructor to the Condition struct to enable emplace_back.
Making builder's methods return reference type prevents unneeded copies of the object.
Hi @chusitoo, Please take a look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@psx95 thanks for the fixes. These changes look good to me 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for the feature.
"accept_third_attr", 3}; | ||
|
||
static instrumentation_scope::InstrumentationScope test_scope_1 = | ||
*instrumentation_scope::InstrumentationScope::Create("test_scope_1"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is weird to invoke Create() and then use the object behind the unique ptr, but given how InstrumentationScope does not expose public constructors, this is the best the test can do.
Approved as is then.
Contributes to #2641
Adds scope configurator for Tracers.
Changes
Some optional, good-to-have convenience functions were recommended by the spec to accommodate common use-cases - these have not been added in this PR.
See the TracerConfigurator spec for these recommendations. They should be added in a future PR.
For significant contributions please make sure you have completed the following items:
CHANGELOG.md
updated for non-trivial changes