Skip to content
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

Merged
merged 31 commits into from
Jun 23, 2020
Merged

Conversation

ziqizh
Copy link
Contributor

@ziqizh ziqizh commented Jun 17, 2020

@ziqizh ziqizh requested a review from a team June 17, 2020 02:21
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jun 17, 2020

CLA Check
The committers are authorized under a signed CLA.

@ziqizh ziqizh marked this pull request as draft June 17, 2020 02:21
Copy link
Contributor

@pyohannes pyohannes left a 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.

sdk/include/opentelemetry/sdk/trace/sampler.h Outdated Show resolved Hide resolved
sdk/include/opentelemetry/sdk/trace/sampler.h Outdated Show resolved Hide resolved
sdk/include/opentelemetry/sdk/trace/sampler.h Outdated Show resolved Hide resolved
sdk/include/opentelemetry/sdk/trace/sampler.h Outdated Show resolved Hide resolved
sdk/include/opentelemetry/sdk/trace/sampler.h Outdated Show resolved Hide resolved
sdk/include/opentelemetry/sdk/trace/sampler.h Outdated Show resolved Hide resolved
@ziqizh ziqizh closed this Jun 17, 2020
@ziqizh ziqizh reopened this Jun 17, 2020
Copy link
Member

@reyang reyang left a 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.

@ziqizh ziqizh marked this pull request as ready for review June 17, 2020 19:29
@ziqizh
Copy link
Contributor Author

ziqizh commented Jun 17, 2020

I signed it

@ziqizh ziqizh mentioned this pull request Jun 19, 2020
@codecov
Copy link

codecov bot commented Jun 22, 2020

Codecov Report

Merging #118 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           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;

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.

sdk/include/opentelemetry/sdk/trace/sampler.h Outdated Show resolved Hide resolved
sdk/include/opentelemetry/sdk/trace/sampler.h Outdated Show resolved Hide resolved
sdk/include/opentelemetry/sdk/trace/sampler.h Outdated Show resolved Hide resolved
sdk/include/opentelemetry/sdk/trace/sampler.h Show resolved Hide resolved
sdk/include/opentelemetry/sdk/trace/sampler.h Show resolved Hide resolved
Copy link
Contributor

@pyohannes pyohannes left a 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!

sdk/include/opentelemetry/sdk/trace/sampler.h Outdated Show resolved Hide resolved
sdk/include/opentelemetry/sdk/trace/sampler.h Show resolved Hide resolved
sdk/include/opentelemetry/sdk/trace/sampler.h Outdated Show resolved Hide resolved
@nholbrook nholbrook mentioned this pull request Jun 23, 2020
Copy link
Contributor

@pyohannes pyohannes left a 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.

sdk/include/opentelemetry/sdk/trace/sampler.h Outdated Show resolved Hide resolved
sdk/include/opentelemetry/sdk/trace/sampler.h Outdated Show resolved Hide resolved
@pyohannes
Copy link
Contributor

Looks all good from my side and ready to merge. @reyang

@reyang
Copy link
Member

reyang commented Jun 23, 2020

LGTM, will merge after CI finished.

@ziqizh
Copy link
Contributor Author

ziqizh commented Jun 23, 2020

LGTM, will merge after CI finished.

  1. Should I change the attributes in SamplingResult from unique_ptr<map> to unique_ptr<const map> since the spec requires attributes to be immutable?
  2. I don't have authorization to merge this PR. Do I need to apply for a maintainer role?

@reyang reyang merged commit 70d86d5 into open-telemetry:master Jun 23, 2020
@reyang
Copy link
Member

reyang commented Jun 24, 2020

  1. Should I change the attributes in SamplingResult from unique_ptr<map> to unique_ptr<const map> since the spec requires attributes to be immutable?

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?

  1. I don't have authorization to merge this PR. Do I need to apply for a maintainer role?

Nope.

@nholbrook nholbrook mentioned this pull request Jun 30, 2020
@reyang reyang mentioned this pull request Aug 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants