Skip to content

feat: add YAML parsing support for Composable Samplers#3966

Open
DCchoudhury15 wants to merge 10 commits intoopen-telemetry:mainfrom
DCchoudhury15:feat/config-composable-samplers
Open

feat: add YAML parsing support for Composable Samplers#3966
DCchoudhury15 wants to merge 10 commits intoopen-telemetry:mainfrom
DCchoudhury15:feat/config-composable-samplers

Conversation

@DCchoudhury15
Copy link
Copy Markdown

Fixes #3914

Changes

This PR implements the necessary infrastructure and test coverage to fully support composable samplers. Specifically, the changes include:

  • Parser Logic: Implemented the YAML parsing logic to properly extract composable sampler configurations.
  • SDK Builder: Added SDK builder visitor instantiation to correctly wire the new sampler variants.
  • Test Safety: Replaced dangerous casts with the safe Visitor pattern in the testing suite.
  • Test Coverage: Expanded unit test coverage to ensure all composable sampler variants are accurately parsed and instantiated.

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

@DCchoudhury15 DCchoudhury15 requested a review from a team as a code owner April 3, 2026 10:02
Implements parser logic, SDK builder visitor instantiation, safe test patterns, and expands coverage for all composable sampler variants to resolve open-telemetry#3914.

Signed-off-by: DCchoudhury15 <divyanshuchoudhury3@gmail.com>
@DCchoudhury15 DCchoudhury15 force-pushed the feat/config-composable-samplers branch from 92ed254 to ae0c9dd Compare April 3, 2026 10:09
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 3, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.17%. Comparing base (185dbd7) to head (1ae7f8c).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3966      +/-   ##
==========================================
- Coverage   90.18%   90.17%   -0.01%     
==========================================
  Files         230      230              
  Lines        7299     7299              
==========================================
- Hits         6582     6581       -1     
- Misses        717      718       +1     

see 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.

@marcalff
Copy link
Copy Markdown
Member

marcalff commented Apr 3, 2026

@DCchoudhury15

Thanks for the PR.

Please check the CI logs for failures, and adjust the code accordingly.

In the mean time, I will start the code review and provide feedback.

Thanks for your contribution.

Examples of failures below


Include what you use:

[ 57%] Building CXX object sdk/src/configuration/CMakeFiles/opentelemetry_configuration.dir/yaml_configuration_parser.cc.o
Warning: include-what-you-use reported diagnostics:

/home/runner/work/opentelemetry-cpp/opentelemetry-cpp/sdk/src/configuration/configuration_parser.cc should add these lines:
#include "opentelemetry/sdk/configuration/composable_always_off_sampler_configuration.h"
#include "opentelemetry/sdk/configuration/composable_always_on_sampler_configuration.h"
#include "opentelemetry/sdk/configuration/composable_parent_threshold_sampler_configuration.h"
#include "opentelemetry/sdk/configuration/composable_probability_sampler_configuration.h"
#include "opentelemetry/sdk/configuration/composable_rule_based_sampler_configuration.h"
#include "opentelemetry/sdk/configuration/composable_sampler_configuration.h"

Build in maintainer mode

[ 53%] Building CXX object sdk/src/configuration/CMakeFiles/opentelemetry_configuration.dir/configuration_parser.cc.o
In file included from /home/runner/work/opentelemetry-cpp/opentelemetry-cpp/sdk/src/configuration/configuration_parser.cc:29:
In file included from /home/runner/work/opentelemetry-cpp/opentelemetry-cpp/sdk/include/opentelemetry/sdk/configuration/configuration_parser.h:20:
/home/runner/work/opentelemetry-cpp/opentelemetry-cpp/sdk/include/opentelemetry/sdk/configuration/composable_always_off_sampler_configuration.h:26:28: error: no newline at end of file [-Werror,-Wnewline-eof]
   26 | OPENTELEMETRY_END_NAMESPACE
      |                            ^

Clang-tidy

Download the clang-tidy report from ci, and inspect failures related to new code.


opentelemetry-cpp/sdk/include/opentelemetry/sdk/configuration/composable_always_off_sampler_configuration.h (1 warnings)Line Check Message15 cppcoreguidelines-special-member-functions class ‘ComposableAlwaysOffSamplerConfiguration’ defines a default destructor but
does not define a copy constructor, a copy assignment operator, a move constructor
or a move assignment operator 

@DCchoudhury15
Copy link
Copy Markdown
Author

Hi @marcalff, addressed all the CI failures:

  • Added direct includes in configuration_parser.cc (IWYU)
  • Fixed missing newlines at end of the new headers (clang-tidy)
  • Dropped the explicit destructors since the base class already handles it
    Also did some changes in the test visitor for a SamplerType enum while I was at it.
    Thanks for the patience!

