From ede28370899c6cec07ab9f5752808a0cc0e19db9 Mon Sep 17 00:00:00 2001 From: Armen Petrosyan Date: Tue, 13 Oct 2026 10:07:40 +0300 Subject: [PATCH 1/3] Keep the current sampler when the Jaeger sampling strategy is unchanged JaegerRemoteSampler built a new sampler from every poll response, even when the strategy had not changed. For a RATE_LIMITING strategy that means a new RateLimiter with a full balance on every poll, so under load each poll lets through up to maxTracesPerSecond extra traces. Remember the last applied response and keep the current sampler when the response bytes are the same. Keeping the limiter state when only the rate changes is left for a follow-up. Part of #2465 --- .../jaeger/sampler/JaegerRemoteSampler.java | 22 +++++++++++-- .../sampler/JaegerRemoteSamplerTest.java | 32 +++++++++++++++++++ 2 files changed, 51 insertions(+), 3 deletions(-) diff --git a/sdk-extensions/jaeger-remote-sampler/src/main/java/io/opentelemetry/sdk/extension/trace/jaeger/sampler/JaegerRemoteSampler.java b/sdk-extensions/jaeger-remote-sampler/src/main/java/io/opentelemetry/sdk/extension/trace/jaeger/sampler/JaegerRemoteSampler.java index ac8a6d56033..8d23a45d205 100644 --- a/sdk-extensions/jaeger-remote-sampler/src/main/java/io/opentelemetry/sdk/extension/trace/jaeger/sampler/JaegerRemoteSampler.java +++ b/sdk-extensions/jaeger-remote-sampler/src/main/java/io/opentelemetry/sdk/extension/trace/jaeger/sampler/JaegerRemoteSampler.java @@ -6,6 +6,7 @@ package io.opentelemetry.sdk.extension.trace.jaeger.sampler; import io.opentelemetry.api.common.Attributes; +import io.opentelemetry.api.internal.GuardedBy; import io.opentelemetry.api.trace.SpanKind; import io.opentelemetry.context.Context; import io.opentelemetry.sdk.common.CompletableResultCode; @@ -19,6 +20,7 @@ import io.opentelemetry.sdk.trace.samplers.SamplingResult; import java.io.IOException; import java.net.URI; +import java.util.Arrays; import java.util.List; import java.util.concurrent.Executors; import java.util.concurrent.ScheduledExecutorService; @@ -48,6 +50,11 @@ public final class JaegerRemoteSampler implements Sampler { private final GrpcSender grpcSender; private final URI endpoint; private final int pollingIntervalMs; + private final Object lock = new Object(); + + @GuardedBy("lock") + @Nullable + private byte[] lastResponseMessage; JaegerRemoteSampler( GrpcSender grpcSender, @@ -95,10 +102,19 @@ private void onResponse(GrpcResponse grpcResponse) { if (isShutdown.get()) { return; } + byte[] responseMessage = grpcResponse.getResponseMessage(); try { - SamplingStrategyResponse strategyResponse = - SamplingStrategyResponseUnMarshaler.read(grpcResponse.getResponseMessage()); - sampler = updateSampler(strategyResponse); + synchronized (lock) { + // Keep the current sampler when the strategy hasn't changed, so stateful samplers such + // as RateLimitingSampler don't lose their state on every poll. + if (Arrays.equals(responseMessage, lastResponseMessage)) { + return; + } + SamplingStrategyResponse strategyResponse = + SamplingStrategyResponseUnMarshaler.read(responseMessage); + sampler = updateSampler(strategyResponse); + lastResponseMessage = responseMessage; + } } catch (IOException e) { logger.log(Level.WARNING, "Failed to unmarshal strategy response", e); } diff --git a/sdk-extensions/jaeger-remote-sampler/src/test/java/io/opentelemetry/sdk/extension/trace/jaeger/sampler/JaegerRemoteSamplerTest.java b/sdk-extensions/jaeger-remote-sampler/src/test/java/io/opentelemetry/sdk/extension/trace/jaeger/sampler/JaegerRemoteSamplerTest.java index 5b42ca9f0d8..46ff40a0cd3 100644 --- a/sdk-extensions/jaeger-remote-sampler/src/test/java/io/opentelemetry/sdk/extension/trace/jaeger/sampler/JaegerRemoteSamplerTest.java +++ b/sdk-extensions/jaeger-remote-sampler/src/test/java/io/opentelemetry/sdk/extension/trace/jaeger/sampler/JaegerRemoteSamplerTest.java @@ -38,6 +38,7 @@ import java.util.Collections; import java.util.concurrent.ConcurrentLinkedQueue; import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; import java.util.stream.Stream; import javax.annotation.Nullable; import javax.net.ssl.KeyManager; @@ -71,6 +72,8 @@ class JaegerRemoteSamplerTest { private static final ConcurrentLinkedQueue responses = new ConcurrentLinkedQueue<>(); + private static final AtomicInteger requestCount = new AtomicInteger(); + private static void addGrpcError(GrpcStatusCode code, @Nullable String message) { grpcErrors.add(new ArmeriaStatusException(code.getValue(), message)); } @@ -104,6 +107,7 @@ public void getSamplingStrategy( io.opentelemetry.sdk.extension.trace.jaeger.proto.api_v2 .Sampling.SamplingStrategyResponse> responseObserver) { + requestCount.incrementAndGet(); ArmeriaStatusException grpcError = grpcErrors.poll(); if (grpcError != null) { responseObserver.onError(grpcError); @@ -142,6 +146,7 @@ public void getSamplingStrategy( public void before() { grpcErrors.clear(); responses.clear(); + requestCount.set(0); } @Test @@ -449,6 +454,33 @@ void perOperationSampling_zeroRateLimiting() { } } + @Test + void unchangedStrategy_keepsSampler() { + try (JaegerRemoteSampler sampler = + JaegerRemoteSampler.builder() + .setEndpoint(server.httpUri().toString()) + .setServiceName(SERVICE_NAME) + .setPollingInterval(50, TimeUnit.MILLISECONDS) + .build()) { + await().untilAsserted(samplerIsType(sampler, RateLimitingSampler.class)); + Sampler initial = sampler.getSampler(); + + // Further polls return the same strategy, so the stateful sampler must be retained. + int polls = requestCount.get(); + await().untilAsserted(() -> assertThat(requestCount.get()).isGreaterThan(polls + 2)); + assertThat(sampler.getSampler()).isSameAs(initial); + + // A changed strategy still replaces the sampler. + responses.add( + Sampling.SamplingStrategyResponse.newBuilder() + .setStrategyType(SamplingStrategyType.RATE_LIMITING) + .setRateLimitingSampling( + RateLimitingSamplingStrategy.newBuilder().setMaxTracesPerSecond(5).build()) + .build()); + await().untilAsserted(() -> assertThat(sampler.getSampler()).isNotSameAs(initial)); + } + } + @Test void internal_error_server_response() { addGrpcError(GrpcStatusCode.INTERNAL, "internal error"); From 7e856bb61b2e96faccf73eb5933cec388886d069 Mon Sep 17 00:00:00 2001 From: Armen Petrosyan Date: Fri, 6 Nov 2026 17:18:43 +0200 Subject: [PATCH 2/3] Compare the parsed strategy instead of the raw response bytes The unchanged check held the last response message and compared bytes, so a semantically identical strategy that happened to be encoded differently would still have replaced the sampler. Give SamplingStrategyResponse and its nested strategies equals/hashCode and keep the last parsed response instead. --- .../jaeger/sampler/JaegerRemoteSampler.java | 12 ++- .../sampler/SamplingStrategyResponse.java | 89 +++++++++++++++++++ 2 files changed, 94 insertions(+), 7 deletions(-) diff --git a/sdk-extensions/jaeger-remote-sampler/src/main/java/io/opentelemetry/sdk/extension/trace/jaeger/sampler/JaegerRemoteSampler.java b/sdk-extensions/jaeger-remote-sampler/src/main/java/io/opentelemetry/sdk/extension/trace/jaeger/sampler/JaegerRemoteSampler.java index 8d23a45d205..9b6cfa0dd84 100644 --- a/sdk-extensions/jaeger-remote-sampler/src/main/java/io/opentelemetry/sdk/extension/trace/jaeger/sampler/JaegerRemoteSampler.java +++ b/sdk-extensions/jaeger-remote-sampler/src/main/java/io/opentelemetry/sdk/extension/trace/jaeger/sampler/JaegerRemoteSampler.java @@ -20,7 +20,6 @@ import io.opentelemetry.sdk.trace.samplers.SamplingResult; import java.io.IOException; import java.net.URI; -import java.util.Arrays; import java.util.List; import java.util.concurrent.Executors; import java.util.concurrent.ScheduledExecutorService; @@ -54,7 +53,7 @@ public final class JaegerRemoteSampler implements Sampler { @GuardedBy("lock") @Nullable - private byte[] lastResponseMessage; + private SamplingStrategyResponse lastStrategyResponse; JaegerRemoteSampler( GrpcSender grpcSender, @@ -102,18 +101,17 @@ private void onResponse(GrpcResponse grpcResponse) { if (isShutdown.get()) { return; } - byte[] responseMessage = grpcResponse.getResponseMessage(); try { + SamplingStrategyResponse strategyResponse = + SamplingStrategyResponseUnMarshaler.read(grpcResponse.getResponseMessage()); synchronized (lock) { // Keep the current sampler when the strategy hasn't changed, so stateful samplers such // as RateLimitingSampler don't lose their state on every poll. - if (Arrays.equals(responseMessage, lastResponseMessage)) { + if (strategyResponse.equals(lastStrategyResponse)) { return; } - SamplingStrategyResponse strategyResponse = - SamplingStrategyResponseUnMarshaler.read(responseMessage); sampler = updateSampler(strategyResponse); - lastResponseMessage = responseMessage; + lastStrategyResponse = strategyResponse; } } catch (IOException e) { logger.log(Level.WARNING, "Failed to unmarshal strategy response", e); diff --git a/sdk-extensions/jaeger-remote-sampler/src/main/java/io/opentelemetry/sdk/extension/trace/jaeger/sampler/SamplingStrategyResponse.java b/sdk-extensions/jaeger-remote-sampler/src/main/java/io/opentelemetry/sdk/extension/trace/jaeger/sampler/SamplingStrategyResponse.java index 7f1d5fc2281..d00d73c25ab 100644 --- a/sdk-extensions/jaeger-remote-sampler/src/main/java/io/opentelemetry/sdk/extension/trace/jaeger/sampler/SamplingStrategyResponse.java +++ b/sdk-extensions/jaeger-remote-sampler/src/main/java/io/opentelemetry/sdk/extension/trace/jaeger/sampler/SamplingStrategyResponse.java @@ -8,6 +8,7 @@ import java.util.ArrayList; import java.util.Collections; import java.util.List; +import javax.annotation.Nullable; class SamplingStrategyResponse { @@ -24,6 +25,20 @@ private RateLimitingSamplingStrategy(Builder builder) { this.maxTracesPerSecond = builder.maxTracesPerSecond; } + @Override + public boolean equals(@Nullable Object o) { + if (!(o instanceof RateLimitingSamplingStrategy)) { + return false; + } + RateLimitingSamplingStrategy that = (RateLimitingSamplingStrategy) o; + return maxTracesPerSecond == that.maxTracesPerSecond; + } + + @Override + public int hashCode() { + return Integer.hashCode(maxTracesPerSecond); + } + static class Builder { private int maxTracesPerSecond; @@ -45,6 +60,20 @@ static class ProbabilisticSamplingStrategy { this.samplingRate = builder.samplingRate; } + @Override + public boolean equals(@Nullable Object o) { + if (!(o instanceof ProbabilisticSamplingStrategy)) { + return false; + } + ProbabilisticSamplingStrategy that = (ProbabilisticSamplingStrategy) o; + return Double.doubleToLongBits(samplingRate) == Double.doubleToLongBits(that.samplingRate); + } + + @Override + public int hashCode() { + return Double.hashCode(samplingRate); + } + static class Builder { private double samplingRate; @@ -72,6 +101,30 @@ private PerOperationSamplingStrategies(Builder builder) { this.strategies = Collections.unmodifiableList(builder.strategies); } + @Override + public boolean equals(@Nullable Object o) { + if (!(o instanceof PerOperationSamplingStrategies)) { + return false; + } + PerOperationSamplingStrategies that = (PerOperationSamplingStrategies) o; + return Double.doubleToLongBits(defaultSamplingProbability) + == Double.doubleToLongBits(that.defaultSamplingProbability) + && Double.doubleToLongBits(defaultLowerBoundTracesPerSecond) + == Double.doubleToLongBits(that.defaultLowerBoundTracesPerSecond) + && Double.doubleToLongBits(defaultUpperBoundTracesPerSecond) + == Double.doubleToLongBits(that.defaultUpperBoundTracesPerSecond) + && strategies.equals(that.strategies); + } + + @Override + public int hashCode() { + int result = Double.hashCode(defaultSamplingProbability); + result = 31 * result + Double.hashCode(defaultLowerBoundTracesPerSecond); + result = 31 * result + Double.hashCode(defaultUpperBoundTracesPerSecond); + result = 31 * result + strategies.hashCode(); + return result; + } + static class Builder { private double defaultSamplingProbability; private double defaultLowerBoundTracesPerSecond; @@ -113,6 +166,21 @@ private OperationSamplingStrategy(Builder builder) { this.probabilisticSamplingStrategy = builder.probabilisticSamplingStrategy; } + @Override + public boolean equals(@Nullable Object o) { + if (!(o instanceof OperationSamplingStrategy)) { + return false; + } + OperationSamplingStrategy that = (OperationSamplingStrategy) o; + return operation.equals(that.operation) + && probabilisticSamplingStrategy.equals(that.probabilisticSamplingStrategy); + } + + @Override + public int hashCode() { + return 31 * operation.hashCode() + probabilisticSamplingStrategy.hashCode(); + } + static class Builder { private String operation = ""; private ProbabilisticSamplingStrategy probabilisticSamplingStrategy = @@ -147,6 +215,27 @@ private SamplingStrategyResponse(Builder builder) { this.perOperationSamplingStrategies = builder.perOperationSamplingStrategies; } + @Override + public boolean equals(@Nullable Object o) { + if (!(o instanceof SamplingStrategyResponse)) { + return false; + } + SamplingStrategyResponse that = (SamplingStrategyResponse) o; + return strategyType == that.strategyType + && rateLimitingSamplingStrategy.equals(that.rateLimitingSamplingStrategy) + && probabilisticSamplingStrategy.equals(that.probabilisticSamplingStrategy) + && perOperationSamplingStrategies.equals(that.perOperationSamplingStrategies); + } + + @Override + public int hashCode() { + int result = strategyType.hashCode(); + result = 31 * result + rateLimitingSamplingStrategy.hashCode(); + result = 31 * result + probabilisticSamplingStrategy.hashCode(); + result = 31 * result + perOperationSamplingStrategies.hashCode(); + return result; + } + static class Builder { private SamplingStrategyType samplingStrategyType = SamplingStrategyType.UNRECOGNIZED; private RateLimitingSamplingStrategy rateLimitingSamplingStrategy = From 8d087b2e62d043fd2b4f7ae162480b14e8f2a61f Mon Sep 17 00:00:00 2001 From: Armen Petrosyan Date: Fri, 13 Nov 2026 09:55:14 +0200 Subject: [PATCH 3/3] Cover the new equals/hashCode with EqualsVerifier The five strategy types are final now, so the instanceof checks in equals hold for subclasses too and EqualsVerifier needs no inheritance suppression. The builders always supply a value, so the reference fields are never null in practice and NULL_FIELDS is suppressed where equals dereferences them. --- .../sampler/SamplingStrategyResponse.java | 10 +++---- .../sampler/SamplingStrategyResponseTest.java | 29 +++++++++++++++++++ 2 files changed, 34 insertions(+), 5 deletions(-) create mode 100644 sdk-extensions/jaeger-remote-sampler/src/test/java/io/opentelemetry/sdk/extension/trace/jaeger/sampler/SamplingStrategyResponseTest.java diff --git a/sdk-extensions/jaeger-remote-sampler/src/main/java/io/opentelemetry/sdk/extension/trace/jaeger/sampler/SamplingStrategyResponse.java b/sdk-extensions/jaeger-remote-sampler/src/main/java/io/opentelemetry/sdk/extension/trace/jaeger/sampler/SamplingStrategyResponse.java index d00d73c25ab..05bd072192f 100644 --- a/sdk-extensions/jaeger-remote-sampler/src/main/java/io/opentelemetry/sdk/extension/trace/jaeger/sampler/SamplingStrategyResponse.java +++ b/sdk-extensions/jaeger-remote-sampler/src/main/java/io/opentelemetry/sdk/extension/trace/jaeger/sampler/SamplingStrategyResponse.java @@ -10,7 +10,7 @@ import java.util.List; import javax.annotation.Nullable; -class SamplingStrategyResponse { +final class SamplingStrategyResponse { enum SamplingStrategyType { PROBABILISTIC, @@ -18,7 +18,7 @@ enum SamplingStrategyType { UNRECOGNIZED, } - static class RateLimitingSamplingStrategy { + static final class RateLimitingSamplingStrategy { final int maxTracesPerSecond; private RateLimitingSamplingStrategy(Builder builder) { @@ -53,7 +53,7 @@ RateLimitingSamplingStrategy build() { } } - static class ProbabilisticSamplingStrategy { + static final class ProbabilisticSamplingStrategy { final double samplingRate; ProbabilisticSamplingStrategy(Builder builder) { @@ -88,7 +88,7 @@ ProbabilisticSamplingStrategy build() { } } - static class PerOperationSamplingStrategies { + static final class PerOperationSamplingStrategies { final double defaultSamplingProbability; final double defaultLowerBoundTracesPerSecond; final double defaultUpperBoundTracesPerSecond; @@ -157,7 +157,7 @@ PerOperationSamplingStrategies build() { } } - static class OperationSamplingStrategy { + static final class OperationSamplingStrategy { final String operation; final ProbabilisticSamplingStrategy probabilisticSamplingStrategy; diff --git a/sdk-extensions/jaeger-remote-sampler/src/test/java/io/opentelemetry/sdk/extension/trace/jaeger/sampler/SamplingStrategyResponseTest.java b/sdk-extensions/jaeger-remote-sampler/src/test/java/io/opentelemetry/sdk/extension/trace/jaeger/sampler/SamplingStrategyResponseTest.java new file mode 100644 index 00000000000..f9fef1a475a --- /dev/null +++ b/sdk-extensions/jaeger-remote-sampler/src/test/java/io/opentelemetry/sdk/extension/trace/jaeger/sampler/SamplingStrategyResponseTest.java @@ -0,0 +1,29 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.sdk.extension.trace.jaeger.sampler; + +import io.opentelemetry.sdk.extension.trace.jaeger.sampler.SamplingStrategyResponse.OperationSamplingStrategy; +import io.opentelemetry.sdk.extension.trace.jaeger.sampler.SamplingStrategyResponse.PerOperationSamplingStrategies; +import io.opentelemetry.sdk.extension.trace.jaeger.sampler.SamplingStrategyResponse.ProbabilisticSamplingStrategy; +import io.opentelemetry.sdk.extension.trace.jaeger.sampler.SamplingStrategyResponse.RateLimitingSamplingStrategy; +import nl.jqno.equalsverifier.EqualsVerifier; +import nl.jqno.equalsverifier.Warning; +import org.junit.jupiter.api.Test; + +class SamplingStrategyResponseTest { + + @Test + void equalsVerifier() { + // The builders always supply a value, so the fields are never null in practice. + EqualsVerifier.forClass(RateLimitingSamplingStrategy.class).verify(); + EqualsVerifier.forClass(ProbabilisticSamplingStrategy.class).verify(); + EqualsVerifier.forClass(OperationSamplingStrategy.class).suppress(Warning.NULL_FIELDS).verify(); + EqualsVerifier.forClass(PerOperationSamplingStrategies.class) + .suppress(Warning.NULL_FIELDS) + .verify(); + EqualsVerifier.forClass(SamplingStrategyResponse.class).suppress(Warning.NULL_FIELDS).verify(); + } +}