From 4d62068b522872f19a4f90fd4b665d682a888e53 Mon Sep 17 00:00:00 2001 From: John Watson Date: Wed, 22 Apr 2020 16:32:43 -0700 Subject: [PATCH] Add probability to decision when appropriate (#1116) * add the sampling probability to the decision, when appropriate. * formatting * normalize a method name and move attribute to semantic convention attributes * move the sampling attribute to its own class in the SDK * Move the sampling priority attribute to a non-public spot --- .../io/opentelemetry/sdk/trace/Sampler.java | 2 +- .../io/opentelemetry/sdk/trace/Samplers.java | 57 +++++++++++++++++-- .../sdk/trace/SpanBuilderSdk.java | 2 +- .../opentelemetry/sdk/trace/SamplersTest.java | 7 ++- .../sdk/trace/SpanBuilderSdkTest.java | 2 +- 5 files changed, 61 insertions(+), 9 deletions(-) diff --git a/sdk/src/main/java/io/opentelemetry/sdk/trace/Sampler.java b/sdk/src/main/java/io/opentelemetry/sdk/trace/Sampler.java index be862472e72..be0d7fab2c5 100644 --- a/sdk/src/main/java/io/opentelemetry/sdk/trace/Sampler.java +++ b/sdk/src/main/java/io/opentelemetry/sdk/trace/Sampler.java @@ -92,6 +92,6 @@ interface Decision { * span or when sampling decision {@link #isSampled()} changes from false to true. * @since 0.1.0 */ - Map attributes(); + Map getAttributes(); } } diff --git a/sdk/src/main/java/io/opentelemetry/sdk/trace/Samplers.java b/sdk/src/main/java/io/opentelemetry/sdk/trace/Samplers.java index f9f77bb6782..a0c7c2cdf7a 100644 --- a/sdk/src/main/java/io/opentelemetry/sdk/trace/Samplers.java +++ b/sdk/src/main/java/io/opentelemetry/sdk/trace/Samplers.java @@ -16,6 +16,9 @@ package io.opentelemetry.sdk.trace; +import static io.opentelemetry.common.AttributeValue.doubleAttributeValue; +import static java.util.Collections.singletonMap; + import com.google.auto.value.AutoValue; import com.google.common.base.Preconditions; import io.opentelemetry.common.AttributeValue; @@ -25,6 +28,7 @@ import io.opentelemetry.trace.SpanContext; import io.opentelemetry.trace.SpanId; import io.opentelemetry.trace.TraceId; +import io.opentelemetry.trace.attributes.DoubleAttributeSetter; import java.util.Collections; import java.util.List; import java.util.Map; @@ -38,6 +42,7 @@ */ @Immutable public final class Samplers { + private static final Sampler ALWAYS_ON = new AlwaysOnSampler(); private static final Sampler ALWAYS_OFF = new AlwaysOffSampler(); private static final Decision ALWAYS_ON_DECISION = new SimpleDecision(/* decision= */ true); @@ -162,13 +167,21 @@ static Probability create(double probability) { } else { idUpperBound = (long) (probability * Long.MAX_VALUE); } - return new AutoValue_Samplers_Probability(probability, idUpperBound); + return new AutoValue_Samplers_Probability( + probability, + idUpperBound, + ProbabilityDecision.create(/* decision= */ true, probability), + ProbabilityDecision.create(/* decision= */ false, probability)); } abstract double getProbability(); abstract long getIdUpperBound(); + abstract Decision getPositiveDecision(); + + abstract Decision getNegativeDecision(); + @Override public final Decision shouldSample( @Nullable SpanContext parentContext, @@ -198,8 +211,8 @@ public final Decision shouldSample( // This is considered a reasonable tradeoff for the simplicity/performance requirements (this // code is executed in-line for every Span creation). return Math.abs(traceId.getLowerLong()) < getIdUpperBound() - ? ALWAYS_ON_DECISION - : ALWAYS_OFF_DECISION; + ? getPositiveDecision() + : getNegativeDecision(); } @Override @@ -229,8 +242,44 @@ public boolean isSampled() { } @Override - public Map attributes() { + public Map getAttributes() { return Collections.emptyMap(); } } + + /** + * Probability value used by a probability-based Span sampling strategy. + * + *

Note: This will need to be updated if a specification for this value is merged which changes + * this proposed value. Also, once it's in the spec, we should move it somewhere more visible. + * + *

See https://github.com/open-telemetry/opentelemetry-specification/pull/570 + */ + static final DoubleAttributeSetter SAMPLING_PROBABILITY = + DoubleAttributeSetter.create("sampling.probability"); + + /** Probability-based sampling decision with a single attribute for the probability. */ + @Immutable + @AutoValue + abstract static class ProbabilityDecision implements Decision { + + ProbabilityDecision() {} + + /** + * Creates sampling decision without attributes. + * + * @param decision sampling decision + * @param probability the probability that was used for the decision. + */ + static ProbabilityDecision create(boolean decision, double probability) { + return new AutoValue_Samplers_ProbabilityDecision( + decision, singletonMap(SAMPLING_PROBABILITY.key(), doubleAttributeValue(probability))); + } + + @Override + public abstract boolean isSampled(); + + @Override + public abstract Map getAttributes(); + } } diff --git a/sdk/src/main/java/io/opentelemetry/sdk/trace/SpanBuilderSdk.java b/sdk/src/main/java/io/opentelemetry/sdk/trace/SpanBuilderSdk.java index 43c3dad54ec..f1e484b0926 100644 --- a/sdk/src/main/java/io/opentelemetry/sdk/trace/SpanBuilderSdk.java +++ b/sdk/src/main/java/io/opentelemetry/sdk/trace/SpanBuilderSdk.java @@ -217,7 +217,7 @@ public Span startSpan() { return DefaultSpan.create(spanContext); } - attributes.putAllAttributes(samplingDecision.attributes()); + attributes.putAllAttributes(samplingDecision.getAttributes()); return RecordEventsReadableSpan.startSpan( spanContext, diff --git a/sdk/src/test/java/io/opentelemetry/sdk/trace/SamplersTest.java b/sdk/src/test/java/io/opentelemetry/sdk/trace/SamplersTest.java index 21f53d0d30e..fb8c1812eaf 100644 --- a/sdk/src/test/java/io/opentelemetry/sdk/trace/SamplersTest.java +++ b/sdk/src/test/java/io/opentelemetry/sdk/trace/SamplersTest.java @@ -17,6 +17,7 @@ package io.opentelemetry.sdk.trace; import static com.google.common.truth.Truth.assertThat; +import static io.opentelemetry.common.AttributeValue.doubleAttributeValue; import com.google.common.truth.Truth; import io.opentelemetry.common.AttributeValue; @@ -267,7 +268,8 @@ public void probabilitySampler_SampleBasedOnTraceId() { Collections.emptyMap(), Collections.emptyList()); assertThat(decision1.isSampled()).isFalse(); - assertThat(decision1.attributes()).isEmpty(); + assertThat(decision1.getAttributes()) + .containsExactly(Samplers.SAMPLING_PROBABILITY.key(), doubleAttributeValue(0.0001)); // This traceId will be sampled by the Probability Sampler because the first 8 bytes as long // is less than probability * Long.MAX_VALUE; TraceId sampledtraceId = @@ -301,6 +303,7 @@ public void probabilitySampler_SampleBasedOnTraceId() { Collections.emptyMap(), Collections.emptyList()); assertThat(decision2.isSampled()).isTrue(); - assertThat(decision2.attributes()).isEmpty(); + assertThat(decision1.getAttributes()) + .containsExactly(Samplers.SAMPLING_PROBABILITY.key(), doubleAttributeValue(0.0001)); } } diff --git a/sdk/src/test/java/io/opentelemetry/sdk/trace/SpanBuilderSdkTest.java b/sdk/src/test/java/io/opentelemetry/sdk/trace/SpanBuilderSdkTest.java index 3b3b1430bd1..2a0be77a6bb 100644 --- a/sdk/src/test/java/io/opentelemetry/sdk/trace/SpanBuilderSdkTest.java +++ b/sdk/src/test/java/io/opentelemetry/sdk/trace/SpanBuilderSdkTest.java @@ -329,7 +329,7 @@ public boolean isSampled() { } @Override - public Map attributes() { + public Map getAttributes() { Map attributes = new LinkedHashMap<>(); attributes.put( samplerAttributeName, AttributeValue.stringAttributeValue("bar"));