Add AlwaysRecordSampler #7877
Add AlwaysRecordSampler #7877
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7877 +/- ##
============================================
+ Coverage 90.06% 90.10% +0.04%
- Complexity 7320 7329 +9
============================================
Files 825 826 +1
Lines 22048 22062 +14
Branches 2179 2180 +1
============================================
+ Hits 19857 19879 +22
+ Misses 1511 1503 -8
Partials 680 680 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Ready for review! |
| // Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| package io.opentelemetry.sdk.trace.internal; |
There was a problem hiding this comment.
Is this the correct package? How are you planning for this to be consumed by users if it's internal?
There was a problem hiding this comment.
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
| SamplingResult result = | ||
| rootSampler.shouldSample(parentContext, traceId, name, spanKind, attributes, parentLinks); | ||
| if (result.getDecision() == SamplingDecision.DROP) { | ||
| result = wrapResultWithRecordOnlyResult(result); |
There was a problem hiding this comment.
nit: no need to fall through. I think it reads marginally better to return early.
| result = wrapResultWithRecordOnlyResult(result); | |
| return wrapResultWithRecordOnlyResult(result); |
| if (rootDecision.equals(expectedDecision)) { | ||
| assertThat(actualResult).isEqualTo(rootResult); | ||
| assertThat(actualResult.getDecision()).isEqualTo(rootDecision); | ||
| } else { | ||
| assertThat(actualResult).isNotEqualTo(rootResult); | ||
| assertThat(actualResult.getDecision()).isEqualTo(expectedDecision); | ||
| } |
There was a problem hiding this comment.
nit: I think this makes the tests slightly harder to follow, rather than having some minor duplication in the test methods. Not a huge problem.
| 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); | ||
| } | ||
| }; | ||
| } |
There was a problem hiding this comment.
Let's use a concrete class instead of an anonymous class:
| 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); | |
| } | |
| } |
| } | ||
|
|
||
| @Test | ||
| void testGetDescription() { |
There was a problem hiding this comment.
remote test prefix on all these
| } | ||
|
|
||
| @Test | ||
| void testRecordAndSampleSamplingDecision() { |
There was a problem hiding this comment.
Can use a org.junit.jupiter.params.ParameterizedTest test to improve readability.
jack-berg
left a comment
jack-berg
left a comment
There was a problem hiding this comment.
Couple small things that need adjusting. Thanks for working on this!
3 participants
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 checkas per CONTRIBUTING.md guide