Skip to content

Conversation

mathieurobin1
Copy link
Contributor

Fixes #3545

Changes

This PR updates the ParentBasedSampler to be fully compliant with the OpenTelemetry specification.

The previous implementation was a simplified version. This change introduces the complete logic, which distinguishes between local and remote parents, and whether they were sampled. The constructor now accepts five configurable delegate samplers with defaults as defined in the spec (AlwaysOnSampler and AlwaysOffSampler).

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

@mathieurobin1 mathieurobin1 requested a review from a team as a code owner July 20, 2025 20:16
Copy link

netlify bot commented Jul 20, 2025

Deploy Preview for opentelemetry-cpp-api-docs canceled.

Name Link
🔨 Latest commit fb1052d
🔍 Latest deploy log https://app.netlify.com/projects/opentelemetry-cpp-api-docs/deploys/687dabc6bc6de200083c61f2

@mathieurobin1 mathieurobin1 force-pushed the parent-based-sampler-options branch from 62a71fa to f148f7a Compare July 20, 2025 20:19
@mathieurobin1 mathieurobin1 changed the title [SDK] ParentBasedSampler is missing parameters [SDK] Implements options for the ParentBasedSampler with default values Jul 20, 2025
@mathieurobin1 mathieurobin1 force-pushed the parent-based-sampler-options branch from f148f7a to e68eb81 Compare July 20, 2025 20:20
Copy link

codecov bot commented Jul 20, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.03%. Comparing base (c406864) to head (fb1052d).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3553      +/-   ##
==========================================
+ Coverage   90.00%   90.03%   +0.03%     
==========================================
  Files         220      220              
  Lines        7056     7065       +9     
==========================================
+ Hits         6350     6360      +10     
+ Misses        706      705       -1     
Files with missing lines Coverage Δ
sdk/src/trace/samplers/parent.cc 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Member

@marcalff marcalff left a comment

Choose a reason for hiding this comment

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

Thanks the the PR.

See some minor comments, looks good overall.

@lalitb lalitb requested a review from Copilot July 21, 2025 16:22
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR enhances the ParentBasedSampler to fully comply with the OpenTelemetry specification by implementing comprehensive sampling logic that distinguishes between local and remote parents and their sampling states. The previous simplified implementation has been replaced with a complete solution that supports five configurable delegate samplers with spec-defined defaults.

Key changes:

  • Expanded ParentBasedSampler constructor to accept five delegate samplers with default values
  • Updated sampling logic to handle local vs remote parent contexts appropriately
  • Enhanced factory methods to support both simple and fully-configured sampler creation

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file
File Description
sdk/include/opentelemetry/sdk/trace/samplers/parent.h Updated ParentBasedSampler class with new constructor accepting five delegate samplers and improved documentation
sdk/src/trace/samplers/parent.cc Implemented new sampling logic that delegates based on parent context type and sampling state
sdk/include/opentelemetry/sdk/trace/samplers/parent_factory.h Added overloaded factory method for creating fully-configured ParentBasedSampler
sdk/src/trace/samplers/parent_factory.cc Implemented factory methods with default sampler instantiation
sdk/test/trace/parent_sampler_test.cc Extended test coverage for new local/remote parent sampling scenarios
CHANGELOG.md Added entry documenting the ParentBasedSampler enhancement
Comments suppressed due to low confidence (1)

sdk/src/trace/samplers/parent.cc:79

  • [nitpick] Extra blank line at end of file should be removed to maintain consistent formatting.
}  // namespace trace

@marcalff marcalff merged commit 5984a2d into open-telemetry:main Jul 21, 2025
70 checks passed
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.

[SDK] ParentBasedSampler is missing parameters

3 participants