Add incubator ComposableRuleBasedSampler #7787

anuraaga · 2025-10-23T07:42:59Z

Previous PRs have focused on the composable versions of existing samplers. This adds one of the new ones, ComposableRuleBased.

https://opentelemetry.io/docs/specs/otel/trace/sdk/#composablerulebased

Though technically a similar one has been in contrib for a while - IIUC, the agent uses it now adays so would be good to confirm the general usability from a javaagent perspective too /cc @jaydeluca

anuraaga · 2025-10-23T07:43:45Z

...java/io/opentelemetry/sdk/extension/incubator/trace/samplers/ComposableRuleBasedSampler.java

+    this.rules = rules.toArray(new SamplingRule[0]);
+
+    StringBuilder description = new StringBuilder("ComposableRuleBasedSampler{[");
+    if (this.rules.length > 0) {


I considered whether empty should be an error, but I could conceive of cases that update the rules dynamically in a sort of "default-to-deny-all, sometimes accept something" type of situation. If making empty an error, that's still implementable though with an explicit catch-all and drop rule, so open to thoughts.

Will go with it for now but realized that this is probably something to just confirm on the spec

anuraaga · 2025-10-23T07:44:28Z

.../opentelemetry/sdk/extension/incubator/trace/samplers/ComposableRuleBasedSamplerBuilder.java

+import java.util.List;
+
+/** A builder for a composable rule-based sampler. */
+public final class ComposableRuleBasedSamplerBuilder {


This is just an initial implementation. Presumably this should have at least a few helpers on it, addAttributePattern for example

anuraaga · 2025-10-23T07:45:13Z

...src/main/java/io/opentelemetry/sdk/extension/incubator/trace/samplers/SamplingPredicate.java

+   * Returns a description of the {@link SamplingPredicate}. This may be displayed on debug pages or
+   * in the logs.
+   */
+  String getDescription();


Thought long on whether this should be a FunctionalInterface, but I felt just like Sampler, this is too important to not require

What are you thoughts on Sampler#getDescription() and this? I've always seen Sampler#getDescription() and thought it unnecessary because:

Its not used anywhere in the SDK

Its just like Object#toString(), but you're forced to implement it

Maybe the point you're making is that its important to have a human readable description of the sampler for things like OpenTelemetrySdk#toString(), and so its good to force sampler to print a description?

If so, I don't necessarily disagree, but then I would have liked to have getDescription() on all SDK plugin interfaces (i.e. SpanExporter, SpanProcessor, etc).

Yeah it's a good point that with discipline, it's enough to implement toString sensibly at least in the SDK implementations, that was generally the case in zipkin. I can imagine a world without Sampler#getDescription() working out fine. But that isn't the world - for this, it feels to me we can't implement Sampler#getDescription() properly without ensuring predicates do the same. We could still suggest strongly to implement toString for use there if it seems better though.

Yeah I've definitely run into that problem with OpenTelemetrySdk#toString() having sub-optimal results unless every sub-component follows the convention of a useful toString() implementation. In retrospect I wish we would have had getDescription be required on all SDK extension plugin interfaces.

I don't have a strong opinion here. I lean slightly towards relying on toString() because we missed the boat on getDescription() elsewhere, but don't feel strongly. Requiring getDescription will improve the string output for this narrow part of the codebase, which is worth something.

Sounds good, I removed getDescription - always good to err on the side of less API. I added a javadoc strongly recommending toString instead.

codecov · 2025-10-23T07:46:53Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.17%. Comparing base (18780a3) to head (65ad8e7).
⚠️ Report is 16 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##               main    #7787   +/-   ##
=========================================
  Coverage     90.17%   90.17%           
- Complexity     7189     7216   +27     
=========================================
  Files           814      819    +5     
  Lines         21730    21767   +37     
  Branches       2129     2133    +4     
=========================================
+ Hits          19594    19629   +35     
- Misses         1467     1468    +1     
- Partials        669      670    +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

jack-berg · 2025-10-23T15:49:41Z

Though technically a similar one has been in contrib for a while - IIUC, the agent uses it now adays so would be good to confirm the general usability from a javaagent perspective too

Best way for these to be usable in the agent is to drive the declarative configuration implementation of this, which includes:

Defining declarative config JSON schema types for the new spec components: ProbabilitySampler Configuration opentelemetry-configuration#315
Updating the java implementation of declarative config to understand the new types

At that point, users would be able to define complex composable samplers in YAML and use them in the agent without an agent extension.

anuraaga · 2025-10-24T02:35:26Z

Thanks @jack-berg - agree that it sounds like a good idea to get new samplers into configuration. I raised some PR/issues there to see if we can move forward. At the same time, I guess we can get this in to have the SDK side moving as well.

jack-berg

Couple minor comments but looks good overall. Thanks!

jack-berg · 2025-10-24T14:05:00Z

...src/main/java/io/opentelemetry/sdk/extension/incubator/trace/samplers/SamplingPredicate.java

+import java.util.List;
+
+/** A predicate for a composable sampler, indicating whether a set of sampling arguments matches. */
+public interface SamplingPredicate {


The spec seems incomplete in that it lists a rule as a predicate, ComposableSampler pair, but fails to describe what parameters the predicate can operate on. I think including all the parameters of ComposableSampler#getSamplingINtent is the natural choice.

open-telemetry/opentelemetry-specification#4701

jack-berg · 2025-10-24T14:08:37Z

...src/main/java/io/opentelemetry/sdk/extension/incubator/trace/samplers/SamplingPredicate.java

+   * Returns a description of the {@link SamplingPredicate}. This may be displayed on debug pages or
+   * in the logs.
+   */
+  String getDescription();


What are you thoughts on Sampler#getDescription() and this? I've always seen Sampler#getDescription() and thought it unnecessary because:

Its not used anywhere in the SDK

Its just like Object#toString(), but you're forced to implement it

Maybe the point you're making is that its important to have a human readable description of the sampler for things like OpenTelemetrySdk#toString(), and so its good to force sampler to print a description?

If so, I don't necessarily disagree, but then I would have liked to have getDescription() on all SDK plugin interfaces (i.e. SpanExporter, SpanProcessor, etc).

...ator/src/main/java/io/opentelemetry/sdk/extension/incubator/trace/samplers/SamplingRule.java

.../opentelemetry/sdk/extension/incubator/trace/samplers/ComposableRuleBasedSamplerBuilder.java

...java/io/opentelemetry/sdk/extension/incubator/trace/samplers/ComposableRuleBasedSampler.java

anuraaga

Thanks @jack-berg sorry for the delay

anuraaga · 2025-10-30T01:45:25Z

...src/main/java/io/opentelemetry/sdk/extension/incubator/trace/samplers/SamplingPredicate.java

+   * Returns a description of the {@link SamplingPredicate}. This may be displayed on debug pages or
+   * in the logs.
+   */
+  String getDescription();


Yeah it's a good point that with discipline, it's enough to implement toString sensibly at least in the SDK implementations, that was generally the case in zipkin. I can imagine a world without Sampler#getDescription() working out fine. But that isn't the world - for this, it feels to me we can't implement Sampler#getDescription() properly without ensuring predicates do the same. We could still suggest strongly to implement toString for use there if it seems better though.

...java/io/opentelemetry/sdk/extension/incubator/trace/samplers/ComposableRuleBasedSampler.java

anuraaga · 2025-10-30T01:49:00Z

...java/io/opentelemetry/sdk/extension/incubator/trace/samplers/ComposableRuleBasedSampler.java

+    this.rules = rules.toArray(new SamplingRule[0]);
+
+    StringBuilder description = new StringBuilder("ComposableRuleBasedSampler{[");
+    if (this.rules.length > 0) {


Will go with it for now but realized that this is probably something to just confirm on the spec

jack-berg

Let me know where you land on getDescription(). I'm happy to merge either way.

Thanks!

otelbot · 2025-10-31T16:56:29Z

Thank you for your contribution @anuraaga! 🎉 We would like to hear from you about your experience contributing to OpenTelemetry by taking a few minutes to fill out this survey.

Add incubator ComposableRuleBasedSampler

9ebd9d3

anuraaga requested a review from a team as a code owner October 23, 2025 07:43

anuraaga commented Oct 23, 2025

View reviewed changes

jack-berg approved these changes Oct 24, 2025

View reviewed changes

Cleanup

a6b79e4

anuraaga commented Oct 30, 2025

View reviewed changes

jack-berg approved these changes Oct 30, 2025

View reviewed changes

Remove getDescription

65ad8e7

jack-berg merged commit dfdc85f into open-telemetry:main Oct 31, 2025
29 checks passed

the-clam pushed a commit to the-clam/opentelemetry-java that referenced this pull request Nov 6, 2025

Add incubator ComposableRuleBasedSampler (open-telemetry#7787)

5b3567a

Add incubator ComposableRuleBasedSampler #7787

Add incubator ComposableRuleBasedSampler #7787

Uh oh!

Conversation

anuraaga commented Oct 23, 2025

Uh oh!

anuraaga Oct 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jack-berg Oct 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Oct 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jack-berg commented Oct 23, 2025

Uh oh!

anuraaga commented Oct 24, 2025

Uh oh!

jack-berg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

anuraaga left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jack-berg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

otelbot bot commented Oct 31, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

anuraaga Oct 23, 2025 •

edited

Loading

jack-berg Oct 30, 2025 •

edited

Loading

codecov bot commented Oct 23, 2025 •

edited

Loading