Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add probability to decision when appropriate #1116

Merged
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion sdk/src/main/java/io/opentelemetry/sdk/trace/Sampler.java
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,6 @@ interface Decision {
* span or when sampling decision {@link #isSampled()} changes from false to true.
* @since 0.1.0
*/
Map<String, AttributeValue> attributes();
Map<String, AttributeValue> getAttributes();
}
}
46 changes: 42 additions & 4 deletions sdk/src/main/java/io/opentelemetry/sdk/trace/Samplers.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -162,13 +165,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,
Expand Down Expand Up @@ -198,8 +209,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
Expand Down Expand Up @@ -229,8 +240,35 @@ public boolean isSampled() {
}

@Override
public Map<String, AttributeValue> attributes() {
public Map<String, AttributeValue> getAttributes() {
return Collections.emptyMap();
}
}

/** 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(
jkwatson marked this conversation as resolved.
Show resolved Hide resolved
SamplingAttributes.SAMPLING_PROBABILITY.key(), doubleAttributeValue(probability)));
}

@Override
public abstract boolean isSampled();

@Override
public abstract Map<String, AttributeValue> getAttributes();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
/*
* Copyright 2020, OpenTelemetry Authors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package io.opentelemetry.sdk.trace;

import io.opentelemetry.trace.attributes.DoubleAttributeSetter;

/** Attributes that are applied by {@link Sampler} instances. */
public class SamplingAttributes {

/**
* Probability value used by a probability-based Span sampling strategy.
*
* <p>Note: This will need to be updated if a specification for this value is merged which changes
* this proposed value.
*
* <p>See https://github.com/open-telemetry/opentelemetry-specification/pull/570
*/
public static final DoubleAttributeSetter SAMPLING_PROBABILITY =
DoubleAttributeSetter.create("sampling.probability");
Copy link
Member

Choose a reason for hiding this comment

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

As I mentioned in a previous comment, I would make this internal in the Samplers until a decision in the specs is made. I think this was the confusion from the other thread, but would not expose it for the moment, my comment there was that if we decide to expose it will definitely not going to be in the API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh yeah. I somehow spaced on that part of things. I'll move it in there for the nonce.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, please do it before merging :)

Copy link
Member

Choose a reason for hiding this comment

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

Also good to wait for @Oberon00 because they requested changes.

Copy link
Contributor Author

@jkwatson jkwatson Apr 20, 2020

Choose a reason for hiding this comment

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

moved into the Samplers, near the usage.


private SamplingAttributes() {}
}
Original file line number Diff line number Diff line change
Expand Up @@ -230,7 +230,7 @@ public Span startSpan() {
return DefaultSpan.create(spanContext);
}

attributes.putAllAttributes(samplingDecision.attributes());
attributes.putAllAttributes(samplingDecision.getAttributes());

return RecordEventsReadableSpan.startSpan(
spanContext,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -267,7 +268,9 @@ public void probabilitySampler_SampleBasedOnTraceId() {
Collections.<String, AttributeValue>emptyMap(),
Collections.<Link>emptyList());
assertThat(decision1.isSampled()).isFalse();
assertThat(decision1.attributes()).isEmpty();
assertThat(decision1.getAttributes())
.containsExactly(
SamplingAttributes.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 =
Expand Down Expand Up @@ -301,6 +304,8 @@ public void probabilitySampler_SampleBasedOnTraceId() {
Collections.<String, AttributeValue>emptyMap(),
Collections.<Link>emptyList());
assertThat(decision2.isSampled()).isTrue();
assertThat(decision2.attributes()).isEmpty();
assertThat(decision1.getAttributes())
.containsExactly(
SamplingAttributes.SAMPLING_PROBABILITY.key(), doubleAttributeValue(0.0001));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -305,7 +305,7 @@ public boolean isSampled() {
}

@Override
public Map<String, AttributeValue> attributes() {
public Map<String, AttributeValue> getAttributes() {
Map<String, AttributeValue> attributes = new LinkedHashMap<>();
attributes.put(
samplerAttributeName, AttributeValue.stringAttributeValue("bar"));
Expand Down