Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
Original file line number Diff line number Diff line change
Expand Up @@ -5,43 +5,38 @@

package io.opentelemetry.contrib.dynamic.policy;

import com.fasterxml.jackson.databind.JsonNode;
import java.util.Objects;
import javax.annotation.Nullable;

/**
* Represents a single Telemetry Policy, comprising a type and a specification.
* Represents a single Telemetry Policy identified by type.
*
* <p>A {@code TelemetryPolicy} instance encapsulates a specific rule or configuration intent
* identified by its {@link #getType() type}. The behavior of the policy is defined by its {@link
* #getSpec() specification}, which is a JSON object.
* identified by its {@link #getType() type}.
*
* <p>Policies are typically immutable data carriers. The {@code spec} can be {@code null}, which
* conventionally signifies the removal or absence of a policy for the given type in certain
* contexts (e.g., when calculating diffs or updates).
* <p>Policies are immutable data carriers.
*
* <p>As an example take the JSON structure `{"trace-sampling": {"probability" : 0.5}}` This is of
* type "trace-sampling", with spec `{"probability" : 0.5}`, indicating the intent that the trace
* sampling-probability be set to 50%
* <p>As an example, policy type {@code trace-sampling} indicates that trace sampling behavior
* should be configured.
*
* <p>Direct instantiation of this base class is intentionally supported for type-only policy
* signals (for example, to indicate policy removal/reset without policy-specific values).
*
* @see io.opentelemetry.contrib.dynamic.policy
*/
public class TelemetryPolicy {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not a blocker, but I'm curious if it makes sense to turn this into an interface to make it more flexible/easier to extend? I don't have a specific use case in mind where more flexibility would be needed; it's just that the rules around dynamic support are changing so often that it might be useful to have such flexibility.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

for now its useful as a concrete class for resetting to default. I've considered having it abstract or as an interface but it seems most useful at the moment as a concrete class. My aim is to have a working extension that supports sampling rate changes, then add further functionality and see how it changes as needed. Since this is all experimental, letting it evolve as needed rather than trying to anticipate the end state, is the preferred way to go (I think)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If it's a public class, let's make it final so that users don't try and subclass it (they will!). 🤣

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

oh wait, that's apparently the intended use....oof. I don't love that.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Each policy will be a subclass of TelemetryPolicy. TelemetryPolicy itself could potentially become an abstract superclass or an interface, and we could switch to a "DeleteTelemetryPolicy" subclass/implementation to handle deletions. Please can we do that in a different PR if that's the way we want to go

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes?

private final String type; // e.g. "trace-sampling"
// JSON content after schema validation
// null means removed, which is relevant for merging policies
@Nullable private final JsonNode spec;

/**
* Constructs a new TelemetryPolicy.
*
* <p>This constructor is used by type-specific subclasses and also directly for type-only policy
* signals such as policy removal/reset.
*
* @param type the type of the policy (e.g., "trace-sampling"), must not be null.
* @param spec the JSON specification of the policy, or {@code null} to indicate removal.
*/
public TelemetryPolicy(String type, @Nullable JsonNode spec) {
public TelemetryPolicy(String type) {
Objects.requireNonNull(type, "type cannot be null");
this.type = type;
this.spec = spec;
}

/**
Expand All @@ -55,34 +50,4 @@ public TelemetryPolicy(String type, @Nullable JsonNode spec) {
public String getType() {
return type;
}

/**
* Returns the specification of this policy.
*
* <p>The specification is a JSON structure defining the parameters of the policy. If {@code
* null}, it may indicate that the policy is being removed or is empty.
*
* @return the policy specification, or {@code null}.
*/
@Nullable
public JsonNode getSpec() {
return spec;
}

@Override
public boolean equals(Object o) {
if (this == o) {
return true;
}
if (!(o instanceof TelemetryPolicy)) {
return false;
}
TelemetryPolicy that = (TelemetryPolicy) o;
return Objects.equals(type, that.type) && Objects.equals(spec, that.spec);
}

@Override
public int hashCode() {
return Objects.hash(type, spec);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.contrib.dynamic.policy;

public final class TraceSamplingRatePolicy extends TelemetryPolicy {
public static final String TYPE = "trace-sampling";

private final double probability;

public TraceSamplingRatePolicy(double probability) {
super(TYPE);
if (Double.isNaN(probability) || probability < 0.0 || probability > 1.0) {
throw new IllegalArgumentException("probability must be within [0.0, 1.0]");
}
this.probability = probability;
}

public double getProbability() {
return probability;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@

package io.opentelemetry.contrib.dynamic.policy;

import com.fasterxml.jackson.databind.JsonNode;
import io.opentelemetry.contrib.dynamic.sampler.DelegatingSampler;
import io.opentelemetry.sdk.trace.samplers.Sampler;
import java.util.Collections;
Expand All @@ -16,25 +15,21 @@
* Implements the {@code trace-sampling} policy by updating a {@link DelegatingSampler}.
*
* <p>This implementer listens for validated {@link TelemetryPolicy} updates of type {@code
* "trace-sampling"} and applies the {@code probability} field to the delegate sampler using {@link
* Sampler#traceIdRatioBased(double)} wrapped by {@link Sampler#parentBased(Sampler)}.
* "trace-sampling"} and applies {@link TraceSamplingRatePolicy#getProbability()} to the delegate
* sampler using {@link Sampler#traceIdRatioBased(double)} wrapped by {@link
* Sampler#parentBased(Sampler)}.
*
* <p>If the policy spec is {@code null} (policy removal), the delegate falls back to {@link
* Sampler#alwaysOn()}.
* <p>If a type-only {@link TelemetryPolicy} of type {@code "trace-sampling"} is received, it is
* treated as policy removal and the delegate falls back to {@link Sampler#alwaysOn()}.
*
* <p>Validation is performed by {@link TraceSamplingValidator}; this implementer only consumes
* policies produced by that validator.
*
* <p>Policies with a non-null spec that omit {@code probability} are ignored, which should not
* occur if validation is functioning correctly.
*
* <p>This class is thread-safe. Calls to {@link #onPoliciesChanged(List)} can occur concurrently
* with sampling operations on the associated {@link DelegatingSampler}.
*/
public final class TraceSamplingRatePolicyImplementer implements PolicyImplementer {

private static final String TRACE_SAMPLING_TYPE = "trace-sampling";
private static final String PROBABILITY_FIELD = "probability";
private static final List<PolicyValidator> VALIDATORS =
Collections.<PolicyValidator>singletonList(new TraceSamplingValidator());

Expand All @@ -58,19 +53,17 @@ public List<PolicyValidator> getValidators() {
@Override
public void onPoliciesChanged(List<TelemetryPolicy> policies) {
for (TelemetryPolicy policy : policies) {
if (!TRACE_SAMPLING_TYPE.equals(policy.getType())) {
if (!TraceSamplingRatePolicy.TYPE.equals(policy.getType())) {
continue;
}
JsonNode spec = policy.getSpec();
if (spec == null) {
if (!(policy instanceof TraceSamplingRatePolicy)) {
// Type-only policy represents removing trace-sampling config.
delegatingSampler.setDelegate(Sampler.alwaysOn());
continue;
}
if (spec.has(PROBABILITY_FIELD)) {
double ratio = spec.get(PROBABILITY_FIELD).asDouble(1.0);
Sampler sampler = Sampler.parentBased(Sampler.traceIdRatioBased(ratio));
delegatingSampler.setDelegate(sampler);
}
double ratio = ((TraceSamplingRatePolicy) policy).getProbability();
Sampler sampler = Sampler.parentBased(Sampler.traceIdRatioBased(ratio));
delegatingSampler.setDelegate(sampler);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ public final class TraceSamplingValidator implements PolicyValidator {

@Override
public String getPolicyType() {
return "trace-sampling";
return TraceSamplingRatePolicy.TYPE;
}

@Override
Expand All @@ -43,7 +43,7 @@ public TelemetryPolicy validate(String json) {
if (probNode.isNumber()) {
double d = probNode.asDouble();
if (d >= 0.0 && d <= 1.0) {
return new TelemetryPolicy(getPolicyType(), spec);
return new TraceSamplingRatePolicy(d);
}
}
}
Expand All @@ -62,8 +62,7 @@ public TelemetryPolicy validateAlias(String key, String value) {
try {
double d = Double.parseDouble(value);
if (d >= 0.0 && d <= 1.0) {
JsonNode spec = MAPPER.createObjectNode().put("probability", d);
return new TelemetryPolicy(getPolicyType(), spec);
return new TraceSamplingRatePolicy(d);
}
} catch (NumberFormatException e) {
// invalid
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ public TelemetryPolicy validate(String json) {
if (!acceptJson) {
return null;
}
return new TelemetryPolicy(TRACE_SAMPLING_TYPE, null);
return new TelemetryPolicy(TRACE_SAMPLING_TYPE);
}

@Override
Expand All @@ -124,7 +124,7 @@ public TelemetryPolicy validateAlias(String key, String value) {
if (!acceptAlias) {
return null;
}
return new TelemetryPolicy(TRACE_SAMPLING_TYPE, null);
return new TelemetryPolicy(TRACE_SAMPLING_TYPE);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,6 @@
import static java.util.Collections.singletonList;
import static org.assertj.core.api.Assertions.assertThat;

import com.fasterxml.jackson.databind.JsonNode;
import com.fasterxml.jackson.databind.ObjectMapper;
import io.opentelemetry.api.common.Attributes;
import io.opentelemetry.api.trace.SpanKind;
import io.opentelemetry.context.Context;
Expand All @@ -24,15 +22,13 @@

class TraceSamplingRatePolicyImplementerTest {

private static final ObjectMapper MAPPER = new ObjectMapper();

@Test
void nullSpecFallsBackToAlwaysOn() {
void typeOnlyTraceSamplingPolicyFallsBackToAlwaysOn() {
DelegatingSampler delegatingSampler = new DelegatingSampler(Sampler.alwaysOff());
TraceSamplingRatePolicyImplementer implementer =
new TraceSamplingRatePolicyImplementer(delegatingSampler);

implementer.onPoliciesChanged(singletonList(new TelemetryPolicy("trace-sampling", null)));
implementer.onPoliciesChanged(singletonList(new TelemetryPolicy("trace-sampling")));

assertThat(decisionFor(delegatingSampler)).isEqualTo(SamplingDecision.RECORD_AND_SAMPLE);
}
Expand All @@ -43,8 +39,7 @@ void appliesProbabilityToDelegate() {
TraceSamplingRatePolicyImplementer implementer =
new TraceSamplingRatePolicyImplementer(delegatingSampler);

implementer.onPoliciesChanged(
singletonList(new TelemetryPolicy("trace-sampling", spec("probability", 1.0))));
implementer.onPoliciesChanged(singletonList(new TraceSamplingRatePolicy(1.0)));

assertThat(decisionFor(delegatingSampler)).isEqualTo(SamplingDecision.RECORD_AND_SAMPLE);
}
Expand All @@ -55,20 +50,7 @@ void ignoresUnrelatedPolicyTypes() {
TraceSamplingRatePolicyImplementer implementer =
new TraceSamplingRatePolicyImplementer(delegatingSampler);

implementer.onPoliciesChanged(
singletonList(new TelemetryPolicy("other-policy", spec("value", 1.0))));

assertThat(decisionFor(delegatingSampler)).isEqualTo(SamplingDecision.DROP);
}

@Test
void ignoresTraceSamplingPolicyWithoutProbability() {
DelegatingSampler delegatingSampler = new DelegatingSampler(Sampler.alwaysOff());
TraceSamplingRatePolicyImplementer implementer =
new TraceSamplingRatePolicyImplementer(delegatingSampler);

implementer.onPoliciesChanged(
singletonList(new TelemetryPolicy("trace-sampling", spec("other-field", 1.0))));
implementer.onPoliciesChanged(singletonList(new TelemetryPolicy("other-policy")));

assertThat(decisionFor(delegatingSampler)).isEqualTo(SamplingDecision.DROP);
}
Expand All @@ -80,9 +62,7 @@ void lastTraceSamplingPolicyWins() {
new TraceSamplingRatePolicyImplementer(delegatingSampler);

List<TelemetryPolicy> policies =
Arrays.asList(
new TelemetryPolicy("trace-sampling", spec("probability", 0.0)),
new TelemetryPolicy("trace-sampling", spec("probability", 1.0)));
Arrays.asList(new TraceSamplingRatePolicy(0.0), new TraceSamplingRatePolicy(1.0));

implementer.onPoliciesChanged(policies);

Expand All @@ -100,8 +80,4 @@ private static SamplingDecision decisionFor(DelegatingSampler sampler) {
Collections.emptyList());
return result.getDecision();
}

private static JsonNode spec(String field, double value) {
return MAPPER.createObjectNode().put(field, value);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
class TraceSamplingValidatorTest {

private static final String TRACE_SAMPLING_POLICY_TYPE = "trace-sampling";
private static final String PROBABILITY_FIELD = "probability";
private static final String TRACE_SAMPLING_ALIAS = "trace-sampling.probability";

private final TraceSamplingValidator validator = new TraceSamplingValidator();
Expand All @@ -36,7 +35,8 @@ void testValidate_ValidJson() {
TelemetryPolicy policy = validator.validate(json);
assertThat(policy).isNotNull();
assertThat(policy.getType()).isEqualTo(TRACE_SAMPLING_POLICY_TYPE);
assertThat(policy.getSpec().get(PROBABILITY_FIELD).asDouble()).isCloseTo(0.5, within(1e-9));
assertThat(policy).isInstanceOf(TraceSamplingRatePolicy.class);
assertThat(((TraceSamplingRatePolicy) policy).getProbability()).isCloseTo(0.5, within(1e-9));
}

@ParameterizedTest
Expand All @@ -46,7 +46,8 @@ void testValidate_ValidJson_BoundaryValues(double probability) {
TelemetryPolicy policy = validator.validate(json);
assertThat(policy).isNotNull();
assertThat(policy.getType()).isEqualTo(TRACE_SAMPLING_POLICY_TYPE);
assertThat(policy.getSpec().get(PROBABILITY_FIELD).asDouble())
assertThat(policy).isInstanceOf(TraceSamplingRatePolicy.class);
assertThat(((TraceSamplingRatePolicy) policy).getProbability())
.isCloseTo(probability, within(1e-9));
}

Expand All @@ -70,8 +71,7 @@ void testValidate_InvalidJson_MissingProbability() {

@Test
void testValidate_InvalidJson_ProbabilityNotNumber() {
String json =
"{\"" + TRACE_SAMPLING_POLICY_TYPE + "\": {\"" + PROBABILITY_FIELD + "\": \"high\"}}";
String json = "{\"" + TRACE_SAMPLING_POLICY_TYPE + "\": {\"probability\": \"high\"}}";
assertThat(validator.validate(json)).isNull();
}

Expand All @@ -87,7 +87,8 @@ void testValidateAlias_Valid() {
TelemetryPolicy policy = validator.validateAlias(TRACE_SAMPLING_ALIAS, "0.5");
assertThat(policy).isNotNull();
assertThat(policy.getType()).isEqualTo(TRACE_SAMPLING_POLICY_TYPE);
assertThat(policy.getSpec().get(PROBABILITY_FIELD).asDouble()).isCloseTo(0.5, within(1e-9));
assertThat(policy).isInstanceOf(TraceSamplingRatePolicy.class);
assertThat(((TraceSamplingRatePolicy) policy).getProbability()).isCloseTo(0.5, within(1e-9));
}

@ParameterizedTest
Expand All @@ -96,7 +97,8 @@ void testValidateAlias_Valid_BoundaryValues(String probability) {
TelemetryPolicy policy = validator.validateAlias(TRACE_SAMPLING_ALIAS, probability);
assertThat(policy).isNotNull();
assertThat(policy.getType()).isEqualTo(TRACE_SAMPLING_POLICY_TYPE);
assertThat(policy.getSpec().get(PROBABILITY_FIELD).asDouble())
assertThat(policy).isInstanceOf(TraceSamplingRatePolicy.class);
assertThat(((TraceSamplingRatePolicy) policy).getProbability())
.isCloseTo(Double.parseDouble(probability), within(1e-9));
}

Expand All @@ -117,12 +119,6 @@ void testValidateAlias_InvalidValue_OutOfRange(String probability) {
}

private static String jsonForProbability(double probability) {
return "{\""
+ TRACE_SAMPLING_POLICY_TYPE
+ "\": {\""
+ PROBABILITY_FIELD
+ "\": "
+ probability
+ "}}";
return "{\"" + TRACE_SAMPLING_POLICY_TYPE + "\": {\"probability\": " + probability + "}}";
}
}
Loading