Signed-off-by: DCchoudhury15 <divyanshuchoudhury3@gmail.com>
@DCchoudhury15 DCchoudhury15 force-pushed the feat/config-composable-samplers branch from f5be4bf to 659aafe Compare April 4, 2026 07:24
{
model = ParseTraceIdRatioBasedSamplerConfiguration(child, depth);
}
else if (name == "composable_always_off")
Copy link
Copy Markdown
Member

@lalitb lalitb Apr 8, 2026

Choose a reason for hiding this comment

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

This is confusing to me - As per the config specs, the YAML shape should be something like:

tracer_provider:
  sampler:
    composite/development:
      always_on:

However in the implementation we are using composable_* sampler keys, which seems incorrect. Or am I missing something.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Lalit is correct here, the only name to check should be "composite/development".

In this case, invoke ParseComposableSamplerConfiguration(), which will lookup the child yaml nodes.

size_t /* depth */) const
{
auto model = std::make_unique<ComposableProbabilitySamplerConfiguration>();
model->probability = node->GetDouble("probability", 1.0);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The property name in the spec is ratio.

const std::unique_ptr<DocumentNode> & /* node */,
size_t /* depth */) const
{
return std::make_unique<ComposableParentThresholdSamplerConfiguration>();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Property root is not parsed from yaml.

const std::unique_ptr<DocumentNode> & /* node */,
size_t /* depth */) const
{
// FIXME-CONFIG: Parse rules list from YAML node here
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

To implement, parse the array of rules from yaml.

const std::unique_ptr<DocumentNode> & /* node */,
size_t /* depth */) const
{
return std::make_unique<ComposableSamplerConfiguration>();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is an empty node.

The code needs to parse the child nodes in yaml, and create the proper sampler.

See

    minProperties: 1
    maxProperties: 1
    properties:
      always_off:
        $ref: "#/$defs/ExperimentalComposableAlwaysOffSampler"
        description: Configure sampler to be always_off.
        defaultBehavior: ignore
      always_on:
        $ref: "#/$defs/ExperimentalComposableAlwaysOnSampler"
        description: Configure sampler to be always_on.
        defaultBehavior: ignore
      parent_threshold:
        $ref: "#/$defs/ExperimentalComposableParentThresholdSampler"
        description: |
          Configure sampler to be parent_threshold.
        defaultBehavior: ignore
      probability:
        $ref: "#/$defs/ExperimentalComposableProbabilitySampler"
        description: Configure sampler to be probability.
        defaultBehavior: ignore
      rule_based:
        $ref: "#/$defs/ExperimentalComposableRuleBasedSampler"
        description: Configure sampler to be rule_based.
        defaultBehavior: ignore

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

Focusing on the yaml part for now: all the yaml nodes and properties should be parsed, and represented in memory in C++ classes.

This includes rules with attribute_values, attribute_patterns and span_kinds.

{
model = ParseTraceIdRatioBasedSamplerConfiguration(child, depth);
}
else if (name == "composable_always_off")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Lalit is correct here, the only name to check should be "composite/development".

In this case, invoke ParseComposableSamplerConfiguration(), which will lookup the child yaml nodes.

Comment on lines +932 to +933
sampler:
composable_always_on:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is not the expected schema.

See file snippets/Sampler_rule_based_kitchen_sink.yaml in the opentelemetry-configuration repository.

tracer_provider:
  processors:
    - simple:
        exporter:
          console:
  sampler:
    # SNIPPET_START
    composite/development:
      ...

@DCchoudhury15 DCchoudhury15 force-pushed the feat/config-composable-samplers branch from 2e15234 to 758e3a5 Compare April 10, 2026 05:48
@DCchoudhury15
Copy link
Copy Markdown
Author

I have applied the changes given on the pr review , will resolve all the ci errors and commit again asap .

Signed-off-by: DCchoudhury15 <divyanshuchoudhury3@gmail.com>
@DCchoudhury15 DCchoudhury15 force-pushed the feat/config-composable-samplers branch from 13f24d4 to 1a4617d Compare April 11, 2026 01:07
Signed-off-by: DCchoudhury15 <divyanshuchoudhury3@gmail.com>
@DCchoudhury15 DCchoudhury15 force-pushed the feat/config-composable-samplers branch from e207db5 to e22556d Compare April 13, 2026 04:32
virtual void VisitParentBased(const ParentBasedSamplerConfiguration *model) = 0;
virtual void VisitTraceIdRatioBased(const TraceIdRatioBasedSamplerConfiguration *model) = 0;
virtual void VisitExtension(const ExtensionSamplerConfiguration *model) = 0;
virtual void VisitComposableAlwaysOff(const ComposableAlwaysOffSamplerConfiguration *model) = 0;
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.

Provide default implementation to avoid breaking downstream users for these 2 virtual functions?

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.

[CONFIGURATION] File configuration - composable samplers

4 participants