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.

Sign up for GitHub

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

Jump to bottom

Support traceId-based r-values #417

Merged
merged 7 commits into from
Aug 19, 2022
Merged

Support traceId-based r-values #417

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a0a57e4
Support traceId-based r-values
trask Aug 12, 2022
d0d7dbd
Interface
trask Aug 12, 2022
49d8714
Fix javadoc
trask Aug 12, 2022
5259ee6
Spotless
trask Aug 12, 2022
3302ee2
More
trask Aug 12, 2022
71c5a32
Clean
trask Aug 12, 2022
a5b569c
Review
trask Aug 18, 2022
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
4 changes: 3 additions & 1 deletion ...-sampling/src/main/java/io/opentelemetry/contrib/samplers/ConsistentAlwaysOffSampler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,9 @@
@Immutable
final class ConsistentAlwaysOffSampler extends ConsistentSampler {

private ConsistentAlwaysOffSampler() {}
private ConsistentAlwaysOffSampler() {
super(s -> RandomGenerator.getDefault().numberOfLeadingZerosOfRandomLong());
trask marked this conversation as resolved.
Show resolved Hide resolved
}

private static final ConsistentSampler INSTANCE = new ConsistentAlwaysOffSampler();

Expand Down
4 changes: 3 additions & 1 deletion ...t-sampling/src/main/java/io/opentelemetry/contrib/samplers/ConsistentAlwaysOnSampler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,9 @@
@Immutable
final class ConsistentAlwaysOnSampler extends ConsistentSampler {

private ConsistentAlwaysOnSampler() {}
private ConsistentAlwaysOnSampler() {
super(s -> RandomGenerator.getDefault().numberOfLeadingZerosOfRandomLong());
}

private static final ConsistentSampler INSTANCE = new ConsistentAlwaysOnSampler();

Expand Down
1 change: 1 addition & 0 deletions ...ampling/src/main/java/io/opentelemetry/contrib/samplers/ConsistentComposedAndSampler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ final class ConsistentComposedAndSampler extends ConsistentSampler {
private final String description;

ConsistentComposedAndSampler(ConsistentSampler sampler1, ConsistentSampler sampler2) {
super(s -> RandomGenerator.getDefault().numberOfLeadingZerosOfRandomLong());
this.sampler1 = requireNonNull(sampler1);
this.sampler2 = requireNonNull(sampler2);
this.description =
Expand Down
1 change: 1 addition & 0 deletions ...sampling/src/main/java/io/opentelemetry/contrib/samplers/ConsistentComposedOrSampler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ final class ConsistentComposedOrSampler extends ConsistentSampler {
private final String description;

ConsistentComposedOrSampler(ConsistentSampler sampler1, ConsistentSampler sampler2) {
super(s -> RandomGenerator.getDefault().numberOfLeadingZerosOfRandomLong());
this.sampler1 = requireNonNull(sampler1);
this.sampler2 = requireNonNull(sampler2);
this.description =
Expand Down
16 changes: 4 additions & 12 deletions ...ampling/src/main/java/io/opentelemetry/contrib/samplers/ConsistentParentBasedSampler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

import static java.util.Objects.requireNonNull;

import java.util.function.ToIntFunction;
import javax.annotation.concurrent.Immutable;

/**
Expand All @@ -21,25 +22,16 @@ final class ConsistentParentBasedSampler extends ConsistentSampler {

private final String description;

/**
* Constructs a new consistent parent based sampler using the given root sampler.
*
* @param rootSampler the root sampler
*/
ConsistentParentBasedSampler(ConsistentSampler rootSampler) {
this(rootSampler, RandomGenerator.getDefault());
}

/**
* Constructs a new consistent parent based sampler using the given root sampler and the given
* thread-safe random generator.
*
* @param rootSampler the root sampler
* @param threadSafeRandomGenerator a thread-safe random generator
* @param rValueGenerator the function to use for generating the r-value
*/
ConsistentParentBasedSampler(
ConsistentSampler rootSampler, RandomGenerator threadSafeRandomGenerator) {
super(threadSafeRandomGenerator);
ConsistentSampler rootSampler, ToIntFunction<String> rValueGenerator) {
super(rValueGenerator);
this.rootSampler = requireNonNull(rootSampler);
this.description =
"ConsistentParentBasedSampler{rootSampler=" + rootSampler.getDescription() + '}';
Expand Down
21 changes: 9 additions & 12 deletions ...ng/src/main/java/io/opentelemetry/contrib/samplers/ConsistentProbabilityBasedSampler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

package io.opentelemetry.contrib.samplers;

import java.util.function.ToIntFunction;
import javax.annotation.concurrent.Immutable;

/** A consistent sampler that samples with a fixed probability. */
Expand All @@ -15,29 +16,25 @@ final class ConsistentProbabilityBasedSampler extends ConsistentSampler {
private final int upperPValue;
private final double probabilityToUseLowerPValue;
private final String description;
private final RandomGenerator randomGenerator;

/**
* Constructor.
*
* @param samplingProbability the sampling probability
* @param rValueGenerator the function to use for generating the r-value
*/
ConsistentProbabilityBasedSampler(double samplingProbability) {
this(samplingProbability, RandomGenerator.getDefault());
}

/**
* Constructor.
*
* @param samplingProbability the sampling probability
* @param randomGenerator a random generator
*/
ConsistentProbabilityBasedSampler(double samplingProbability, RandomGenerator randomGenerator) {
super(randomGenerator);
ConsistentProbabilityBasedSampler(
double samplingProbability,
ToIntFunction<String> rValueGenerator,
RandomGenerator randomGenerator) {
super(rValueGenerator);
if (samplingProbability < 0.0 || samplingProbability > 1.0) {
throw new IllegalArgumentException("Sampling probability must be in range [0.0, 1.0]!");
}
this.description =
String.format("ConsistentProbabilityBasedSampler{%.6f}", samplingProbability);
this.randomGenerator = randomGenerator;

lowerPValue = getLowerBoundP(samplingProbability);
upperPValue = getUpperBoundP(samplingProbability);
Expand Down
23 changes: 7 additions & 16 deletions ...mpling/src/main/java/io/opentelemetry/contrib/samplers/ConsistentRateLimitingSampler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import io.opentelemetry.sdk.trace.samplers.Sampler;
import java.util.concurrent.atomic.AtomicReference;
import java.util.function.LongSupplier;
import java.util.function.ToIntFunction;
import javax.annotation.concurrent.Immutable;

/**
Expand Down Expand Up @@ -85,37 +86,25 @@ public State(double effectiveWindowCount, double effectiveWindowNanos, long last
private final double inverseAdaptationTimeNanos;
private final double targetSpansPerNanosecondLimit;
private final AtomicReference<State> state;
private final RandomGenerator randomGenerator;

/**
* Constructor.
*
* @param targetSpansPerSecondLimit the desired spans per second limit
* @param adaptationTimeSeconds the typical time to adapt to a new load (time constant used for
* exponential smoothing)
*/
ConsistentRateLimitingSampler(double targetSpansPerSecondLimit, double adaptationTimeSeconds) {
this(
targetSpansPerSecondLimit,
adaptationTimeSeconds,
RandomGenerator.getDefault(),
System::nanoTime);
}

/**
* Constructor.
*
* @param targetSpansPerSecondLimit the desired spans per second limit
* @param adaptationTimeSeconds the typical time to adapt to a new load (time constant used for
* exponential smoothing)
* @param rValueGenerator the function to use for generating the r-value
* @param randomGenerator a random generator
* @param nanoTimeSupplier a supplier for the current nano time
*/
ConsistentRateLimitingSampler(
double targetSpansPerSecondLimit,
double adaptationTimeSeconds,
ToIntFunction<String> rValueGenerator,
RandomGenerator randomGenerator,
LongSupplier nanoTimeSupplier) {
super(randomGenerator);
super(rValueGenerator);

if (targetSpansPerSecondLimit < 0.0) {
throw new IllegalArgumentException("Limit for sampled spans per second must be nonnegative!");
Expand All @@ -133,6 +122,8 @@ public State(double effectiveWindowCount, double effectiveWindowNanos, long last
this.targetSpansPerNanosecondLimit = 1e-9 * targetSpansPerSecondLimit;

this.state = new AtomicReference<>(new State(0, 0, nanoTimeSupplier.getAsLong()));

this.randomGenerator = randomGenerator;
}

private State updateState(State oldState, long currentNanoTime) {
Expand Down
79 changes: 53 additions & 26 deletions consistent-sampling/src/main/java/io/opentelemetry/contrib/samplers/ConsistentSampler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import io.opentelemetry.sdk.trace.samplers.SamplingResult;
import java.util.List;
import java.util.function.LongSupplier;
import java.util.function.ToIntFunction;

/** Abstract base class for consistent samplers. */
public abstract class ConsistentSampler implements Sampler {
Expand All @@ -28,7 +29,7 @@ public abstract class ConsistentSampler implements Sampler {
*
* @return a sampler
*/
public static final ConsistentSampler alwaysOn() {
public static ConsistentSampler alwaysOn() {
return ConsistentAlwaysOnSampler.getInstance();
}

Expand All @@ -37,7 +38,7 @@ public static final ConsistentSampler alwaysOn() {
*
* @return a sampler
*/
public static final ConsistentSampler alwaysOff() {
public static ConsistentSampler alwaysOff() {
return ConsistentAlwaysOffSampler.getInstance();
}

Expand All @@ -47,8 +48,25 @@ public static final ConsistentSampler alwaysOff() {
* @param samplingProbability the sampling probability
* @return a sampler
*/
public static final ConsistentSampler probabilityBased(double samplingProbability) {
return new ConsistentProbabilityBasedSampler(samplingProbability);
public static ConsistentSampler probabilityBased(double samplingProbability) {
RandomGenerator randomGenerator = RandomGenerator.getDefault();
return new ConsistentProbabilityBasedSampler(
samplingProbability,
s -> randomGenerator.numberOfLeadingZerosOfRandomLong(),
randomGenerator);
}

/**
* Returns a {@link ConsistentSampler} that samples each span with a fixed probability.
*
* @param samplingProbability the sampling probability
* @param rValueGenerator the function to use for generating the r-value
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably needs some more documentation. The rValueGenerator must map a trace ID to a value in range [0,62] with probabilities as specified in https://opentelemetry.io/docs/reference/specification/trace/tracestate-probability-sampling/#appendix. I would prefer a new functional interface instead of ToIntFunction<String> where all this can be documented.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree. A new interface could be also used to define the default behavior "s -> RandomGenerator.getDefault().numberOfLeadingZerosOfRandomLong()" and avoid repeating this code phrase in multiple places.

Copy link
Member Author

Choose a reason for hiding this comment

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

Great ideas, implemented. I put the default behavior in a separate class RValueGenerators to keep it package-private.

* @return a sampler
*/
public static ConsistentSampler probabilityBased(
double samplingProbability, ToIntFunction<String> rValueGenerator) {
return new ConsistentProbabilityBasedSampler(
samplingProbability, rValueGenerator, RandomGenerator.getDefault());
}

/**
Expand All @@ -58,9 +76,12 @@ public static final ConsistentSampler probabilityBased(double samplingProbabilit
* @param randomGenerator a random generator
* @return a sampler
*/
static final ConsistentSampler probabilityBased(
static ConsistentSampler probabilityBased(
double samplingProbability, RandomGenerator randomGenerator) {
return new ConsistentProbabilityBasedSampler(samplingProbability, randomGenerator);
return new ConsistentProbabilityBasedSampler(
samplingProbability,
s -> randomGenerator.numberOfLeadingZerosOfRandomLong(),
randomGenerator);
}

/**
Expand All @@ -69,20 +90,21 @@ static final ConsistentSampler probabilityBased(
*
* @param rootSampler the root sampler
*/
public static final ConsistentSampler parentBased(ConsistentSampler rootSampler) {
return new ConsistentParentBasedSampler(rootSampler);
public static ConsistentSampler parentBased(ConsistentSampler rootSampler) {
return parentBased(
rootSampler, s -> RandomGenerator.getDefault().numberOfLeadingZerosOfRandomLong());
}

/**
* Returns a new {@link ConsistentSampler} that respects the sampling decision of the parent span
* or falls-back to the given sampler if it is a root span.
*
* @param rootSampler the root sampler
* @param randomGenerator a random generator
* @param rValueGenerator the function to use for generating the r-value
*/
static final ConsistentSampler parentBased(
ConsistentSampler rootSampler, RandomGenerator randomGenerator) {
return new ConsistentParentBasedSampler(rootSampler, randomGenerator);
static ConsistentSampler parentBased(
trask marked this conversation as resolved.
Show resolved Hide resolved
ConsistentSampler rootSampler, ToIntFunction<String> rValueGenerator) {
return new ConsistentParentBasedSampler(rootSampler, rValueGenerator);
}

/**
Expand All @@ -93,9 +115,15 @@ static final ConsistentSampler parentBased(
* @param adaptationTimeSeconds the typical time to adapt to a new load (time constant used for
* exponential smoothing)
*/
public static final ConsistentSampler rateLimited(
public static ConsistentSampler rateLimited(
double targetSpansPerSecondLimit, double adaptationTimeSeconds) {
return new ConsistentRateLimitingSampler(targetSpansPerSecondLimit, adaptationTimeSeconds);
RandomGenerator randomGenerator = RandomGenerator.getDefault();
return new ConsistentRateLimitingSampler(
targetSpansPerSecondLimit,
adaptationTimeSeconds,
s -> randomGenerator.numberOfLeadingZerosOfRandomLong(),
randomGenerator,
System::nanoTime);
}

/**
Expand All @@ -108,13 +136,17 @@ public static final ConsistentSampler rateLimited(
* @param randomGenerator a random generator
* @param nanoTimeSupplier a supplier for the current nano time
*/
static final ConsistentSampler rateLimited(
static ConsistentSampler rateLimited(
double targetSpansPerSecondLimit,
double adaptationTimeSeconds,
RandomGenerator randomGenerator,
LongSupplier nanoTimeSupplier) {
return new ConsistentRateLimitingSampler(
targetSpansPerSecondLimit, adaptationTimeSeconds, randomGenerator, nanoTimeSupplier);
targetSpansPerSecondLimit,
adaptationTimeSeconds,
s -> randomGenerator.numberOfLeadingZerosOfRandomLong(),
RandomGenerator.getDefault(),
nanoTimeSupplier);
}

/**
Expand Down Expand Up @@ -161,17 +193,13 @@ public ConsistentSampler or(ConsistentSampler otherConsistentSampler) {
return new ConsistentComposedOrSampler(this, otherConsistentSampler);
}

protected final RandomGenerator randomGenerator;

protected ConsistentSampler(RandomGenerator randomGenerator) {
this.randomGenerator = requireNonNull(randomGenerator);
}
private final ToIntFunction<String> rValueGenerator;

protected ConsistentSampler() {
this(RandomGenerator.getDefault());
protected ConsistentSampler(ToIntFunction<String> rValueGenerator) {
this.rValueGenerator = requireNonNull(rValueGenerator);
}

private static final boolean isInvariantViolated(
private static boolean isInvariantViolated(
OtelTraceState otelTraceState, boolean isParentSampled) {
if (otelTraceState.hasValidR() && otelTraceState.hasValidP()) {
// if valid p- and r-values are given, they must be consistent with the isParentSampled flag
Expand Down Expand Up @@ -212,8 +240,7 @@ public final SamplingResult shouldSample(

// generate new r-value if not available
if (!otelTraceState.hasValidR()) {
otelTraceState.setR(
Math.min(randomGenerator.numberOfLeadingZerosOfRandomLong(), OtelTraceState.getMaxR()));
otelTraceState.setR(Math.min(rValueGenerator.applyAsInt(traceId), OtelTraceState.getMaxR()));
Copy link
Contributor

@PeterF778 PeterF778 Aug 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I'd love to see this approach made more general with rValueGenerator taking not only traceId, but also the Attributes as arguments.
The use case for this is to assign the same rValue to a group of traces, so they are likely to survive sampling as an intact group - while still ensuring the correct statistical distribution of the rValues over the whole trace population.

}

// determine and set new p-value that is used for the sampling decision
Expand Down
3 changes: 2 additions & 1 deletion ...rc/test/java/io/opentelemetry/contrib/samplers/ConsistentProbabilityBasedSamplerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,8 @@ private void test(SplittableRandom rng, double samplingProbability) {

Sampler sampler =
ConsistentSampler.probabilityBased(
samplingProbability, RandomGenerator.create(rng::nextLong));
samplingProbability,
s -> RandomGenerator.create(rng::nextLong).numberOfLeadingZerosOfRandomLong());

Map<Integer, Long> observedPvalues = new HashMap<>();
for (long i = 0; i < numSpans; ++i) {
Expand Down
3 changes: 2 additions & 1 deletion ...stent-sampling/src/test/java/io/opentelemetry/contrib/samplers/ConsistentSamplerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,8 @@ void testRandomValues() {
private static ConsistentSampler createConsistentSampler(int p, int r) {
long randomLong = ~(0xFFFFFFFFFFFFFFFFL << r);

return new ConsistentSampler(RandomGenerator.create(() -> randomLong)) {
return new ConsistentSampler(
s -> RandomGenerator.create(() -> randomLong).numberOfLeadingZerosOfRandomLong()) {
@Override
public String getDescription() {
throw new UnsupportedOperationException();
Expand Down