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 TraceState in SamplerResult, and use it for creating span context. #591

Merged
merged 7 commits into from
Mar 11, 2021

Conversation

lalitb
Copy link
Member

@lalitb lalitb commented Feb 26, 2021

Fixes #556

Changes

Changes includes

  • Adding trace_state in sampling result. For custom samplers provided by sdk, this is trace_state from parent context ( if available), else default.
  • Passing this trace_state in sdk::span constructor to be used for creating current span context.
  • Use this trace_state while creating current span context in sdk::span

For significant contributions please make sure you have completed the following items:

  • CHANGELOG.md updated for non-trivial changes
  • Unit tests have been added
  • Changes in public API reviewed

@lalitb lalitb requested a review from a team February 26, 2021 16:51
@lalitb lalitb changed the title Add TraceState in Samplers Add TraceState in SamplerResult, and using it for creating span context. Feb 26, 2021
@lalitb lalitb changed the title Add TraceState in SamplerResult, and using it for creating span context. Add TraceState in SamplerResult, and use it for creating span context. Feb 26, 2021
@codecov
Copy link

codecov bot commented Feb 26, 2021

Codecov Report

Merging #591 (d8f55cb) into main (a33bb55) will increase coverage by 0.01%.
The diff coverage is 95.65%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #591      +/-   ##
==========================================
+ Coverage   94.39%   94.41%   +0.01%     
==========================================
  Files         191      191              
  Lines        9053     9066      +13     
==========================================
+ Hits         8546     8560      +14     
+ Misses        507      506       -1     
Impacted Files Coverage Δ
sdk/include/opentelemetry/sdk/trace/sampler.h 100.00% <ø> (ø)
sdk/src/trace/span.h 100.00% <ø> (ø)
...lude/opentelemetry/sdk/trace/samplers/always_off.h 80.00% <66.66%> (-20.00%) ⬇️
...clude/opentelemetry/sdk/trace/samplers/always_on.h 100.00% <100.00%> (ø)
sdk/src/trace/samplers/parent.cc 100.00% <100.00%> (ø)
sdk/src/trace/span.cc 89.47% <100.00%> (ø)
sdk/src/trace/tracer.cc 75.00% <100.00%> (+0.71%) ⬆️
sdk/test/trace/always_off_sampler_test.cc 86.66% <100.00%> (+0.95%) ⬆️
sdk/test/trace/always_on_sampler_test.cc 91.66% <100.00%> (+0.75%) ⬆️
sdk/test/trace/parent_sampler_test.cc 100.00% <100.00%> (ø)
... and 1 more

@@ -22,7 +22,9 @@ class Span final : public trace_api::Span
const trace_api::SpanContextKeyValueIterable &links,
const trace_api::StartSpanOptions &options,
const trace_api::SpanContext &parent_span_context,
const opentelemetry::sdk::resource::Resource &resource) noexcept;
const opentelemetry::sdk::resource::Resource &resource,
const nostd::shared_ptr<opentelemetry::trace::TraceState> trace_state =
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI, this might be conflicted with the trace_flags change which needs to be resolved.

@lalitb lalitb merged commit 9aba950 into open-telemetry:main Mar 11, 2021
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.

Sampler: SamplingResult should return TraceState
3 participants