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

Composite Samplers prototype #1443

Merged
merged 18 commits into from
Sep 16, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
f242459
Composite Samplers prototype
PeterF778 Aug 30, 2024
23690a4
Refactoring.
PeterF778 Aug 30, 2024
a8443c4
Refactoring, adding a new test case based on https://github.com/open-…
PeterF778 Sep 3, 2024
9b53416
Update consistent-sampling/src/main/java/io/opentelemetry/contrib/sam…
PeterF778 Sep 5, 2024
b3b57ac
Update consistent-sampling/src/main/java/io/opentelemetry/contrib/sam…
PeterF778 Sep 5, 2024
7c13e4a
Update consistent-sampling/src/main/java/io/opentelemetry/contrib/sam…
PeterF778 Sep 5, 2024
3308d4a
Update consistent-sampling/src/main/java/io/opentelemetry/contrib/sam…
PeterF778 Sep 5, 2024
49fd364
Update consistent-sampling/src/test/java/io/opentelemetry/contrib/sam…
PeterF778 Sep 5, 2024
7db19ea
Update consistent-sampling/src/main/java/io/opentelemetry/contrib/sam…
PeterF778 Sep 5, 2024
75201a9
Update consistent-sampling/src/main/java/io/opentelemetry/contrib/sam…
PeterF778 Sep 5, 2024
e017c45
Update consistent-sampling/src/main/java/io/opentelemetry/contrib/sam…
PeterF778 Sep 5, 2024
02c3411
Update consistent-sampling/src/main/java/io/opentelemetry/contrib/sam…
PeterF778 Sep 5, 2024
66cf1d7
Update consistent-sampling/src/test/java/io/opentelemetry/contrib/sam…
PeterF778 Sep 5, 2024
9a21ab1
Update consistent-sampling/src/main/java/io/opentelemetry/contrib/sam…
PeterF778 Sep 5, 2024
1d0ba27
Update consistent-sampling/src/main/java/io/opentelemetry/contrib/sam…
PeterF778 Sep 5, 2024
eb66327
Addressing code review suggestions.
PeterF778 Sep 5, 2024
03104e7
Generalizing CoinFlipSampler.
PeterF778 Sep 6, 2024
17aa757
Adding a test for low-resolution clock.
PeterF778 Sep 10, 2024
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.contrib.sampler.consistent56;

import io.opentelemetry.api.common.Attributes;
import io.opentelemetry.api.trace.SpanKind;
import io.opentelemetry.context.Context;
import io.opentelemetry.sdk.trace.data.LinkData;
import java.util.List;

