Skip to content

Sampler tracer integration#128

Merged
reyang merged 63 commits into
open-telemetry:masterfrom
ziqizh:sampler-tracer-integration
Jul 7, 2020
Merged

Sampler tracer integration#128
reyang merged 63 commits into
open-telemetry:masterfrom
ziqizh:sampler-tracer-integration

Conversation

@ziqizh
Copy link
Copy Markdown
Contributor

@ziqizh ziqizh commented Jun 24, 2020

Integrate sampler into Tracer.
Dependency: Always On Sampler #122 and Always Off Sampler #125

  • Integration of tracer_provider:
    Implemented the constructor, setter and getter for sampler stored as an atomic_share_pointer.
  • Why atomic_share_pointer:
    Because multiple tracers may share the same sampler. When one program changes the sampler, others might have trouble reading it, creating a race condition.
    Note: atomic_share_pointer only protects the pointer, but doesn't guarantee the content of the object to be thread safe.
  • Tracer
    Added a reference of the sampler in the Tracer
  • Added test case

Comment thread sdk/src/trace/tracer_provider.cc Outdated
@ziqizh ziqizh marked this pull request as ready for review June 29, 2020 15:30
@ziqizh ziqizh requested a review from a team June 29, 2020 15:30
Comment thread sdk/include/opentelemetry/sdk/trace/tracer.h Outdated
Comment thread sdk/test/trace/always_on_sampler_test.cc
Comment thread sdk/test/trace/always_on_sampler_test.cc
Comment thread sdk/include/opentelemetry/sdk/trace/tracer_provider.h Outdated
/**
* @return Description MUST be AlwaysOnSampler
*/
std::string GetDescription() const noexcept override { return "AlwaysOnSampler"; }
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If description is always expected to be a string literal, we could return a string_view instead of a std::string.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. a string_view can be more efficient. I need to open another PR for this issue, since we need to change the data type in multiple header files.

Comment thread sdk/include/opentelemetry/sdk/trace/tracer_provider.h Outdated
Comment thread sdk/include/opentelemetry/sdk/trace/tracer_provider.h
Comment thread sdk/include/opentelemetry/sdk/trace/tracer.h Outdated
Comment thread sdk/test/trace/tracer_provider_test.cc Outdated
Comment thread sdk/test/trace/tracer_test.cc Outdated
Copy link
Copy Markdown
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.

That looks good.

As discussed, we will integrate the ShouldSample call in a separate PR.

@ziqizh
Copy link
Copy Markdown
Contributor Author

ziqizh commented Jul 7, 2020

@reyang Could you please take a look of this PR and merge it? Thank you!

Copy link
Copy Markdown
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.

LGTM.

@reyang
Copy link
Copy Markdown
Member

reyang commented Jul 7, 2020

@ziqizh please rebase, thanks.

@ziqizh
Copy link
Copy Markdown
Contributor Author

ziqizh commented Jul 7, 2020

Done.

@reyang reyang merged commit 65f6639 into open-telemetry:master Jul 7, 2020
@reyang reyang mentioned this pull request Aug 25, 2020
GerHobbelt pushed a commit to GerHobbelt/opentelemetry-cpp that referenced this pull request Jun 17, 2025
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.

6 participants