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

[ETW Exporter] Tail based sampling support #1780

Merged
merged 18 commits into from
Dec 2, 2022

Conversation

lalitb
Copy link
Member

@lalitb lalitb commented Nov 16, 2022

Changes

Adding limited support for tail based sampling. Similar to head-sampling, TailSampler::ShouldSample() method needs to be overridden with sampling logic, which get's executed before the span is exporter.

class TailSampler
{
public:
  // convert to etw span if required for getters on span.
  // auto etw_span = static_cast<const opentelemetry::exporter::etw::Span*>(&span);
  // Decision based on
  //      Span::GetStatus()
  //      Span::GetProperties()
  //      Span::GetContext()
  virtual opentelemetry::sdk::trace::SamplingResult ShouldSample(
      const opentelemetry::trace::Span &span) noexcept = 0;
};

The inherited TailSampler class object needs to be passed as argument to TracerProvider constructor. The default is AlwaysOnTailSampler

Also, added use of macro OPENTELEMETRY_NOT_USE_RTTI to disable using RTTI in ETW exporter even if it is enabled in compiler. This is because the ETW exporter modified the attributes passed as argument if RTTI is enabled to reuse them for creating ETW event structure, which is sometime not desirable.

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 November 16, 2022 21:05
@lalitb lalitb marked this pull request as draft November 16, 2022 21:07
@codecov
Copy link

codecov bot commented Nov 16, 2022

Codecov Report

Merging #1780 (e9423ee) into main (f23c4a2) will decrease coverage by 0.02%.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1780      +/-   ##
==========================================
- Coverage   85.73%   85.71%   -0.01%     
==========================================
  Files         171      171              
  Lines        5240     5240              
==========================================
- Hits         4492     4491       -1     
- Misses        748      749       +1     
Impacted Files Coverage Δ
sdk/src/trace/batch_span_processor.cc 90.63% <0.00%> (-0.78%) ⬇️

@lalitb lalitb marked this pull request as ready for review November 17, 2022 01:26
{"enableRelatedActivityId", true},
{"enableAutoParent", true}
},
std::move(always_on),
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we start a span and test it is indeed dropped?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes I was thinking to mock the actual ETW transport layer and do the test. In current design, this is not easy to do, perhaps can be taken separately if it is fine?

const opentelemetry::trace::Span &spanBase =
reinterpret_cast<const opentelemetry::trace::Span &>(span);

// Sample span based on the decision of tail based sampler
auto sampling_result = const_cast<TailSampler *>(&tail_sampler)->ShouldSample(spanBase);
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of making a virtual call to AlwaysOn sampler by default, perhaps check if there is a non-default sampler set, then make the virtual call? In this way, it doesn't introduce extra cost (except the null check) when tail based sampler is not used.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good comment. Actually we do have virtual method calls for head-sampler, id-generator both in ETW exporter, and the otel-sdk. There is no reported performance reported with them. Seemingly modern compilers can optimize these repeated calls by caching the address, and avoid virtual indirection. Perhaps we can look back to this if there are reported overhead internally and/or by community.

@ThomsonTan ThomsonTan merged commit d74e02f into open-telemetry:main Dec 2, 2022
yxue pushed a commit to yxue/opentelemetry-cpp that referenced this pull request Dec 5, 2022
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.

2 participants