-
Notifications
You must be signed in to change notification settings - Fork 411
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
Add sampler header file #118
Conversation
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.
This is a good start.
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.
Overall looks good to me, might need to wait for the attributes PR #117.
I signed it |
…-cpp into sampling-api-header
Codecov Report
@@ Coverage Diff @@
## master #118 +/- ##
=======================================
Coverage 93.29% 93.29%
=======================================
Files 66 66
Lines 1656 1656
=======================================
Hits 1545 1545
Misses 111 111 |
* A key/value pair that can be used to set attributes. | ||
*/ | ||
struct AttributeKeyValue { | ||
nostd::string_view key; |
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.
Storing a string_view has some pitfalls - string_view is essentially a pointer to an underlying string, so it relies on the underlying string value to outlive the string_view's lifetime. If "key" isn't too large, I recommend just storing it as a string.
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.
I have two more nits regarding the doc strings, otherwise this looks good to me. The TODOs for Links and SpanContext can remain, as this is work that still needs to be done and is not part of this PR.
Also, there is no more dependency on #117 any more, so this can be merged as soon as the doc strings are addressed.
Great job!
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.
A few more nits. Please make sure you rebase. Then I think this is ready to be merged.
Looks all good from my side and ready to merge. @reyang |
LGTM, will merge after CI finished. |
|
This seems to be a nice to have thing. On the other side, I'm guessing even with a const map, the actual value are not guaranteed to be const?
Nope. |
sampler.h
under SDK.sampler.h
defines sampler and SamplingResult interface.Waiting for Support for attributes on spans #117 to merge first.