/** An interface for components to be used by composite consistent probability samplers. */
public interface ComposableSampler {

/**
* Returns the SamplingIntent that is used for the sampling decision. The SamplingIntent includes
* the threshold value which will be used for the sampling decision.
*
* <p>NOTE: Keep in mind, that in any case the returned threshold value must not depend directly
* or indirectly on the random value. In particular this means that the parent sampled flag must
* not be used for the calculation of the threshold as the sampled flag depends itself on the
* random value.
*/
SamplingIntent getSamplingIntent(
Context parentContext,
String name,
SpanKind spanKind,
Attributes attributes,
List<LinkData> parentLinks);

/** Return the string providing a description of the implementation */
PeterF778 marked this conversation as resolved.
Show resolved Hide resolved
String getDescription();
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,13 @@

package io.opentelemetry.contrib.sampler.consistent56;

import static io.opentelemetry.contrib.sampler.consistent56.ConsistentSamplingUtil.getInvalidThreshold;

import io.opentelemetry.api.common.Attributes;
import io.opentelemetry.api.trace.SpanKind;
import io.opentelemetry.context.Context;
import io.opentelemetry.sdk.trace.data.LinkData;
import java.util.List;
import javax.annotation.concurrent.Immutable;

@Immutable
Expand All @@ -19,8 +26,14 @@ static ConsistentAlwaysOffSampler getInstance() {
}

@Override
protected long getThreshold(long parentThreshold, boolean isRoot) {
return ConsistentSamplingUtil.getMaxThreshold();
public SamplingIntent getSamplingIntent(
Context parentContext,
String name,
SpanKind spanKind,
Attributes attributes,
List<LinkData> parentLinks) {

return () -> getInvalidThreshold();
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,11 @@

import static io.opentelemetry.contrib.sampler.consistent56.ConsistentSamplingUtil.getMinThreshold;

import io.opentelemetry.api.common.Attributes;
import io.opentelemetry.api.trace.SpanKind;
import io.opentelemetry.context.Context;
import io.opentelemetry.sdk.trace.data.LinkData;
import java.util.List;
import javax.annotation.concurrent.Immutable;

@Immutable
Expand All @@ -21,8 +26,14 @@ static ConsistentAlwaysOnSampler getInstance() {
}

@Override
protected long getThreshold(long parentThreshold, boolean isRoot) {
return getMinThreshold();
public SamplingIntent getSamplingIntent(
Context parentContext,
String name,
SpanKind spanKind,
Attributes attributes,
List<LinkData> parentLinks) {

return () -> getMinThreshold();
}

@Override
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.contrib.sampler.consistent56;

import static io.opentelemetry.contrib.sampler.consistent56.ConsistentSamplingUtil.getInvalidThreshold;
import static io.opentelemetry.contrib.sampler.consistent56.ConsistentSamplingUtil.isValidThreshold;

import io.opentelemetry.api.common.Attributes;
import io.opentelemetry.api.common.AttributesBuilder;
import io.opentelemetry.api.trace.SpanKind;
import io.opentelemetry.api.trace.TraceState;
import io.opentelemetry.context.Context;
import io.opentelemetry.sdk.trace.data.LinkData;
import java.util.List;
import javax.annotation.concurrent.Immutable;

/**
* A consistent sampler that queries all its delegate samplers for their sampling threshold, and
* uses the minimum threshold value received.
*/
@Immutable
final class ConsistentAnyOf extends ConsistentSampler {

private final ComposableSampler[] delegates;

private final String description;

/**
* Constructs a new consistent AnyOf sampler using the provided delegate samplers.
*
* @param delegates the delegate samplers
*/
ConsistentAnyOf(ComposableSampler... delegates) {
PeterF778 marked this conversation as resolved.
Show resolved Hide resolved
if (delegates.length == 0) {
PeterF778 marked this conversation as resolved.
Show resolved Hide resolved
throw new IllegalArgumentException(
"At least one delegate must be specified for ConsistentAnyOf");
}

this.delegates = delegates;

StringBuilder builder = new StringBuilder("ConsistentAnyOf{");
for (int i = 0; i < delegates.length; ++i) {
builder.append(delegates[i].getDescription());
if (i < delegates.length - 1) {
builder.append(",");
}
}
builder.append("}");
this.description = builder.toString();
PeterF778 marked this conversation as resolved.
Show resolved Hide resolved
}

@Override
public SamplingIntent getSamplingIntent(
Context parentContext,
String name,
SpanKind spanKind,
Attributes attributes,
List<LinkData> parentLinks) {

SamplingIntent[] intents = new SamplingIntent[delegates.length];
int k = 0;
long minimumThreshold = getInvalidThreshold();
for (ComposableSampler delegate : delegates) {
SamplingIntent delegateIntent =
delegate.getSamplingIntent(parentContext, name, spanKind, attributes, parentLinks);
long delegateThreshold = delegateIntent.getThreshold();
if (isValidThreshold(delegateThreshold)) {
if (isValidThreshold(minimumThreshold)) {
minimumThreshold = Math.min(delegateThreshold, minimumThreshold);
} else {
minimumThreshold = delegateThreshold;
}
}
intents[k++] = delegateIntent;
}

long resultingThreshold = minimumThreshold;

return new SamplingIntent() {
@Override
public long getThreshold() {
return resultingThreshold;
}

@Override
public Attributes getAttributes() {
AttributesBuilder builder = Attributes.builder();
for (SamplingIntent intent : intents) {
builder = builder.putAll(intent.getAttributes());
}
return builder.build();
}

@Override
public TraceState updateTraceState(TraceState previousState) {
for (SamplingIntent intent : intents) {
previousState = intent.updateTraceState(previousState);
}
return previousState;
}
};
}

@Override
public String getDescription() {
return description;
}
}

This file was deleted.

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,14 @@

import static io.opentelemetry.contrib.sampler.consistent56.ConsistentSamplingUtil.calculateSamplingProbability;
import static io.opentelemetry.contrib.sampler.consistent56.ConsistentSamplingUtil.checkThreshold;
import static io.opentelemetry.contrib.sampler.consistent56.ConsistentSamplingUtil.getInvalidThreshold;
import static io.opentelemetry.contrib.sampler.consistent56.ConsistentSamplingUtil.getMaxThreshold;

import io.opentelemetry.api.common.Attributes;
import io.opentelemetry.api.trace.SpanKind;
import io.opentelemetry.context.Context;
import io.opentelemetry.sdk.trace.data.LinkData;
import java.util.List;

public class ConsistentFixedThresholdSampler extends ConsistentSampler {
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks and feels like what the OTel Trace SDK TraceIdRatioBased sampler wants to be. 👍


Expand All @@ -18,7 +26,7 @@ protected ConsistentFixedThresholdSampler(long threshold) {
this.threshold = threshold;

String thresholdString;
if (threshold == ConsistentSamplingUtil.getMaxThreshold()) {
if (threshold == getMaxThreshold()) {
thresholdString = "max";
} else {
thresholdString =
Expand All @@ -41,7 +49,18 @@ public String getDescription() {
}

@Override
protected long getThreshold(long parentThreshold, boolean isRoot) {
return threshold;
public SamplingIntent getSamplingIntent(
Context parentContext,
String name,
SpanKind spanKind,
Attributes attributes,
List<LinkData> parentLinks) {

return () -> {
if (threshold == getMaxThreshold()) {
return getInvalidThreshold();
}
Comment on lines +60 to +62
Copy link
Contributor

Choose a reason for hiding this comment

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

Please comment, I had to go review https://github.com/open-telemetry/opentelemetry-java-contrib/blob/main/consistent-sampling/src/main/java/io/opentelemetry/contrib/sampler/consistent56/ConsistentSamplingUtil.java to understand that getMaxThreshold() is what I was expecting getInvalidThreshold() to be.

Why does the Util package distinguish Max and Invalid? (Why aren't they a single value, maybe?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This probably requires a cleanup. The prototype currently uses long rather than hex String to represent the thresholds, but the proposed specs allow both implementations. I'd love to see the same logic and behavior for all the new samplers regardless of the choice of the threshold representation. However, the issue is with the MaxThreshold, which is really an out-of-range value. Representing the maximum threshold as a hex string is not useful, as it would be a 15-character string and it would break lexicographical comparisons.
Therefore I opted into requiring that getSamplingIntent always provides a valid threshold (which is smaller than 2^56), or an invalid one, which is a special value easy to test for.

return threshold;
};
}
}
Loading
Loading