Skip to content

Conversation

@majanjua-amzn
Copy link

@majanjua-amzn majanjua-amzn commented Nov 27, 2025

Background

Spec issue: open-telemetry/opentelemetry-specification#4698
Spec PR: open-telemetry/opentelemetry-specification#4699

Adding built in access to the RECORD_ONLY flag for sampling to allow for users to process spans without exporting them.

Changes

Testing

Added unit tests, ./gradlew spotlessApply && ./gradlew build && ./gradlew check as per CONTRIBUTING.md guide

@codecov
Copy link

codecov bot commented Nov 27, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.15%. Comparing base (18f8a13) to head (f699191).

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #7877   +/-   ##
=========================================
  Coverage     90.15%   90.15%           
- Complexity     7476     7481    +5     
=========================================
  Files           836      837    +1     
  Lines         22551    22567   +16     
  Branches       2224     2225    +1     
=========================================
+ Hits          20330    20346   +16     
  Misses         1517     1517           
  Partials        704      704           

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

@majanjua-amzn
Copy link
Author

Ready for review!

Copy link
Contributor

@breedx-splk breedx-splk 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. I think my biggest question is about how this will get used (if internal), but otherwise is looking good to me. There's also a merge conflict that needs to be resolved before merge.

// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
// SPDX-License-Identifier: Apache-2.0

package io.opentelemetry.sdk.trace.internal;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the correct package? How are you planning for this to be consumed by users if it's internal?

Copy link
Member

Choose a reason for hiding this comment

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

Experimental SDK components is the place we've been least consistent about in terms of packaging, since sometimes the concept needs to be bundled into the SDK internals to work (i.e. ExceptionAttributeResolver). But for components which are SDK extension plugin interfaces (span processor, sampler, etc), we've been keeping them in opentelemetry-sdk-extension-incubator. Here's the relevant package for samplers: https://github.com/open-telemetry/opentelemetry-java/tree/main/sdk-extensions/incubator/src/main/java/io/opentelemetry/sdk/extension/incubator/trace/samplers

Copy link
Author

Choose a reason for hiding this comment

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

Moved the code to the incubator package

SamplingResult result =
rootSampler.shouldSample(parentContext, traceId, name, spanKind, attributes, parentLinks);
if (result.getDecision() == SamplingDecision.DROP) {
result = wrapResultWithRecordOnlyResult(result);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: no need to fall through. I think it reads marginally better to return early.

Suggested change
result = wrapResultWithRecordOnlyResult(result);
return wrapResultWithRecordOnlyResult(result);

Copy link
Author

Choose a reason for hiding this comment

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

Improved this as part of the changes for another comment: replaced the method with a delegate wrapper class instead.

Comment on lines 61 to 90
if (result.getDecision() == SamplingDecision.DROP) {
result = wrapResultWithRecordOnlyResult(result);
}

return result;
}

@Override
public String getDescription() {
return "AlwaysRecordSampler{" + rootSampler.getDescription() + "}";
}

private static SamplingResult wrapResultWithRecordOnlyResult(SamplingResult result) {
return new SamplingResult() {
@Override
public SamplingDecision getDecision() {
return SamplingDecision.RECORD_ONLY;
}

@Override
public Attributes getAttributes() {
return result.getAttributes();
}

@Override
public TraceState getUpdatedTraceState(TraceState parentTraceState) {
return result.getUpdatedTraceState(parentTraceState);
}
};
}
Copy link
Member

Choose a reason for hiding this comment

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

Let's use a concrete class instead of an anonymous class:

Suggested change
if (result.getDecision() == SamplingDecision.DROP) {
result = wrapResultWithRecordOnlyResult(result);
}
return result;
}
@Override
public String getDescription() {
return "AlwaysRecordSampler{" + rootSampler.getDescription() + "}";
}
private static SamplingResult wrapResultWithRecordOnlyResult(SamplingResult result) {
return new SamplingResult() {
@Override
public SamplingDecision getDecision() {
return SamplingDecision.RECORD_ONLY;
}
@Override
public Attributes getAttributes() {
return result.getAttributes();
}
@Override
public TraceState getUpdatedTraceState(TraceState parentTraceState) {
return result.getUpdatedTraceState(parentTraceState);
}
};
}
if (result.getDecision() != SamplingDecision.DROP) {
return result;
}
return new RecordOnlyDelegateSamplingResult(result);
}
@Override
public String getDescription() {
return "AlwaysRecordSampler{" + rootSampler.getDescription() + "}";
}
private static class RecordOnlyDelegateSamplingResult implements SamplingResult {
private final SamplingResult delegate;
private RecordOnlyDelegateSamplingResult(SamplingResult delegate) {this.delegate = delegate;}
@Override
public SamplingDecision getDecision() {
return SamplingDecision.RECORD_ONLY;
}
@Override
public Attributes getAttributes() {
return delegate.getAttributes();
}
@Override
public TraceState getUpdatedTraceState(TraceState parentTraceState) {
return delegate.getUpdatedTraceState(parentTraceState);
}
}

Copy link
Author

Choose a reason for hiding this comment

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

Done

}

@Test
void testGetDescription() {
Copy link
Member

Choose a reason for hiding this comment

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

remote test prefix on all these

Copy link
Author

Choose a reason for hiding this comment

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

Done

}

@Test
void testRecordAndSampleSamplingDecision() {
Copy link
Member

Choose a reason for hiding this comment

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

Can use a org.junit.jupiter.params.ParameterizedTest test to improve readability.

Copy link
Author

Choose a reason for hiding this comment

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

Done!

Copy link
Member

@jack-berg jack-berg left a comment

Choose a reason for hiding this comment

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

Couple small things that need adjusting. Thanks for working on this!

@jack-berg jack-berg linked an issue Dec 19, 2025 that may be closed by this pull request
@breedx-splk
Copy link
Contributor

bump @majanjua-amzn in case you can circle back to this now in the new year. 😉 Thanks again!

@majanjua-amzn majanjua-amzn force-pushed the main branch 2 times, most recently from 7d9e351 to 3dd5a79 Compare January 21, 2026 00:05
@majanjua-amzn
Copy link
Author

Ready for review again! Apologies for the delay, cc @jack-berg since you marked it as requested change, thanks in advance

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.

Add alwaysRecord Sampler

3 participants