From 0163e53ad55eb02cdb19eb1e10bfb63f1ca44890 Mon Sep 17 00:00:00 2001 From: Alexander Dinauer Date: Tue, 28 Jun 2022 11:54:53 +0200 Subject: [PATCH 1/9] Add sample rate from traces sampler to DSC --- .../PerformanceAndroidEventProcessorTest.kt | 3 +- .../apollo/SentryApolloInterceptorTest.kt | 3 +- .../SentrySpanRestTemplateCustomizerTest.kt | 3 +- .../boot/SentrySpanWebClientCustomizerTest.kt | 3 +- sentry/api/sentry.api | 23 ++++- sentry/src/main/java/io/sentry/Hub.java | 6 +- .../src/main/java/io/sentry/ITransaction.java | 3 + .../main/java/io/sentry/NoOpTransaction.java | 5 ++ .../src/main/java/io/sentry/SentryTracer.java | 9 +- sentry/src/main/java/io/sentry/Span.java | 7 +- .../src/main/java/io/sentry/SpanContext.java | 35 +++++--- .../src/main/java/io/sentry/TraceContext.java | 16 +++- .../main/java/io/sentry/TracesSampler.java | 28 +++++-- .../io/sentry/TracesSamplingDecision.java | 33 ++++++++ .../java/io/sentry/TransactionContext.java | 37 +++++--- .../io/sentry/protocol/SentryTransaction.java | 18 +++- sentry/src/test/java/io/sentry/HubTest.kt | 18 ++-- .../test/java/io/sentry/SentryTracerTest.kt | 22 ++--- sentry/src/test/java/io/sentry/SpanTest.kt | 2 +- .../sentry/TraceContextSerializationTest.kt | 3 +- .../test/java/io/sentry/TracesSamplerTest.kt | 84 ++++++++++++++++--- .../protocol/SpanContextSerializationTest.kt | 3 +- 22 files changed, 283 insertions(+), 81 deletions(-) create mode 100644 sentry/src/main/java/io/sentry/TracesSamplingDecision.java diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/PerformanceAndroidEventProcessorTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/PerformanceAndroidEventProcessorTest.kt index 30a78d411d..4250419e2b 100644 --- a/sentry-android-core/src/test/java/io/sentry/android/core/PerformanceAndroidEventProcessorTest.kt +++ b/sentry-android-core/src/test/java/io/sentry/android/core/PerformanceAndroidEventProcessorTest.kt @@ -6,6 +6,7 @@ import com.nhaarman.mockitokotlin2.whenever import io.sentry.Hint import io.sentry.IHub import io.sentry.SentryTracer +import io.sentry.TracesSamplingDecision import io.sentry.TransactionContext import io.sentry.android.core.ActivityLifecycleIntegration.UI_LOAD_OP import io.sentry.protocol.MeasurementValue @@ -21,7 +22,7 @@ class PerformanceAndroidEventProcessorTest { val options = SentryAndroidOptions() val hub = mock() - val context = TransactionContext("name", "op", true) + val context = TransactionContext("name", "op", TracesSamplingDecision(true)) val tracer = SentryTracer(context, hub) val activityFramesTracker = mock() diff --git a/sentry-apollo/src/test/java/io/sentry/apollo/SentryApolloInterceptorTest.kt b/sentry-apollo/src/test/java/io/sentry/apollo/SentryApolloInterceptorTest.kt index 419c13cedd..4b0760fd17 100644 --- a/sentry-apollo/src/test/java/io/sentry/apollo/SentryApolloInterceptorTest.kt +++ b/sentry-apollo/src/test/java/io/sentry/apollo/SentryApolloInterceptorTest.kt @@ -16,6 +16,7 @@ import io.sentry.SentryTraceHeader import io.sentry.SentryTracer import io.sentry.SpanStatus import io.sentry.TraceContext +import io.sentry.TracesSamplingDecision import io.sentry.TransactionContext import io.sentry.protocol.SentryTransaction import kotlinx.coroutines.launch @@ -200,7 +201,7 @@ class SentryApolloInterceptorTest { private fun executeQuery(sut: ApolloClient = fixture.getSut(), isSpanActive: Boolean = true) = runBlocking { var tx: ITransaction? = null if (isSpanActive) { - tx = SentryTracer(TransactionContext("op", "desc", true), fixture.hub) + tx = SentryTracer(TransactionContext("op", "desc", TracesSamplingDecision(true)), fixture.hub) whenever(fixture.hub.span).thenReturn(tx) } diff --git a/sentry-spring-boot-starter/src/test/kotlin/io/sentry/spring/boot/SentrySpanRestTemplateCustomizerTest.kt b/sentry-spring-boot-starter/src/test/kotlin/io/sentry/spring/boot/SentrySpanRestTemplateCustomizerTest.kt index 3f87cf9f23..db727aa1ee 100644 --- a/sentry-spring-boot-starter/src/test/kotlin/io/sentry/spring/boot/SentrySpanRestTemplateCustomizerTest.kt +++ b/sentry-spring-boot-starter/src/test/kotlin/io/sentry/spring/boot/SentrySpanRestTemplateCustomizerTest.kt @@ -11,6 +11,7 @@ import io.sentry.SentryOptions import io.sentry.SentryTraceHeader import io.sentry.SentryTracer import io.sentry.SpanStatus +import io.sentry.TracesSamplingDecision import io.sentry.TransactionContext import okhttp3.mockwebserver.MockResponse import okhttp3.mockwebserver.MockWebServer @@ -28,7 +29,7 @@ class SentrySpanRestTemplateCustomizerTest { val hub = mock() val restTemplate = RestTemplateBuilder().build() var mockServer = MockWebServer() - val transaction = SentryTracer(TransactionContext("aTransaction", "op", true), hub) + val transaction = SentryTracer(TransactionContext("aTransaction", "op", TracesSamplingDecision(true)), hub) internal val customizer = SentrySpanRestTemplateCustomizer(hub) val url = mockServer.url("/test/123").toString() diff --git a/sentry-spring-boot-starter/src/test/kotlin/io/sentry/spring/boot/SentrySpanWebClientCustomizerTest.kt b/sentry-spring-boot-starter/src/test/kotlin/io/sentry/spring/boot/SentrySpanWebClientCustomizerTest.kt index 8cb5bee0e8..a1a4c8941b 100644 --- a/sentry-spring-boot-starter/src/test/kotlin/io/sentry/spring/boot/SentrySpanWebClientCustomizerTest.kt +++ b/sentry-spring-boot-starter/src/test/kotlin/io/sentry/spring/boot/SentrySpanWebClientCustomizerTest.kt @@ -12,6 +12,7 @@ import io.sentry.SentryOptions import io.sentry.SentryTraceHeader import io.sentry.SentryTracer import io.sentry.SpanStatus +import io.sentry.TracesSamplingDecision import io.sentry.TransactionContext import okhttp3.mockwebserver.Dispatcher import okhttp3.mockwebserver.MockResponse @@ -34,7 +35,7 @@ class SentrySpanWebClientCustomizerTest { lateinit var sentryOptions: SentryOptions val hub = mock() var mockServer = MockWebServer() - val transaction = SentryTracer(TransactionContext("aTransaction", "op", true), hub) + val transaction = SentryTracer(TransactionContext("aTransaction", "op", TracesSamplingDecision(true)), hub) private val customizer = SentrySpanWebClientCustomizer(hub) fun getSut(isTransactionActive: Boolean, status: HttpStatus = HttpStatus.OK, throwIOException: Boolean = false, includeMockServerInTracingOrigins: Boolean = true): WebClient { diff --git a/sentry/api/sentry.api b/sentry/api/sentry.api index dd4fe25cd9..a6f26099a4 100644 --- a/sentry/api/sentry.api +++ b/sentry/api/sentry.api @@ -474,6 +474,7 @@ public abstract interface class io/sentry/ITransaction : io/sentry/ISpan { public abstract fun getEventId ()Lio/sentry/protocol/SentryId; public abstract fun getLatestActiveSpan ()Lio/sentry/Span; public abstract fun getName ()Ljava/lang/String; + public abstract fun getSamplingDecision ()Lio/sentry/TracesSamplingDecision; public abstract fun getSpans ()Ljava/util/List; public abstract fun isSampled ()Ljava/lang/Boolean; public abstract fun scheduleFinish ()V @@ -661,6 +662,7 @@ public final class io/sentry/NoOpTransaction : io/sentry/ITransaction { public fun getLatestActiveSpan ()Lio/sentry/Span; public fun getName ()Ljava/lang/String; public fun getOperation ()Ljava/lang/String; + public fun getSamplingDecision ()Lio/sentry/TracesSamplingDecision; public fun getSpanContext ()Lio/sentry/SpanContext; public fun getSpans ()Ljava/util/List; public fun getStatus ()Lio/sentry/SpanStatus; @@ -1387,6 +1389,7 @@ public final class io/sentry/SentryTracer : io/sentry/ITransaction { public fun getLatestActiveSpan ()Lio/sentry/Span; public fun getName ()Ljava/lang/String; public fun getOperation ()Ljava/lang/String; + public fun getSamplingDecision ()Lio/sentry/TracesSamplingDecision; public fun getSpanContext ()Lio/sentry/SpanContext; public fun getSpans ()Ljava/util/List; public fun getStartTimestamp ()Ljava/util/Date; @@ -1489,6 +1492,7 @@ public final class io/sentry/Span : io/sentry/ISpan { public fun getHighPrecisionTimestamp ()Ljava/lang/Double; public fun getOperation ()Ljava/lang/String; public fun getParentSpanId ()Lio/sentry/SpanId; + public fun getSamplingDecision ()Lio/sentry/TracesSamplingDecision; public fun getSpanContext ()Lio/sentry/SpanContext; public fun getSpanId ()Lio/sentry/SpanId; public fun getStartTimestamp ()Ljava/util/Date; @@ -1521,14 +1525,15 @@ public class io/sentry/SpanContext : io/sentry/JsonSerializable, io/sentry/JsonU protected field status Lio/sentry/SpanStatus; protected field tags Ljava/util/Map; public fun (Lio/sentry/SpanContext;)V - public fun (Lio/sentry/protocol/SentryId;Lio/sentry/SpanId;Lio/sentry/SpanId;Ljava/lang/String;Ljava/lang/String;Ljava/lang/Boolean;Lio/sentry/SpanStatus;)V - public fun (Lio/sentry/protocol/SentryId;Lio/sentry/SpanId;Ljava/lang/String;Lio/sentry/SpanId;Ljava/lang/Boolean;)V + public fun (Lio/sentry/protocol/SentryId;Lio/sentry/SpanId;Lio/sentry/SpanId;Ljava/lang/String;Ljava/lang/String;Lio/sentry/TracesSamplingDecision;Lio/sentry/SpanStatus;)V + public fun (Lio/sentry/protocol/SentryId;Lio/sentry/SpanId;Ljava/lang/String;Lio/sentry/SpanId;Lio/sentry/TracesSamplingDecision;)V public fun (Ljava/lang/String;)V - public fun (Ljava/lang/String;Ljava/lang/Boolean;)V + public fun (Ljava/lang/String;Lio/sentry/TracesSamplingDecision;)V public fun getDescription ()Ljava/lang/String; public fun getOperation ()Ljava/lang/String; public fun getParentSpanId ()Lio/sentry/SpanId; public fun getSampled ()Ljava/lang/Boolean; + public fun getSamplingDecision ()Lio/sentry/TracesSamplingDecision; public fun getSpanId ()Lio/sentry/SpanId; public fun getStatus ()Lio/sentry/SpanStatus; public fun getTags ()Ljava/util/Map; @@ -1538,6 +1543,7 @@ public class io/sentry/SpanContext : io/sentry/JsonSerializable, io/sentry/JsonU public fun setDescription (Ljava/lang/String;)V public fun setOperation (Ljava/lang/String;)V public fun setSampled (Ljava/lang/Boolean;)V + public fun setSamplingDecision (Lio/sentry/TracesSamplingDecision;)V public fun setStatus (Lio/sentry/SpanStatus;)V public fun setTag (Ljava/lang/String;Ljava/lang/String;)V public fun setUnknown (Ljava/util/Map;)V @@ -1649,6 +1655,13 @@ public final class io/sentry/TraceContext$JsonKeys { public fun ()V } +public final class io/sentry/TracesSamplingDecision { + public fun (Ljava/lang/Boolean;)V + public fun (Ljava/lang/Boolean;Ljava/lang/Double;)V + public fun getSampleRate ()Ljava/lang/Double; + public fun getSampled ()Ljava/lang/Boolean; +} + public final class io/sentry/TracingOrigins { public fun ()V public static fun contain (Ljava/util/List;Ljava/lang/String;)Z @@ -1657,10 +1670,11 @@ public final class io/sentry/TracingOrigins { public final class io/sentry/TransactionContext : io/sentry/SpanContext { public fun (Ljava/lang/String;Ljava/lang/String;)V - public fun (Ljava/lang/String;Ljava/lang/String;Ljava/lang/Boolean;)V + public fun (Ljava/lang/String;Ljava/lang/String;Lio/sentry/TracesSamplingDecision;)V public static fun fromSentryTrace (Ljava/lang/String;Ljava/lang/String;Lio/sentry/SentryTraceHeader;)Lio/sentry/TransactionContext; public fun getName ()Ljava/lang/String; public fun getParentSampled ()Ljava/lang/Boolean; + public fun getParentSamplingDecision ()Lio/sentry/TracesSamplingDecision; public fun setParentSampled (Ljava/lang/Boolean;)V } @@ -2809,6 +2823,7 @@ public final class io/sentry/protocol/SentryTransaction : io/sentry/SentryBaseEv public fun (Lio/sentry/SentryTracer;)V public fun (Ljava/lang/String;Ljava/lang/Double;Ljava/lang/Double;Ljava/util/List;Ljava/util/Map;)V public fun getMeasurements ()Ljava/util/Map; + public fun getSamplingDecision ()Lio/sentry/TracesSamplingDecision; public fun getSpans ()Ljava/util/List; public fun getStartTimestamp ()Ljava/lang/Double; public fun getStatus ()Lio/sentry/SpanStatus; diff --git a/sentry/src/main/java/io/sentry/Hub.java b/sentry/src/main/java/io/sentry/Hub.java index 5351c9d2b4..48435c7db0 100644 --- a/sentry/src/main/java/io/sentry/Hub.java +++ b/sentry/src/main/java/io/sentry/Hub.java @@ -730,8 +730,8 @@ public void flush(long timeoutMillis) { } else { final SamplingContext samplingContext = new SamplingContext(transactionContext, customSamplingContext); - boolean samplingDecision = tracesSampler.sample(samplingContext); - transactionContext.setSampled(samplingDecision); + @NotNull TracesSamplingDecision samplingDecision = tracesSampler.sample(samplingContext); + transactionContext.setSamplingDecision(samplingDecision); transaction = new SentryTracer( @@ -745,7 +745,7 @@ public void flush(long timeoutMillis) { // The listener is called only if the transaction exists, as the transaction is needed to // stop it - if (samplingDecision && options.isProfilingEnabled()) { + if (samplingDecision.getSampled() && options.isProfilingEnabled()) { final ITransactionProfiler transactionProfiler = options.getTransactionProfiler(); transactionProfiler.onTransactionStart(transaction); } diff --git a/sentry/src/main/java/io/sentry/ITransaction.java b/sentry/src/main/java/io/sentry/ITransaction.java index 2718f8988e..4ce3e773eb 100644 --- a/sentry/src/main/java/io/sentry/ITransaction.java +++ b/sentry/src/main/java/io/sentry/ITransaction.java @@ -35,6 +35,9 @@ public interface ITransaction extends ISpan { @Nullable Boolean isSampled(); + @Nullable + TracesSamplingDecision getSamplingDecision(); + /** * Returns the latest span that is not finished. * diff --git a/sentry/src/main/java/io/sentry/NoOpTransaction.java b/sentry/src/main/java/io/sentry/NoOpTransaction.java index 0c415449bb..8e4b6e0410 100644 --- a/sentry/src/main/java/io/sentry/NoOpTransaction.java +++ b/sentry/src/main/java/io/sentry/NoOpTransaction.java @@ -136,6 +136,11 @@ public void setTag(@NotNull String key, @NotNull String value) {} return null; } + @Override + public @Nullable TracesSamplingDecision getSamplingDecision() { + return null; + } + @Override public void setData(@NotNull String key, @NotNull Object value) {} diff --git a/sentry/src/main/java/io/sentry/SentryTracer.java b/sentry/src/main/java/io/sentry/SentryTracer.java index e941dc7ba5..a05d37c579 100644 --- a/sentry/src/main/java/io/sentry/SentryTracer.java +++ b/sentry/src/main/java/io/sentry/SentryTracer.java @@ -367,7 +367,9 @@ public void finish(@Nullable SpanStatus status) { scope -> { userAtomicReference.set(scope.getUser()); }); - this.traceContext = new TraceContext(this, userAtomicReference.get(), hub.getOptions()); + this.traceContext = + new TraceContext( + this, userAtomicReference.get(), hub.getOptions(), this.getSamplingDecision()); } return this.traceContext; } @@ -502,6 +504,11 @@ public void setData(@NotNull String key, @NotNull Object value) { return this.root.isSampled(); } + @Override + public @Nullable TracesSamplingDecision getSamplingDecision() { + return this.root.getSamplingDecision(); + } + @Override public void setName(@NotNull String name) { if (root.isFinished()) { diff --git a/sentry/src/main/java/io/sentry/Span.java b/sentry/src/main/java/io/sentry/Span.java index 5207adea91..1f5889c669 100644 --- a/sentry/src/main/java/io/sentry/Span.java +++ b/sentry/src/main/java/io/sentry/Span.java @@ -66,7 +66,8 @@ public final class Span implements ISpan { final @Nullable Date startTimestamp, final @Nullable SpanFinishedCallback spanFinishedCallback) { this.context = - new SpanContext(traceId, new SpanId(), operation, parentSpanId, transaction.isSampled()); + new SpanContext( + traceId, new SpanId(), operation, parentSpanId, transaction.getSamplingDecision()); this.transaction = Objects.requireNonNull(transaction, "transaction is required"); this.hub = Objects.requireNonNull(hub, "hub is required"); this.spanFinishedCallback = spanFinishedCallback; @@ -258,6 +259,10 @@ public boolean isFinished() { return context.getSampled(); } + public @Nullable TracesSamplingDecision getSamplingDecision() { + return context.getSamplingDecision(); + } + @Override public void setThrowable(final @Nullable Throwable throwable) { if (finished.get()) { diff --git a/sentry/src/main/java/io/sentry/SpanContext.java b/sentry/src/main/java/io/sentry/SpanContext.java index 7efce08c30..3bb77a74d7 100644 --- a/sentry/src/main/java/io/sentry/SpanContext.java +++ b/sentry/src/main/java/io/sentry/SpanContext.java @@ -26,8 +26,7 @@ public class SpanContext implements JsonUnknown, JsonSerializable { /** Id of a parent span. */ private final @Nullable SpanId parentSpanId; - /** If trace is sampled. */ - private transient @Nullable Boolean sampled; + private transient @Nullable TracesSamplingDecision samplingDecision; /** Short code identifying the type of operation the span is measuring. */ protected @NotNull String op; @@ -46,8 +45,9 @@ public class SpanContext implements JsonUnknown, JsonSerializable { private @Nullable Map unknown; - public SpanContext(final @NotNull String operation, final @Nullable Boolean sampled) { - this(new SentryId(), new SpanId(), operation, null, sampled); + public SpanContext( + final @NotNull String operation, final @Nullable TracesSamplingDecision samplingDecision) { + this(new SentryId(), new SpanId(), operation, null, samplingDecision); } /** @@ -64,8 +64,8 @@ public SpanContext( final @NotNull SpanId spanId, final @NotNull String operation, final @Nullable SpanId parentSpanId, - final @Nullable Boolean sampled) { - this(traceId, spanId, parentSpanId, operation, null, sampled, null); + final @Nullable TracesSamplingDecision samplingDecision) { + this(traceId, spanId, parentSpanId, operation, null, samplingDecision, null); } @ApiStatus.Internal @@ -75,13 +75,13 @@ public SpanContext( final @Nullable SpanId parentSpanId, final @NotNull String operation, final @Nullable String description, - final @Nullable Boolean sampled, + final @Nullable TracesSamplingDecision samplingDecision, final @Nullable SpanStatus status) { this.traceId = Objects.requireNonNull(traceId, "traceId is required"); this.spanId = Objects.requireNonNull(spanId, "spanId is required"); this.op = Objects.requireNonNull(operation, "operation is required"); this.parentSpanId = parentSpanId; - this.sampled = sampled; + this.samplingDecision = samplingDecision; this.description = description; this.status = status; } @@ -95,7 +95,7 @@ public SpanContext(final @NotNull SpanContext spanContext) { this.traceId = spanContext.traceId; this.spanId = spanContext.spanId; this.parentSpanId = spanContext.parentSpanId; - this.sampled = spanContext.sampled; + this.samplingDecision = spanContext.samplingDecision; this.op = spanContext.op; this.description = spanContext.description; this.status = spanContext.status; @@ -155,13 +155,26 @@ public SpanId getParentSpanId() { return tags; } + public @Nullable TracesSamplingDecision getSamplingDecision() { + return samplingDecision; + } + public @Nullable Boolean getSampled() { - return sampled; + if (samplingDecision == null) { + return null; + } + + return samplingDecision.getSampled(); } @ApiStatus.Internal public void setSampled(final @Nullable Boolean sampled) { - this.sampled = sampled; + this.setSamplingDecision(new TracesSamplingDecision(sampled == null ? false : sampled)); + } + + @ApiStatus.Internal + public void setSamplingDecision(final @Nullable TracesSamplingDecision samplingDecision) { + this.samplingDecision = samplingDecision; } // region JsonSerializable diff --git a/sentry/src/main/java/io/sentry/TraceContext.java b/sentry/src/main/java/io/sentry/TraceContext.java index 55ab322e02..f30e2e0a8f 100644 --- a/sentry/src/main/java/io/sentry/TraceContext.java +++ b/sentry/src/main/java/io/sentry/TraceContext.java @@ -53,7 +53,8 @@ public final class TraceContext implements JsonUnknown, JsonSerializable { TraceContext( final @NotNull ITransaction transaction, final @Nullable User user, - final @NotNull SentryOptions sentryOptions) { + final @NotNull SentryOptions sentryOptions, + final @Nullable TracesSamplingDecision samplingDecision) { this( transaction.getSpanContext().getTraceId(), new Dsn(sentryOptions.getDsn()).getPublicKey(), @@ -62,7 +63,7 @@ public final class TraceContext implements JsonUnknown, JsonSerializable { user != null ? user.getId() : null, user != null ? getSegment(user) : null, transaction.getName(), - sampleRateToString(sentryOptions.getTracesSampleRate())); + sampleRateToString(sampleRate(sentryOptions, samplingDecision))); } private static @Nullable String getSegment(final @NotNull User user) { @@ -74,6 +75,17 @@ public final class TraceContext implements JsonUnknown, JsonSerializable { } } + private static @Nullable Double sampleRate( + @NotNull SentryOptions sentryOptions, @Nullable TracesSamplingDecision samplingDecision) { + // TODO does this fallback even make sense? could also just write 1.0 as + // options.tracesSampleRate should have been written to a sampling decision + if (samplingDecision == null) { + return sentryOptions.getTracesSampleRate(); + } + + return samplingDecision.getSampleRate(); + } + private static @Nullable String sampleRateToString(@Nullable Double sampleRateAsDouble) { if (sampleRateAsDouble == null) { return null; diff --git a/sentry/src/main/java/io/sentry/TracesSampler.java b/sentry/src/main/java/io/sentry/TracesSampler.java index 9f14ec9502..08fef2846b 100644 --- a/sentry/src/main/java/io/sentry/TracesSampler.java +++ b/sentry/src/main/java/io/sentry/TracesSampler.java @@ -19,23 +19,33 @@ public TracesSampler(final @NotNull SentryOptions options) { this.random = random; } - boolean sample(final @NotNull SamplingContext samplingContext) { - if (samplingContext.getTransactionContext().getSampled() != null) { - return samplingContext.getTransactionContext().getSampled(); + @NotNull + TracesSamplingDecision sample(final @NotNull SamplingContext samplingContext) { + TracesSamplingDecision samplingContextSamplingDecision = + samplingContext.getTransactionContext().getSamplingDecision(); + if (samplingContextSamplingDecision != null) { + return samplingContextSamplingDecision; } + if (options.getTracesSampler() != null) { final Double samplerResult = options.getTracesSampler().sample(samplingContext); if (samplerResult != null) { - return sample(samplerResult); + return new TracesSamplingDecision(sample(samplerResult), samplerResult); } } - if (samplingContext.getTransactionContext().getParentSampled() != null) { - return samplingContext.getTransactionContext().getParentSampled(); + + Boolean parentSampled = samplingContext.getTransactionContext().getParentSampled(); + if (parentSampled != null) { + return new TracesSamplingDecision(parentSampled); } - if (options.getTracesSampleRate() != null) { - return sample(options.getTracesSampleRate()); + + Double tracesSampleRateFromOptions = options.getTracesSampleRate(); + if (tracesSampleRateFromOptions != null) { + return new TracesSamplingDecision( + sample(tracesSampleRateFromOptions), tracesSampleRateFromOptions); } - return false; + + return new TracesSamplingDecision(false); } private boolean sample(final @NotNull Double aDouble) { diff --git a/sentry/src/main/java/io/sentry/TracesSamplingDecision.java b/sentry/src/main/java/io/sentry/TracesSamplingDecision.java new file mode 100644 index 0000000000..559a1333cc --- /dev/null +++ b/sentry/src/main/java/io/sentry/TracesSamplingDecision.java @@ -0,0 +1,33 @@ +package io.sentry; + +import org.jetbrains.annotations.ApiStatus; +import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; + +@ApiStatus.Internal +public final class TracesSamplingDecision { + + private final @NotNull Boolean sampled; + private final @NotNull Double sampleRate; + + public TracesSamplingDecision(@NotNull Boolean sampled, @Nullable Double sampleRate) { + this.sampled = sampled; + if (sampleRate != null) { + this.sampleRate = sampleRate; + } else { + this.sampleRate = sampled ? 1.0 : 0.0; + } + } + + public TracesSamplingDecision(@NotNull Boolean sampled) { + this(sampled, null); + } + + public @NotNull Boolean getSampled() { + return sampled; + } + + public @NotNull Double getSampleRate() { + return sampleRate; + } +} diff --git a/sentry/src/main/java/io/sentry/TransactionContext.java b/sentry/src/main/java/io/sentry/TransactionContext.java index 25296713b7..4b7c1f8da1 100644 --- a/sentry/src/main/java/io/sentry/TransactionContext.java +++ b/sentry/src/main/java/io/sentry/TransactionContext.java @@ -7,7 +7,7 @@ public final class TransactionContext extends SpanContext { private final @NotNull String name; - private @Nullable Boolean parentSampled; + private @Nullable TracesSamplingDecision parentSamplingDecision; /** * Creates {@link TransactionContext} from sentry-trace header. @@ -21,19 +21,24 @@ public final class TransactionContext extends SpanContext { final @NotNull String name, final @NotNull String operation, final @NotNull SentryTraceHeader sentryTrace) { + @Nullable Boolean parentSampled = sentryTrace.isSampled(); return new TransactionContext( name, operation, sentryTrace.getTraceId(), new SpanId(), sentryTrace.getSpanId(), - sentryTrace.isSampled()); + parentSampled == null + ? null + : new TracesSamplingDecision( + parentSampled)); // TODO sampleRate should be retrieved from baggage and passed here + // in the future } public TransactionContext(final @NotNull String name, final @NotNull String operation) { super(operation); this.name = Objects.requireNonNull(name, "name is required"); - this.parentSampled = null; + this.parentSamplingDecision = null; } /** @@ -41,15 +46,15 @@ public TransactionContext(final @NotNull String name, final @NotNull String oper * * @param name - transaction name * @param operation - operation - * @param sampled - sampling decision + * @param samplingDecision - sampling decision */ public TransactionContext( final @NotNull String name, final @NotNull String operation, - final @Nullable Boolean sampled) { + final @Nullable TracesSamplingDecision samplingDecision) { super(operation); this.name = Objects.requireNonNull(name, "name is required"); - this.setSampled(sampled); + this.setSamplingDecision(samplingDecision); } private TransactionContext( @@ -58,10 +63,10 @@ private TransactionContext( final @NotNull SentryId traceId, final @NotNull SpanId spanId, final @Nullable SpanId parentSpanId, - final @Nullable Boolean parentSampled) { + final @Nullable TracesSamplingDecision parentSamplingDecision) { super(traceId, spanId, operation, parentSpanId, null); this.name = Objects.requireNonNull(name, "name is required"); - this.parentSampled = parentSampled; + this.parentSamplingDecision = parentSamplingDecision; } public @NotNull String getName() { @@ -69,10 +74,22 @@ private TransactionContext( } public @Nullable Boolean getParentSampled() { - return parentSampled; + if (parentSamplingDecision == null) { + return null; + } + + return parentSamplingDecision.getSampled(); + } + + public @Nullable TracesSamplingDecision getParentSamplingDecision() { + return parentSamplingDecision; } public void setParentSampled(final @Nullable Boolean parentSampled) { - this.parentSampled = parentSampled; + if (parentSampled == null) { + this.parentSamplingDecision = null; + } else { + this.parentSamplingDecision = new TracesSamplingDecision(parentSampled); + } } } diff --git a/sentry/src/main/java/io/sentry/protocol/SentryTransaction.java b/sentry/src/main/java/io/sentry/protocol/SentryTransaction.java index fecd9072f8..f2c6f31898 100644 --- a/sentry/src/main/java/io/sentry/protocol/SentryTransaction.java +++ b/sentry/src/main/java/io/sentry/protocol/SentryTransaction.java @@ -12,6 +12,7 @@ import io.sentry.Span; import io.sentry.SpanContext; import io.sentry.SpanStatus; +import io.sentry.TracesSamplingDecision; import io.sentry.util.Objects; import io.sentry.vendor.gson.stream.JsonToken; import java.io.IOException; @@ -73,7 +74,7 @@ public SentryTransaction(final @NotNull SentryTracer sentryTracer) { tracerContext.getParentSpanId(), tracerContext.getOperation(), tracerContext.getDescription(), - tracerContext.getSampled(), + tracerContext.getSamplingDecision(), tracerContext.getStatus())); for (final Map.Entry tag : tracerContext.getTags().entrySet()) { this.setTag(tag.getKey(), tag.getValue()); @@ -131,8 +132,21 @@ public boolean isFinished() { } public boolean isSampled() { + final @Nullable TracesSamplingDecision samplingDecsion = getSamplingDecision(); + if (samplingDecsion == null) { + return false; + } + + return samplingDecsion.getSampled(); + } + + public @Nullable TracesSamplingDecision getSamplingDecision() { final SpanContext trace = this.getContexts().getTrace(); - return trace != null && Boolean.TRUE.equals(trace.getSampled()); + if (trace == null) { + return null; + } + + return trace.getSamplingDecision(); } public @NotNull Map getMeasurements() { diff --git a/sentry/src/test/java/io/sentry/HubTest.kt b/sentry/src/test/java/io/sentry/HubTest.kt index 5f64108b6f..5ab9f53b03 100644 --- a/sentry/src/test/java/io/sentry/HubTest.kt +++ b/sentry/src/test/java/io/sentry/HubTest.kt @@ -1204,7 +1204,7 @@ class HubTest { val mockClient = mock() sut.bindClient(mockClient) - val sentryTracer = SentryTracer(TransactionContext("name", "op", true), sut) + val sentryTracer = SentryTracer(TransactionContext("name", "op", TracesSamplingDecision(true)), sut) sentryTracer.finish() val traceContext = sentryTracer.traceContext() verify(mockClient).captureTransaction(any(), eq(traceContext), any(), eq(null), anyOrNull()) @@ -1218,14 +1218,14 @@ class HubTest { it.setTransactionProfiler(mockTransactionProfiler) } // Transaction is not sampled, so it should not be profiled - val contexts = TransactionContext("name", "op", false) + val contexts = TransactionContext("name", "op", TracesSamplingDecision(false)) val transaction = hub.startTransaction(contexts) transaction.finish() verify(mockTransactionProfiler, never()).onTransactionStart(anyOrNull()) verify(mockTransactionProfiler, never()).onTransactionFinish(anyOrNull()) // Transaction is sampled, so it should be profiled - val sampledContexts = TransactionContext("name", "op", true) + val sampledContexts = TransactionContext("name", "op", TracesSamplingDecision(true)) val sampledTransaction = hub.startTransaction(sampledContexts) sampledTransaction.finish() verify(mockTransactionProfiler).onTransactionStart(anyOrNull()) @@ -1239,7 +1239,7 @@ class HubTest { it.isProfilingEnabled = false it.setTransactionProfiler(mockTransactionProfiler) } - val contexts = TransactionContext("name", "op", true) + val contexts = TransactionContext("name", "op", TracesSamplingDecision(true)) val transaction = hub.startTransaction(contexts) transaction.finish() verify(mockTransactionProfiler, never()).onTransactionStart(anyOrNull()) @@ -1257,7 +1257,7 @@ class HubTest { sut.bindClient(mockClient) whenever(mockClient.captureTransaction(anyOrNull(), anyOrNull(), anyOrNull(), anyOrNull(), anyOrNull())).thenReturn(SentryId()) - val sentryTracer = SentryTracer(TransactionContext("name", "op", true), sut) + val sentryTracer = SentryTracer(TransactionContext("name", "op", TracesSamplingDecision(true)), sut) sentryTracer.finish() assertEquals(SentryId.EMPTY_ID, sut.lastEventId) } @@ -1272,7 +1272,7 @@ class HubTest { val mockClient = mock() sut.bindClient(mockClient) - val sentryTracer = SentryTracer(TransactionContext("name", "op", true), sut) + val sentryTracer = SentryTracer(TransactionContext("name", "op", TracesSamplingDecision(true)), sut) sut.captureTransaction(SentryTransaction(sentryTracer), null as TraceContext?) verify(mockClient, never()).captureTransaction(any(), any(), any(), eq(null), anyOrNull()) } @@ -1287,7 +1287,7 @@ class HubTest { val mockClient = mock() sut.bindClient(mockClient) - val sentryTracer = SentryTracer(TransactionContext("name", "op", false), sut) + val sentryTracer = SentryTracer(TransactionContext("name", "op", TracesSamplingDecision(false)), sut) sentryTracer.finish() val traceContext = sentryTracer.traceContext() verify(mockClient, never()).captureTransaction(any(), eq(traceContext), any(), eq(null), anyOrNull()) @@ -1303,7 +1303,7 @@ class HubTest { val mockClient = mock() sut.bindClient(mockClient) - val sentryTracer = SentryTracer(TransactionContext("name", "op", false), sut) + val sentryTracer = SentryTracer(TransactionContext("name", "op", TracesSamplingDecision(false)), sut) sentryTracer.finish() assertClientReport( @@ -1397,7 +1397,7 @@ class HubTest { it.tracesSampleRate = null it.tracesSampler = null } - val transaction = hub.startTransaction(TransactionContext("name", "op", true)) + val transaction = hub.startTransaction(TransactionContext("name", "op", TracesSamplingDecision(true))) assertTrue(transaction is NoOpTransaction) } //endregion diff --git a/sentry/src/test/java/io/sentry/SentryTracerTest.kt b/sentry/src/test/java/io/sentry/SentryTracerTest.kt index d03da63d71..527a2c9af0 100644 --- a/sentry/src/test/java/io/sentry/SentryTracerTest.kt +++ b/sentry/src/test/java/io/sentry/SentryTracerTest.kt @@ -40,10 +40,10 @@ class SentryTracerTest { idleTimeout: Long? = null, trimEnd: Boolean = false, transactionFinishedCallback: TransactionFinishedCallback? = null, - sampled: Boolean? = null + samplingDecision: TracesSamplingDecision? = null ): SentryTracer { optionsConfiguration.configure(options) - return SentryTracer(TransactionContext("name", "op", sampled), hub, startTimestamp, waitForChildren, idleTimeout, trimEnd, transactionFinishedCallback) + return SentryTracer(TransactionContext("name", "op", samplingDecision), hub, startTimestamp, waitForChildren, idleTimeout, trimEnd, transactionFinishedCallback) } } @@ -144,7 +144,7 @@ class SentryTracerTest { val tracer = fixture.getSut(optionsConfiguration = { it.isProfilingEnabled = true it.setTransactionProfiler(transactionProfiler) - }, sampled = true) + }, samplingDecision = TracesSamplingDecision(true)) tracer.finish() verify(transactionProfiler).onTransactionFinish(any()) } @@ -188,7 +188,7 @@ class SentryTracerTest { @Test fun `not sampled spans are filtered out`() { - val tracer = fixture.getSut(sampled = true) + val tracer = fixture.getSut(samplingDecision = TracesSamplingDecision(true)) tracer.startChild("op1") val span = tracer.startChild("op2") span.spanContext.sampled = false @@ -459,7 +459,7 @@ class SentryTracerTest { @Test fun `finishing unfinished spans with the transaction timestamp`() { - val transaction = fixture.getSut(sampled = true) + val transaction = fixture.getSut(samplingDecision = TracesSamplingDecision(true)) val span = transaction.startChild("op") as Span transaction.startChild("op2") transaction.finish(SpanStatus.INVALID_ARGUMENT) @@ -551,7 +551,7 @@ class SentryTracerTest { @Test fun `sets ITransaction data as extra in SentryTransaction`() { - val transaction = fixture.getSut(sampled = true) + val transaction = fixture.getSut(samplingDecision = TracesSamplingDecision(true)) transaction.setData("key", "val") transaction.finish() verify(fixture.hub).captureTransaction( @@ -566,7 +566,7 @@ class SentryTracerTest { @Test fun `sets Span data as data in SentrySpan`() { - val transaction = fixture.getSut(sampled = true) + val transaction = fixture.getSut(samplingDecision = TracesSamplingDecision(true)) val span = transaction.startChild("op") span.setData("key", "val") span.finish() @@ -666,7 +666,7 @@ class SentryTracerTest { @Test fun `when trimEnd, trims idle transaction time to the latest child timestamp`() { - val transaction = fixture.getSut(waitForChildren = true, idleTimeout = 50, trimEnd = true, sampled = true) + val transaction = fixture.getSut(waitForChildren = true, idleTimeout = 50, trimEnd = true, samplingDecision = TracesSamplingDecision(true)) val span = transaction.startChild("op") span.finish() @@ -692,19 +692,19 @@ class SentryTracerTest { @Test fun `timer is created if idle timeout is set`() { - val transaction = fixture.getSut(waitForChildren = true, idleTimeout = 50, trimEnd = true, sampled = true) + val transaction = fixture.getSut(waitForChildren = true, idleTimeout = 50, trimEnd = true, samplingDecision = TracesSamplingDecision(true)) assertNotNull(transaction.timer) } @Test fun `timer is not created if idle timeout is not set`() { - val transaction = fixture.getSut(waitForChildren = true, idleTimeout = null, trimEnd = true, sampled = true) + val transaction = fixture.getSut(waitForChildren = true, idleTimeout = null, trimEnd = true, samplingDecision = TracesSamplingDecision(true)) assertNull(transaction.timer) } @Test fun `timer is cancelled on finish`() { - val transaction = fixture.getSut(waitForChildren = true, idleTimeout = 50, trimEnd = true, sampled = true) + val transaction = fixture.getSut(waitForChildren = true, idleTimeout = 50, trimEnd = true, samplingDecision = TracesSamplingDecision(true)) assertNotNull(transaction.timer) transaction.finish(SpanStatus.OK) assertNull(transaction.timer) diff --git a/sentry/src/test/java/io/sentry/SpanTest.kt b/sentry/src/test/java/io/sentry/SpanTest.kt index a7e1931d4d..da090abe68 100644 --- a/sentry/src/test/java/io/sentry/SpanTest.kt +++ b/sentry/src/test/java/io/sentry/SpanTest.kt @@ -112,7 +112,7 @@ class SpanTest { val span = Span( traceId, parentSpanId, SentryTracer( - TransactionContext("name", "op", true), fixture.hub + TransactionContext("name", "op", TracesSamplingDecision(true)), fixture.hub ), "op", fixture.hub ) diff --git a/sentry/src/test/java/io/sentry/TraceContextSerializationTest.kt b/sentry/src/test/java/io/sentry/TraceContextSerializationTest.kt index 30cf6d262e..15aa989dfd 100644 --- a/sentry/src/test/java/io/sentry/TraceContextSerializationTest.kt +++ b/sentry/src/test/java/io/sentry/TraceContextSerializationTest.kt @@ -62,7 +62,8 @@ class TraceContextSerializationTest { environment = "prod" release = "1.0.17" tracesSampleRate = sRate - } + }, + TracesSamplingDecision(sRate > 0.5, sRate) ) } diff --git a/sentry/src/test/java/io/sentry/TracesSamplerTest.kt b/sentry/src/test/java/io/sentry/TracesSamplerTest.kt index 595d853181..4d4b302d77 100644 --- a/sentry/src/test/java/io/sentry/TracesSamplerTest.kt +++ b/sentry/src/test/java/io/sentry/TracesSamplerTest.kt @@ -4,6 +4,7 @@ import com.nhaarman.mockitokotlin2.mock import com.nhaarman.mockitokotlin2.whenever import java.security.SecureRandom import kotlin.test.Test +import kotlin.test.assertEquals import kotlin.test.assertFalse import kotlin.test.assertTrue @@ -30,25 +31,43 @@ class TracesSamplerTest { @Test fun `when tracesSampleRate is set and random returns greater number returns false`() { val sampler = fixture.getSut(randomResult = 0.9, tracesSampleRate = 0.2) - assertFalse(sampler.sample(SamplingContext(TransactionContext("name", "op"), null))) + val samplingDecision = sampler.sample(SamplingContext(TransactionContext("name", "op"), null)) + assertFalse(samplingDecision.sampled) + assertEquals(0.2, samplingDecision.sampleRate) } @Test fun `when tracesSampleRate is set and random returns lower number returns true`() { val sampler = fixture.getSut(randomResult = 0.1, tracesSampleRate = 0.2) - assertTrue(sampler.sample(SamplingContext(TransactionContext("name", "op"), null))) + val samplingDecision = sampler.sample(SamplingContext(TransactionContext("name", "op"), null)) + assertTrue(samplingDecision.sampled) + assertEquals(0.2, samplingDecision.sampleRate) } @Test fun `when tracesSampleRate is not set, tracesSampler is set and random returns lower number returns false`() { val sampler = fixture.getSut(randomResult = 0.1, tracesSamplerResult = 0.2) - assertTrue(sampler.sample(SamplingContext(TransactionContext("name", "op"), CustomSamplingContext()))) + val samplingDecision = sampler.sample( + SamplingContext( + TransactionContext("name", "op"), + CustomSamplingContext() + ) + ) + assertTrue(samplingDecision.sampled) + assertEquals(0.2, samplingDecision.sampleRate) } @Test fun `when tracesSampleRate is not set, tracesSampler is set and random returns greater number returns false`() { val sampler = fixture.getSut(randomResult = 0.9, tracesSamplerResult = 0.2) - assertFalse(sampler.sample(SamplingContext(TransactionContext("name", "op"), CustomSamplingContext()))) + val samplingDecision = sampler.sample( + SamplingContext( + TransactionContext("name", "op"), + CustomSamplingContext() + ) + ) + assertFalse(samplingDecision.sampled) + assertEquals(0.2, samplingDecision.sampleRate) } @Test @@ -56,19 +75,40 @@ class TracesSamplerTest { val sampler = fixture.getSut(tracesSamplerResult = null) val transactionContextParentSampled = TransactionContext("name", "op") transactionContextParentSampled.parentSampled = true - assertTrue(sampler.sample(SamplingContext(transactionContextParentSampled, CustomSamplingContext()))) + val samplingDecision = sampler.sample( + SamplingContext( + transactionContextParentSampled, + CustomSamplingContext() + ) + ) + assertTrue(samplingDecision.sampled) + assertEquals(1.0, samplingDecision.sampleRate) } @Test fun `when tracesSampler returns null and tracesSampleRate is set sampler uses it as a sampling decision`() { val sampler = fixture.getSut(randomResult = 0.1, tracesSampleRate = 0.2, tracesSamplerResult = null) - assertTrue(sampler.sample(SamplingContext(TransactionContext("name", "op"), CustomSamplingContext()))) + val samplingDecision = sampler.sample( + SamplingContext( + TransactionContext("name", "op"), + CustomSamplingContext() + ) + ) + assertTrue(samplingDecision.sampled) + assertEquals(0.2, samplingDecision.sampleRate) } @Test fun `when tracesSampleRate is not set, and tracesSampler is not set returns false`() { val sampler = fixture.getSut(randomResult = 0.1) - assertFalse(sampler.sample(SamplingContext(TransactionContext("name", "op"), CustomSamplingContext()))) + val samplingDecision = sampler.sample( + SamplingContext( + TransactionContext("name", "op"), + CustomSamplingContext() + ) + ) + assertFalse(samplingDecision.sampled) + assertEquals(0.0, samplingDecision.sampleRate) } @Test @@ -76,10 +116,25 @@ class TracesSamplerTest { val sampler = fixture.getSut() val transactionContextParentNotSampled = TransactionContext("name", "op") transactionContextParentNotSampled.parentSampled = false - assertFalse(sampler.sample(SamplingContext(transactionContextParentNotSampled, CustomSamplingContext()))) + val samplingDecision = sampler.sample( + SamplingContext( + transactionContextParentNotSampled, + CustomSamplingContext() + ) + ) + assertFalse(samplingDecision.sampled) + assertEquals(0.0, samplingDecision.sampleRate) + val transactionContextParentSampled = TransactionContext("name", "op") transactionContextParentSampled.parentSampled = true - assertTrue(sampler.sample(SamplingContext(transactionContextParentSampled, CustomSamplingContext()))) + val samplingDecisionParentSampled = sampler.sample( + SamplingContext( + transactionContextParentSampled, + CustomSamplingContext() + ) + ) + assertTrue(samplingDecisionParentSampled.sampled) + assertEquals(1.0, samplingDecisionParentSampled.sampleRate) } @Test @@ -87,9 +142,16 @@ class TracesSamplerTest { val sampler = fixture.getSut() val transactionContextNotSampled = TransactionContext("name", "op") transactionContextNotSampled.sampled = false - assertFalse(sampler.sample(SamplingContext(transactionContextNotSampled, CustomSamplingContext()))) + val samplingDecision = + sampler.sample(SamplingContext(transactionContextNotSampled, CustomSamplingContext())) + assertFalse(samplingDecision.sampled) + assertEquals(0.0, samplingDecision.sampleRate) + val transactionContextSampled = TransactionContext("name", "op") transactionContextSampled.sampled = true - assertTrue(sampler.sample(SamplingContext(transactionContextSampled, CustomSamplingContext()))) + val samplingDecisionContextSampled = + sampler.sample(SamplingContext(transactionContextSampled, CustomSamplingContext())) + assertTrue(samplingDecisionContextSampled.sampled) + assertEquals(1.0, samplingDecisionContextSampled.sampleRate) } } diff --git a/sentry/src/test/java/io/sentry/protocol/SpanContextSerializationTest.kt b/sentry/src/test/java/io/sentry/protocol/SpanContextSerializationTest.kt index 7ca26d1b2c..9d81d93f7c 100644 --- a/sentry/src/test/java/io/sentry/protocol/SpanContextSerializationTest.kt +++ b/sentry/src/test/java/io/sentry/protocol/SpanContextSerializationTest.kt @@ -9,6 +9,7 @@ import io.sentry.JsonSerializable import io.sentry.SpanContext import io.sentry.SpanId import io.sentry.SpanStatus +import io.sentry.TracesSamplingDecision import org.junit.Test import java.io.StringReader import java.io.StringWriter @@ -26,7 +27,7 @@ class SpanContextSerializationTest { SpanId("bf6b582d-8ce3-412b-a334-f4c5539b9602"), "e481581d-35a4-4e97-8a1c-b554bf49f23e", SpanId("c7500f2a-d4e6-4f5f-a0f4-6bb67e98d5a2"), - false + TracesSamplingDecision(false) ).apply { description = "c204b6c7-9753-4d45-927d-b19789bfc9a5" status = SpanStatus.RESOURCE_EXHAUSTED From 59d32d88940611e2c561c4d80905803fbd899ab6 Mon Sep 17 00:00:00 2001 From: Alexander Dinauer Date: Tue, 28 Jun 2022 14:51:25 +0200 Subject: [PATCH 2/9] Do not replace null with true/false --- sentry/src/main/java/io/sentry/SpanContext.java | 6 +++++- sentry/src/main/java/io/sentry/TracesSampler.java | 7 ++++--- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/sentry/src/main/java/io/sentry/SpanContext.java b/sentry/src/main/java/io/sentry/SpanContext.java index 3bb77a74d7..d879510b84 100644 --- a/sentry/src/main/java/io/sentry/SpanContext.java +++ b/sentry/src/main/java/io/sentry/SpanContext.java @@ -169,7 +169,11 @@ public SpanId getParentSpanId() { @ApiStatus.Internal public void setSampled(final @Nullable Boolean sampled) { - this.setSamplingDecision(new TracesSamplingDecision(sampled == null ? false : sampled)); + if (sampled == null) { + setSamplingDecision(null); + } else { + setSamplingDecision(new TracesSamplingDecision(sampled)); + } } @ApiStatus.Internal diff --git a/sentry/src/main/java/io/sentry/TracesSampler.java b/sentry/src/main/java/io/sentry/TracesSampler.java index 08fef2846b..0658b820f0 100644 --- a/sentry/src/main/java/io/sentry/TracesSampler.java +++ b/sentry/src/main/java/io/sentry/TracesSampler.java @@ -34,9 +34,10 @@ TracesSamplingDecision sample(final @NotNull SamplingContext samplingContext) { } } - Boolean parentSampled = samplingContext.getTransactionContext().getParentSampled(); - if (parentSampled != null) { - return new TracesSamplingDecision(parentSampled); + TracesSamplingDecision parentSamplingDecision = + samplingContext.getTransactionContext().getParentSamplingDecision(); + if (parentSamplingDecision != null) { + return parentSamplingDecision; } Double tracesSampleRateFromOptions = options.getTracesSampleRate(); From 2f95fd15955a7b7dfebbd3b1e9e791d77d381b6d Mon Sep 17 00:00:00 2001 From: Alexander Dinauer Date: Wed, 29 Jun 2022 10:13:59 +0200 Subject: [PATCH 3/9] Restore sample rate in OutboxSender --- .../src/main/java/io/sentry/OutboxSender.java | 43 +++++++++++++++-- .../test/java/io/sentry/OutboxSenderTest.kt | 47 +++++++++++++++++++ .../envelope-transaction-with-sample-rate.txt | 3 ++ 3 files changed, 89 insertions(+), 4 deletions(-) create mode 100644 sentry/src/test/resources/envelope-transaction-with-sample-rate.txt diff --git a/sentry/src/main/java/io/sentry/OutboxSender.java b/sentry/src/main/java/io/sentry/OutboxSender.java index 51049b6d4c..3f6d19c409 100644 --- a/sentry/src/main/java/io/sentry/OutboxSender.java +++ b/sentry/src/main/java/io/sentry/OutboxSender.java @@ -161,12 +161,17 @@ private void processEnvelope(final @NotNull SentryEnvelope envelope, final @NotN continue; } + // if there is no trace context header we also won't send it to Sentry + final @Nullable TraceContext traceContext = envelope.getHeader().getTraceContext(); if (transaction.getContexts().getTrace() != null) { - // Hint: Set sampled in order for the transaction not to be dropped, as this is a - // transient property. - transaction.getContexts().getTrace().setSampled(true); + // Hint: Set sampling decision in order for the transaction not to be dropped, as this + // is a transient property. + transaction + .getContexts() + .getTrace() + .setSamplingDecision(extractSamplingDecision(traceContext)); } - hub.captureTransaction(transaction, envelope.getHeader().getTraceContext(), hint); + hub.captureTransaction(transaction, traceContext, hint); logItemCaptured(currentItem); if (!waitFlush(hint)) { @@ -216,6 +221,36 @@ private void processEnvelope(final @NotNull SentryEnvelope envelope, final @NotN } } + private @NotNull TracesSamplingDecision extractSamplingDecision( + final @Nullable TraceContext traceContext) { + if (traceContext != null) { + final @Nullable String sampleRateString = traceContext.getSampleRate(); + if (sampleRateString != null) { + try { + final Double sampleRate = Double.parseDouble(sampleRateString); + if (sampleRate.isInfinite() + || sampleRate.isNaN() + || sampleRate < 0.0 + || sampleRate > 1.0) { + logger.log( + SentryLevel.ERROR, + "Invalid sample rate parsed from TraceContext: %s", + sampleRateString); + } else { + return new TracesSamplingDecision(true, sampleRate); + } + } catch (Exception e) { + logger.log( + SentryLevel.ERROR, + "Unable to parse sample rate from TraceContext: %s", + sampleRateString); + } + } + } + + return new TracesSamplingDecision(true); + } + private void logEnvelopeItemNull(final @NotNull SentryEnvelopeItem item, int itemIndex) { logger.log( SentryLevel.ERROR, diff --git a/sentry/src/test/java/io/sentry/OutboxSenderTest.kt b/sentry/src/test/java/io/sentry/OutboxSenderTest.kt index 3f14f3cf5c..3e731c76ff 100644 --- a/sentry/src/test/java/io/sentry/OutboxSenderTest.kt +++ b/sentry/src/test/java/io/sentry/OutboxSenderTest.kt @@ -132,6 +132,53 @@ class OutboxSenderTest { verify(fixture.logger, never()).log(eq(SentryLevel.ERROR), any(), any()) } + @Test + fun `restores sampleRate`() { + fixture.envelopeReader = EnvelopeReader(JsonSerializer(fixture.options)) + whenever(fixture.options.maxSpans).thenReturn(1000) + whenever(fixture.hub.options).thenReturn(fixture.options) + whenever(fixture.options.transactionProfiler).thenReturn(NoOpTransactionProfiler.getInstance()) + + val transactionContext = TransactionContext("fixture-name", "http") + transactionContext.description = "fixture-request" + transactionContext.status = SpanStatus.OK + transactionContext.setTag("fixture-tag", "fixture-value") + transactionContext.samplingDecision = TracesSamplingDecision(true, 0.00000021) + + val sentryTracer = SentryTracer(transactionContext, fixture.hub) + val span = sentryTracer.startChild("child") + span.finish(SpanStatus.OK) + sentryTracer.finish() + + val sentryTracerSpy = spy(sentryTracer) + whenever(sentryTracerSpy.eventId).thenReturn(SentryId("3367f5196c494acaae85bbbd535379ac")) + + val expected = SentryTransaction(sentryTracerSpy) + whenever(fixture.serializer.deserialize(any(), eq(SentryTransaction::class.java))).thenReturn(expected) + + val sut = fixture.getSut() + val path = getTempEnvelope(fileName = "envelope-transaction-with-sample-rate.txt") + assertTrue(File(path).exists()) + + val hints = HintUtils.createWithTypeCheckHint(mock()) + sut.processEnvelopeFile(path, hints) + + verify(fixture.hub).captureTransaction( + check { + assertEquals(expected, it) + assertTrue(it.isSampled) + assertEquals(0.00000021, it.samplingDecision?.sampleRate) + assertTrue(it.samplingDecision!!.sampled) + }, + any(), any() + ) + assertFalse(File(path).exists()) + + // Additionally make sure we have no errors logged + verify(fixture.logger, never()).log(eq(SentryLevel.ERROR), any(), any()) + verify(fixture.logger, never()).log(eq(SentryLevel.ERROR), any(), any()) + } + @Test fun `when parser is EnvelopeReader and serializer returns SentryEnvelope, event captured, file is deleted `() { fixture.envelopeReader = EnvelopeReader(JsonSerializer(fixture.options)) diff --git a/sentry/src/test/resources/envelope-transaction-with-sample-rate.txt b/sentry/src/test/resources/envelope-transaction-with-sample-rate.txt new file mode 100644 index 0000000000..ae4746ff8f --- /dev/null +++ b/sentry/src/test/resources/envelope-transaction-with-sample-rate.txt @@ -0,0 +1,3 @@ +{"event_id":"3367f5196c494acaae85bbbd535379ac","trace":{"trace_id":"b156a475de54423d9c1571df97ec7eb6","public_key":"key","sample_rate":"0.00000021"}} +{"type":"transaction","length":640,"content_type":"application/json"} +{"transaction":"a-transaction","type":"transaction","start_timestamp":"2020-10-23T10:24:01.791Z","timestamp":"2020-10-23T10:24:02.791Z","event_id":"3367f5196c494acaae85bbbd535379ac","contexts":{"trace":{"trace_id":"b156a475de54423d9c1571df97ec7eb6","span_id":"0a53026963414893","op":"http","status":"ok"},"custom":{"some-key":"some-value"}},"spans":[{"start_timestamp":"2021-03-05T08:51:12.838Z","timestamp":"2021-03-05T08:51:12.949Z","trace_id":"2b099185293344a5bfdd7ad89ebf9416","span_id":"5b95c29a5ded4281","parent_span_id":"a3b2d1d58b344b07","op":"PersonService.create","description":"desc","status":"aborted","tags":{"name":"value"}}]} From eaf947e85972fc436be77c25ca6da8ddd7bff511 Mon Sep 17 00:00:00 2001 From: Alexander Dinauer Date: Wed, 29 Jun 2022 10:16:57 +0200 Subject: [PATCH 4/9] Remove fallback for sampling decision from TraceContext --- sentry/src/main/java/io/sentry/TraceContext.java | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/sentry/src/main/java/io/sentry/TraceContext.java b/sentry/src/main/java/io/sentry/TraceContext.java index f30e2e0a8f..67969a8d79 100644 --- a/sentry/src/main/java/io/sentry/TraceContext.java +++ b/sentry/src/main/java/io/sentry/TraceContext.java @@ -63,7 +63,7 @@ public final class TraceContext implements JsonUnknown, JsonSerializable { user != null ? user.getId() : null, user != null ? getSegment(user) : null, transaction.getName(), - sampleRateToString(sampleRate(sentryOptions, samplingDecision))); + sampleRateToString(sampleRate(samplingDecision))); } private static @Nullable String getSegment(final @NotNull User user) { @@ -75,12 +75,9 @@ public final class TraceContext implements JsonUnknown, JsonSerializable { } } - private static @Nullable Double sampleRate( - @NotNull SentryOptions sentryOptions, @Nullable TracesSamplingDecision samplingDecision) { - // TODO does this fallback even make sense? could also just write 1.0 as - // options.tracesSampleRate should have been written to a sampling decision + private static @Nullable Double sampleRate(@Nullable TracesSamplingDecision samplingDecision) { if (samplingDecision == null) { - return sentryOptions.getTracesSampleRate(); + return null; } return samplingDecision.getSampleRate(); From 0f9bbdaf8292bafef20a30972f327907c9409b33 Mon Sep 17 00:00:00 2001 From: Alexander Dinauer Date: Wed, 29 Jun 2022 15:36:33 +0200 Subject: [PATCH 5/9] Remove sample rate fallback from TracesSamplingDecision --- sentry/api/sentry.api | 9 ++ .../src/main/java/io/sentry/OutboxSender.java | 5 +- .../main/java/io/sentry/SampleRateUtil.java | 33 ++++++ .../main/java/io/sentry/SentryOptions.java | 4 +- .../src/main/java/io/sentry/TraceContext.java | 103 ++++++++++++++++-- .../io/sentry/TracesSamplingDecision.java | 10 +- .../test/java/io/sentry/SampleRateUtilTest.kt | 103 ++++++++++++++++++ .../sentry/TraceContextSerializationTest.kt | 9 ++ .../test/java/io/sentry/TracesSamplerTest.kt | 13 ++- .../resources/json/trace_state_legacy.json | 12 ++ .../json/trace_state_no_sample_rate.json | 9 ++ 11 files changed, 279 insertions(+), 31 deletions(-) create mode 100644 sentry/src/main/java/io/sentry/SampleRateUtil.java create mode 100644 sentry/src/test/java/io/sentry/SampleRateUtilTest.kt create mode 100644 sentry/src/test/resources/json/trace_state_legacy.json create mode 100644 sentry/src/test/resources/json/trace_state_no_sample_rate.json diff --git a/sentry/api/sentry.api b/sentry/api/sentry.api index a6f26099a4..430fc8dcad 100644 --- a/sentry/api/sentry.api +++ b/sentry/api/sentry.api @@ -795,6 +795,14 @@ public final class io/sentry/RequestDetails { public fun getUrl ()Ljava/net/URL; } +public final class io/sentry/SampleRateUtil { + public fun ()V + public static fun isValidSampleRate (Ljava/lang/Double;)Z + public static fun isValidSampleRate (Ljava/lang/Double;Z)Z + public static fun isValidTracesSampleRate (Ljava/lang/Double;)Z + public static fun isValidTracesSampleRate (Ljava/lang/Double;Z)Z +} + public final class io/sentry/SamplingContext { public fun (Lio/sentry/TransactionContext;Lio/sentry/CustomSamplingContext;)V public fun getCustomSamplingContext ()Lio/sentry/CustomSamplingContext; @@ -1650,6 +1658,7 @@ public final class io/sentry/TraceContext$JsonKeys { public static final field SAMPLE_RATE Ljava/lang/String; public static final field TRACE_ID Ljava/lang/String; public static final field TRANSACTION Ljava/lang/String; + public static final field USER Ljava/lang/String; public static final field USER_ID Ljava/lang/String; public static final field USER_SEGMENT Ljava/lang/String; public fun ()V diff --git a/sentry/src/main/java/io/sentry/OutboxSender.java b/sentry/src/main/java/io/sentry/OutboxSender.java index 3f6d19c409..7017459b58 100644 --- a/sentry/src/main/java/io/sentry/OutboxSender.java +++ b/sentry/src/main/java/io/sentry/OutboxSender.java @@ -228,10 +228,7 @@ private void processEnvelope(final @NotNull SentryEnvelope envelope, final @NotN if (sampleRateString != null) { try { final Double sampleRate = Double.parseDouble(sampleRateString); - if (sampleRate.isInfinite() - || sampleRate.isNaN() - || sampleRate < 0.0 - || sampleRate > 1.0) { + if (!SampleRateUtil.isValidTracesSampleRate(sampleRate, false)) { logger.log( SentryLevel.ERROR, "Invalid sample rate parsed from TraceContext: %s", diff --git a/sentry/src/main/java/io/sentry/SampleRateUtil.java b/sentry/src/main/java/io/sentry/SampleRateUtil.java new file mode 100644 index 0000000000..ec521f52fd --- /dev/null +++ b/sentry/src/main/java/io/sentry/SampleRateUtil.java @@ -0,0 +1,33 @@ +package io.sentry; + +import org.jetbrains.annotations.ApiStatus; +import org.jetbrains.annotations.Nullable; + +@ApiStatus.Internal +public final class SampleRateUtil { + + public static boolean isValidSampleRate(@Nullable Double sampleRate) { + return isValidSampleRate(sampleRate, true); + } + + public static boolean isValidSampleRate(@Nullable Double sampleRate, boolean allowNull) { + if (sampleRate == null) { + return allowNull; + } + + return !(sampleRate.isNaN() || (sampleRate > 1.0 || sampleRate <= 0.0)); + } + + public static boolean isValidTracesSampleRate(@Nullable Double tracesSampleRate) { + return isValidTracesSampleRate(tracesSampleRate, true); + } + + public static boolean isValidTracesSampleRate( + @Nullable Double tracesSampleRate, boolean allowNull) { + if (tracesSampleRate == null) { + return allowNull; + } + + return !(tracesSampleRate.isNaN() || (tracesSampleRate > 1.0 || tracesSampleRate < 0.0)); + } +} diff --git a/sentry/src/main/java/io/sentry/SentryOptions.java b/sentry/src/main/java/io/sentry/SentryOptions.java index ea07b7bc9a..b1eb901113 100644 --- a/sentry/src/main/java/io/sentry/SentryOptions.java +++ b/sentry/src/main/java/io/sentry/SentryOptions.java @@ -729,7 +729,7 @@ public void setProxy(@Nullable Proxy proxy) { * @param sampleRate the sample rate */ public void setSampleRate(Double sampleRate) { - if (sampleRate != null && (sampleRate > 1.0 || sampleRate <= 0.0)) { + if (!SampleRateUtil.isValidSampleRate(sampleRate)) { throw new IllegalArgumentException( "The value " + sampleRate @@ -753,7 +753,7 @@ public void setSampleRate(Double sampleRate) { * @param tracesSampleRate the sample rate */ public void setTracesSampleRate(final @Nullable Double tracesSampleRate) { - if (tracesSampleRate != null && (tracesSampleRate > 1.0 || tracesSampleRate < 0.0)) { + if (!SampleRateUtil.isValidTracesSampleRate(tracesSampleRate)) { throw new IllegalArgumentException( "The value " + tracesSampleRate diff --git a/sentry/src/main/java/io/sentry/TraceContext.java b/sentry/src/main/java/io/sentry/TraceContext.java index 67969a8d79..3486502ff7 100644 --- a/sentry/src/main/java/io/sentry/TraceContext.java +++ b/sentry/src/main/java/io/sentry/TraceContext.java @@ -84,22 +84,12 @@ public final class TraceContext implements JsonUnknown, JsonSerializable { } private static @Nullable String sampleRateToString(@Nullable Double sampleRateAsDouble) { - if (sampleRateAsDouble == null) { - return null; - } - - if (sampleRateAsDouble == null - || sampleRateAsDouble.isNaN() - || sampleRateAsDouble.isInfinite()) { - return null; - } - - if (sampleRateAsDouble < 0.0 || sampleRateAsDouble > 1.0) { + if (!SampleRateUtil.isValidTracesSampleRate(sampleRateAsDouble, false)) { return null; } DecimalFormat df = - new DecimalFormat("#.################", DecimalFormatSymbols.getInstance(Locale.US)); + new DecimalFormat("#.################", DecimalFormatSymbols.getInstance(Locale.ROOT)); return df.format(sampleRateAsDouble); } @@ -150,6 +140,82 @@ public final class TraceContext implements JsonUnknown, JsonSerializable { return baggage; } + /** @deprecated only here to support parsing legacy JSON with non flattened user */ + @Deprecated + private static final class TraceContextUser implements JsonUnknown { + private @Nullable String id; + private @Nullable String segment; + + @SuppressWarnings("unused") + private @Nullable Map unknown; + + private TraceContextUser(final @Nullable String id, final @Nullable String segment) { + this.id = id; + this.segment = segment; + } + + public @Nullable String getId() { + return id; + } + + public @Nullable String getSegment() { + return segment; + } + + // region json + + @Nullable + @Override + public Map getUnknown() { + return unknown; + } + + @Override + public void setUnknown(@Nullable Map unknown) { + this.unknown = unknown; + } + + public static final class JsonKeys { + public static final String ID = "id"; + public static final String SEGMENT = "segment"; + } + + public static final class Deserializer implements JsonDeserializer { + @Override + public @NotNull TraceContextUser deserialize( + @NotNull JsonObjectReader reader, @NotNull ILogger logger) throws Exception { + reader.beginObject(); + + String id = null; + String segment = null; + Map unknown = null; + while (reader.peek() == JsonToken.NAME) { + final String nextName = reader.nextName(); + switch (nextName) { + case TraceContextUser.JsonKeys.ID: + id = reader.nextStringOrNull(); + break; + case TraceContextUser.JsonKeys.SEGMENT: + segment = reader.nextStringOrNull(); + break; + default: + if (unknown == null) { + unknown = new ConcurrentHashMap<>(); + } + reader.nextUnknown(logger, unknown, nextName); + break; + } + } + TraceContextUser traceStateUser = new TraceContextUser(id, segment); + traceStateUser.setUnknown(unknown); + reader.endObject(); + return traceStateUser; + } + } + + // endregion + } + // region json @Nullable @@ -168,6 +234,7 @@ public static final class JsonKeys { public static final String PUBLIC_KEY = "public_key"; public static final String RELEASE = "release"; public static final String ENVIRONMENT = "environment"; + public static final String USER = "user"; public static final String USER_ID = "user_id"; public static final String USER_SEGMENT = "user_segment"; public static final String TRANSACTION = "transaction"; @@ -218,6 +285,7 @@ public static final class Deserializer implements JsonDeserializer String publicKey = null; String release = null; String environment = null; + TraceContextUser user = null; String userId = null; String userSegment = null; String transaction = null; @@ -239,6 +307,9 @@ public static final class Deserializer implements JsonDeserializer case TraceContext.JsonKeys.ENVIRONMENT: environment = reader.nextStringOrNull(); break; + case TraceContext.JsonKeys.USER: + user = reader.nextOrNull(logger, new TraceContextUser.Deserializer()); + break; case TraceContext.JsonKeys.USER_ID: userId = reader.nextStringOrNull(); break; @@ -265,6 +336,14 @@ public static final class Deserializer implements JsonDeserializer if (publicKey == null) { throw missingRequiredFieldException(TraceContext.JsonKeys.PUBLIC_KEY, logger); } + if (user != null) { + if (userId == null) { + userId = user.getId(); + } + if (userSegment == null) { + userSegment = user.getSegment(); + } + } TraceContext traceContext = new TraceContext( traceId, diff --git a/sentry/src/main/java/io/sentry/TracesSamplingDecision.java b/sentry/src/main/java/io/sentry/TracesSamplingDecision.java index 559a1333cc..3e8d895fb5 100644 --- a/sentry/src/main/java/io/sentry/TracesSamplingDecision.java +++ b/sentry/src/main/java/io/sentry/TracesSamplingDecision.java @@ -8,15 +8,11 @@ public final class TracesSamplingDecision { private final @NotNull Boolean sampled; - private final @NotNull Double sampleRate; + private final @Nullable Double sampleRate; public TracesSamplingDecision(@NotNull Boolean sampled, @Nullable Double sampleRate) { this.sampled = sampled; - if (sampleRate != null) { - this.sampleRate = sampleRate; - } else { - this.sampleRate = sampled ? 1.0 : 0.0; - } + this.sampleRate = sampleRate; } public TracesSamplingDecision(@NotNull Boolean sampled) { @@ -27,7 +23,7 @@ public TracesSamplingDecision(@NotNull Boolean sampled) { return sampled; } - public @NotNull Double getSampleRate() { + public @Nullable Double getSampleRate() { return sampleRate; } } diff --git a/sentry/src/test/java/io/sentry/SampleRateUtilTest.kt b/sentry/src/test/java/io/sentry/SampleRateUtilTest.kt new file mode 100644 index 0000000000..339543f3b5 --- /dev/null +++ b/sentry/src/test/java/io/sentry/SampleRateUtilTest.kt @@ -0,0 +1,103 @@ +package io.sentry + +import kotlin.test.Test +import kotlin.test.assertFalse +import kotlin.test.assertTrue + +class SampleRateUtilTest { + + @Test + fun `accepts 0 dot 01 for sample rate`() { + assertTrue(SampleRateUtil.isValidSampleRate(0.01)) + } + + @Test + fun `accepts 1 for sample rate`() { + assertTrue(SampleRateUtil.isValidSampleRate(1.0)) + } + + @Test + fun `rejects 0 for sample rate`() { + assertFalse(SampleRateUtil.isValidSampleRate(0.0)) + } + + @Test + fun `rejects 1 dot 01 for sample rate`() { + assertFalse(SampleRateUtil.isValidSampleRate(1.01)) + } + + @Test + fun `rejects negative sample rate`() { + assertFalse(SampleRateUtil.isValidSampleRate(-0.5)) + } + + @Test + fun `rejects NaN sample rate`() { + assertFalse(SampleRateUtil.isValidSampleRate(Double.NaN)) + } + + @Test + fun `rejects positive infinite sample rate`() { + assertFalse(SampleRateUtil.isValidSampleRate(Double.POSITIVE_INFINITY)) + } + + @Test + fun `rejects negative infinite sample rate`() { + assertFalse(SampleRateUtil.isValidSampleRate(Double.NEGATIVE_INFINITY)) + } + + @Test + fun `accepts null sample rate if told so`() { + assertTrue(SampleRateUtil.isValidSampleRate(null, true)) + } + + @Test + fun `rejects null sample rate if told so`() { + assertFalse(SampleRateUtil.isValidSampleRate(null, false)) + } + + @Test + fun `accepts 0 for traces sample rate`() { + assertTrue(SampleRateUtil.isValidTracesSampleRate(0.0)) + } + + @Test + fun `accepts 1 for traces sample rate`() { + assertTrue(SampleRateUtil.isValidTracesSampleRate(1.0)) + } + + @Test + fun `rejects negative traces sample rate`() { + assertFalse(SampleRateUtil.isValidTracesSampleRate(-0.5)) + } + + @Test + fun `rejects 1 dot 01 for traces sample rate`() { + assertFalse(SampleRateUtil.isValidTracesSampleRate(1.01)) + } + + @Test + fun `rejects NaN traces sample rate`() { + assertFalse(SampleRateUtil.isValidTracesSampleRate(Double.NaN)) + } + + @Test + fun `rejects positive infinite traces sample rate`() { + assertFalse(SampleRateUtil.isValidTracesSampleRate(Double.POSITIVE_INFINITY)) + } + + @Test + fun `rejects negative infinite traces sample rate`() { + assertFalse(SampleRateUtil.isValidTracesSampleRate(Double.NEGATIVE_INFINITY)) + } + + @Test + fun `accepts null traces sample rate if told so`() { + assertTrue(SampleRateUtil.isValidTracesSampleRate(null, true)) + } + + @Test + fun `rejects null traces sample rate if told so`() { + assertFalse(SampleRateUtil.isValidTracesSampleRate(null, false)) + } +} diff --git a/sentry/src/test/java/io/sentry/TraceContextSerializationTest.kt b/sentry/src/test/java/io/sentry/TraceContextSerializationTest.kt index 15aa989dfd..ffe43fa524 100644 --- a/sentry/src/test/java/io/sentry/TraceContextSerializationTest.kt +++ b/sentry/src/test/java/io/sentry/TraceContextSerializationTest.kt @@ -67,6 +67,15 @@ class TraceContextSerializationTest { ) } + @Test + fun `can still parse legacy JSON with non flat user`() { + val expectedJson = sanitizedFile("json/trace_state_no_sample_rate.json") + val legacyJson = sanitizedFile("json/trace_state_legacy.json") + val actual = deserialize(legacyJson) + val actualJson = serialize(actual) + assertEquals(expectedJson, actualJson) + } + // Helper private fun sanitizedFile(path: String): String { diff --git a/sentry/src/test/java/io/sentry/TracesSamplerTest.kt b/sentry/src/test/java/io/sentry/TracesSamplerTest.kt index 4d4b302d77..29d2f69eb7 100644 --- a/sentry/src/test/java/io/sentry/TracesSamplerTest.kt +++ b/sentry/src/test/java/io/sentry/TracesSamplerTest.kt @@ -6,6 +6,7 @@ import java.security.SecureRandom import kotlin.test.Test import kotlin.test.assertEquals import kotlin.test.assertFalse +import kotlin.test.assertNull import kotlin.test.assertTrue class TracesSamplerTest { @@ -82,7 +83,7 @@ class TracesSamplerTest { ) ) assertTrue(samplingDecision.sampled) - assertEquals(1.0, samplingDecision.sampleRate) + assertNull(samplingDecision.sampleRate) } @Test @@ -108,7 +109,7 @@ class TracesSamplerTest { ) ) assertFalse(samplingDecision.sampled) - assertEquals(0.0, samplingDecision.sampleRate) + assertNull(samplingDecision.sampleRate) } @Test @@ -123,7 +124,7 @@ class TracesSamplerTest { ) ) assertFalse(samplingDecision.sampled) - assertEquals(0.0, samplingDecision.sampleRate) + assertNull(samplingDecision.sampleRate) val transactionContextParentSampled = TransactionContext("name", "op") transactionContextParentSampled.parentSampled = true @@ -134,7 +135,7 @@ class TracesSamplerTest { ) ) assertTrue(samplingDecisionParentSampled.sampled) - assertEquals(1.0, samplingDecisionParentSampled.sampleRate) + assertNull(samplingDecisionParentSampled.sampleRate) } @Test @@ -145,13 +146,13 @@ class TracesSamplerTest { val samplingDecision = sampler.sample(SamplingContext(transactionContextNotSampled, CustomSamplingContext())) assertFalse(samplingDecision.sampled) - assertEquals(0.0, samplingDecision.sampleRate) + assertNull(samplingDecision.sampleRate) val transactionContextSampled = TransactionContext("name", "op") transactionContextSampled.sampled = true val samplingDecisionContextSampled = sampler.sample(SamplingContext(transactionContextSampled, CustomSamplingContext())) assertTrue(samplingDecisionContextSampled.sampled) - assertEquals(1.0, samplingDecisionContextSampled.sampleRate) + assertNull(samplingDecisionContextSampled.sampleRate) } } diff --git a/sentry/src/test/resources/json/trace_state_legacy.json b/sentry/src/test/resources/json/trace_state_legacy.json new file mode 100644 index 0000000000..14f4f904a6 --- /dev/null +++ b/sentry/src/test/resources/json/trace_state_legacy.json @@ -0,0 +1,12 @@ +{ + "trace_id": "65bcd18546c942069ed957b15b4ace7c", + "public_key": "5d593cac-f833-4845-bb23-4eabdf720da2", + "release": "9ee2c92c-401e-4296-b6f0-fb3b13edd9ee", + "environment": "0666ab02-6364-4135-aa59-02e8128ce052", + "user": + { + "id": "c052c566-6619-45f5-a61f-172802afa39a", + "segment": "f7d8662b-5551-4ef8-b6a8-090f0561a530" + }, + "transaction": "0252ec25-cd0a-4230-bd2f-936a4585637e" +} diff --git a/sentry/src/test/resources/json/trace_state_no_sample_rate.json b/sentry/src/test/resources/json/trace_state_no_sample_rate.json new file mode 100644 index 0000000000..538dc61671 --- /dev/null +++ b/sentry/src/test/resources/json/trace_state_no_sample_rate.json @@ -0,0 +1,9 @@ +{ + "trace_id": "65bcd18546c942069ed957b15b4ace7c", + "public_key": "5d593cac-f833-4845-bb23-4eabdf720da2", + "release": "9ee2c92c-401e-4296-b6f0-fb3b13edd9ee", + "environment": "0666ab02-6364-4135-aa59-02e8128ce052", + "user_id": "c052c566-6619-45f5-a61f-172802afa39a", + "user_segment": "f7d8662b-5551-4ef8-b6a8-090f0561a530", + "transaction": "0252ec25-cd0a-4230-bd2f-936a4585637e" +} From 755b63f655885caa88d2a435abb716998745d332 Mon Sep 17 00:00:00 2001 From: Alexander Dinauer Date: Wed, 29 Jun 2022 17:28:36 +0200 Subject: [PATCH 6/9] Test more envelope header trace fields for OutboxSender --- sentry/src/test/java/io/sentry/OutboxSenderTest.kt | 12 +++++++++++- .../envelope-transaction-with-sample-rate.txt | 2 +- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/sentry/src/test/java/io/sentry/OutboxSenderTest.kt b/sentry/src/test/java/io/sentry/OutboxSenderTest.kt index 3e731c76ff..4b23fdea90 100644 --- a/sentry/src/test/java/io/sentry/OutboxSenderTest.kt +++ b/sentry/src/test/java/io/sentry/OutboxSenderTest.kt @@ -170,7 +170,17 @@ class OutboxSenderTest { assertEquals(0.00000021, it.samplingDecision?.sampleRate) assertTrue(it.samplingDecision!!.sampled) }, - any(), any() + check { + assertEquals("b156a475de54423d9c1571df97ec7eb6", it.traceId.toString()) + assertEquals("key", it.publicKey) + assertEquals("0.00000021", it.sampleRate) + assertEquals("1.0-beta.1", it.release) + assertEquals("prod", it.environment) + assertEquals("usr1", it.userId) + assertEquals("pro", it.userSegment) + assertEquals("tx1", it.transaction) + }, + any() ) assertFalse(File(path).exists()) diff --git a/sentry/src/test/resources/envelope-transaction-with-sample-rate.txt b/sentry/src/test/resources/envelope-transaction-with-sample-rate.txt index ae4746ff8f..b790648e06 100644 --- a/sentry/src/test/resources/envelope-transaction-with-sample-rate.txt +++ b/sentry/src/test/resources/envelope-transaction-with-sample-rate.txt @@ -1,3 +1,3 @@ -{"event_id":"3367f5196c494acaae85bbbd535379ac","trace":{"trace_id":"b156a475de54423d9c1571df97ec7eb6","public_key":"key","sample_rate":"0.00000021"}} +{"event_id":"3367f5196c494acaae85bbbd535379ac","trace":{"trace_id":"b156a475de54423d9c1571df97ec7eb6","public_key":"key","release":"1.0-beta.1","environment":"prod","user_id":"usr1","user_segment":"pro","transaction":"tx1","sample_rate":"0.00000021"}} {"type":"transaction","length":640,"content_type":"application/json"} {"transaction":"a-transaction","type":"transaction","start_timestamp":"2020-10-23T10:24:01.791Z","timestamp":"2020-10-23T10:24:02.791Z","event_id":"3367f5196c494acaae85bbbd535379ac","contexts":{"trace":{"trace_id":"b156a475de54423d9c1571df97ec7eb6","span_id":"0a53026963414893","op":"http","status":"ok"},"custom":{"some-key":"some-value"}},"spans":[{"start_timestamp":"2021-03-05T08:51:12.838Z","timestamp":"2021-03-05T08:51:12.949Z","trace_id":"2b099185293344a5bfdd7ad89ebf9416","span_id":"5b95c29a5ded4281","parent_span_id":"a3b2d1d58b344b07","op":"PersonService.create","description":"desc","status":"aborted","tags":{"name":"value"}}]} From a8a84790156e0f1f3618789e61f65032c9dc3a94 Mon Sep 17 00:00:00 2001 From: Alexander Dinauer Date: Wed, 29 Jun 2022 18:05:03 +0200 Subject: [PATCH 7/9] Skip sending userId in baggage if send default pii is off --- sentry/api/sentry.api | 10 +-- sentry/src/main/java/io/sentry/Baggage.java | 88 ++++++++++++------- .../src/main/java/io/sentry/SentryTracer.java | 2 +- .../src/main/java/io/sentry/TraceContext.java | 4 +- sentry/src/test/java/io/sentry/BaggageTest.kt | 76 ++++++++++------ .../test/java/io/sentry/SentryTracerTest.kt | 1 + 6 files changed, 111 insertions(+), 70 deletions(-) diff --git a/sentry/api/sentry.api b/sentry/api/sentry.api index 430fc8dcad..163d54288d 100644 --- a/sentry/api/sentry.api +++ b/sentry/api/sentry.api @@ -21,10 +21,10 @@ public final class io/sentry/Attachment { } public final class io/sentry/Baggage { - public fun (Lio/sentry/ILogger;)V - public fun (Ljava/util/Map;Lio/sentry/ILogger;)V - public static fun fromHeader (Ljava/lang/String;Lio/sentry/ILogger;)Lio/sentry/Baggage; - public static fun fromHeader (Ljava/util/List;Lio/sentry/ILogger;)Lio/sentry/Baggage; + public fun (Lio/sentry/SentryOptions;)V + public fun (Ljava/util/Map;Lio/sentry/SentryOptions;)V + public static fun fromHeader (Ljava/lang/String;Lio/sentry/SentryOptions;)Lio/sentry/Baggage; + public static fun fromHeader (Ljava/util/List;Lio/sentry/SentryOptions;)Lio/sentry/Baggage; public fun get (Ljava/lang/String;)Ljava/lang/String; public fun set (Ljava/lang/String;Ljava/lang/String;)V public fun setEnvironment (Ljava/lang/String;)V @@ -1642,7 +1642,7 @@ public final class io/sentry/TraceContext : io/sentry/JsonSerializable, io/sentr public fun getUserSegment ()Ljava/lang/String; public fun serialize (Lio/sentry/JsonObjectWriter;Lio/sentry/ILogger;)V public fun setUnknown (Ljava/util/Map;)V - public fun toBaggage (Lio/sentry/ILogger;)Lio/sentry/Baggage; + public fun toBaggage (Lio/sentry/SentryOptions;)Lio/sentry/Baggage; } public final class io/sentry/TraceContext$Deserializer : io/sentry/JsonDeserializer { diff --git a/sentry/src/main/java/io/sentry/Baggage.java b/sentry/src/main/java/io/sentry/Baggage.java index c93eef61e6..638172174b 100644 --- a/sentry/src/main/java/io/sentry/Baggage.java +++ b/sentry/src/main/java/io/sentry/Baggage.java @@ -20,31 +20,31 @@ public final class Baggage { static final @NotNull Integer MAX_BAGGAGE_LIST_MEMBER_COUNT = 64; final @NotNull Map keyValues; - final @NotNull ILogger logger; + final @NotNull SentryOptions options; public static Baggage fromHeader( - final @Nullable List headerValues, final @NotNull ILogger logger) { + final @Nullable List headerValues, final @NotNull SentryOptions options) { final Map keyValues = new HashMap<>(); if (headerValues != null) { for (final @Nullable String headerValue : headerValues) { final Map keyValuesToAdd = - extractKeyValuesFromBaggageString(headerValue, logger); + extractKeyValuesFromBaggageString(headerValue, options); keyValues.putAll(keyValuesToAdd); } } - return new Baggage(keyValues, logger); + return new Baggage(keyValues, options); } public static Baggage fromHeader( - final @Nullable String headerValue, final @NotNull ILogger logger) { - final Map keyValues = extractKeyValuesFromBaggageString(headerValue, logger); - return new Baggage(keyValues, logger); + final @Nullable String headerValue, final @NotNull SentryOptions options) { + final Map keyValues = extractKeyValuesFromBaggageString(headerValue, options); + return new Baggage(keyValues, options); } private static Map extractKeyValuesFromBaggageString( - final @Nullable String headerValue, final @NotNull ILogger logger) { + final @Nullable String headerValue, final @NotNull SentryOptions options) { final @NotNull Map keyValues = new HashMap<>(); if (headerValue != null) { @@ -62,28 +62,40 @@ private static Map extractKeyValuesFromBaggageString( keyValues.put(keyDecoded, valueDecoded); } else { - logger.log( - SentryLevel.ERROR, "Unable to decode baggage key value pair %s", keyValueString); + options + .getLogger() + .log( + SentryLevel.ERROR, + "Unable to decode baggage key value pair %s", + keyValueString); } } catch (Throwable e) { - logger.log( - SentryLevel.ERROR, e, "Unable to decode baggage key value pair %s", keyValueString); + options + .getLogger() + .log( + SentryLevel.ERROR, + e, + "Unable to decode baggage key value pair %s", + keyValueString); } } } catch (Throwable e) { - logger.log(SentryLevel.ERROR, e, "Unable to decode baggage header %s", headerValue); + options + .getLogger() + .log(SentryLevel.ERROR, e, "Unable to decode baggage header %s", headerValue); } } return keyValues; } - public Baggage(final @NotNull ILogger logger) { - this(new HashMap<>(), logger); + public Baggage(final @NotNull SentryOptions options) { + this(new HashMap<>(), options); } - public Baggage(final @NotNull Map keyValues, final @NotNull ILogger logger) { + public Baggage( + final @NotNull Map keyValues, final @NotNull SentryOptions options) { this.keyValues = keyValues; - this.logger = logger; + this.options = options; } public @NotNull String toHeaderString() { @@ -97,11 +109,13 @@ public Baggage(final @NotNull Map keyValues, final @NotNull ILog if (value != null) { if (listMemberCount >= MAX_BAGGAGE_LIST_MEMBER_COUNT) { - logger.log( - SentryLevel.ERROR, - "Not adding baggage value %s as the total number of list members would exceed the maximum of %s.", - key, - MAX_BAGGAGE_LIST_MEMBER_COUNT); + options + .getLogger() + .log( + SentryLevel.ERROR, + "Not adding baggage value %s as the total number of list members would exceed the maximum of %s.", + key, + MAX_BAGGAGE_LIST_MEMBER_COUNT); } else { try { final String encodedKey = encode(key); @@ -111,23 +125,27 @@ public Baggage(final @NotNull Map keyValues, final @NotNull ILog final int valueLength = encodedKeyValue.length(); final int totalLengthIfValueAdded = sb.length() + valueLength; if (totalLengthIfValueAdded > MAX_BAGGAGE_STRING_LENGTH) { - logger.log( - SentryLevel.ERROR, - "Not adding baggage value %s as the total header value length would exceed the maximum of %s.", - key, - MAX_BAGGAGE_STRING_LENGTH); + options + .getLogger() + .log( + SentryLevel.ERROR, + "Not adding baggage value %s as the total header value length would exceed the maximum of %s.", + key, + MAX_BAGGAGE_STRING_LENGTH); } else { listMemberCount++; sb.append(encodedKeyValue); separator = ","; } } catch (Throwable e) { - logger.log( - SentryLevel.ERROR, - e, - "Unable to encode baggage key value pair (key=%s,value=%s).", - key, - value); + options + .getLogger() + .log( + SentryLevel.ERROR, + e, + "Unable to encode baggage key value pair (key=%s,value=%s).", + key, + value); } } } @@ -169,7 +187,9 @@ public void setRelease(final @Nullable String release) { } public void setUserId(final @Nullable String userId) { - set("sentry-user_id", userId); + if (options.isSendDefaultPii()) { + set("sentry-user_id", userId); + } } public void setUserSegment(final @Nullable String userSegment) { diff --git a/sentry/src/main/java/io/sentry/SentryTracer.java b/sentry/src/main/java/io/sentry/SentryTracer.java index a05d37c579..003bf91c35 100644 --- a/sentry/src/main/java/io/sentry/SentryTracer.java +++ b/sentry/src/main/java/io/sentry/SentryTracer.java @@ -382,7 +382,7 @@ public void finish(@Nullable SpanStatus status) { public @Nullable BaggageHeader toBaggageHeader() { final TraceContext traceContext = traceContext(); if (hub.getOptions().isTraceSampling() && traceContext != null) { - final Baggage baggage = traceContext.toBaggage(hub.getOptions().getLogger()); + final Baggage baggage = traceContext.toBaggage(hub.getOptions()); return new BaggageHeader(baggage); } else { return null; diff --git a/sentry/src/main/java/io/sentry/TraceContext.java b/sentry/src/main/java/io/sentry/TraceContext.java index 3486502ff7..062b92d486 100644 --- a/sentry/src/main/java/io/sentry/TraceContext.java +++ b/sentry/src/main/java/io/sentry/TraceContext.java @@ -125,8 +125,8 @@ public final class TraceContext implements JsonUnknown, JsonSerializable { return sampleRate; } - public @NotNull Baggage toBaggage(@NotNull ILogger logger) { - Baggage baggage = new Baggage(logger); + public @NotNull Baggage toBaggage(@NotNull SentryOptions options) { + Baggage baggage = new Baggage(options); baggage.setTraceId(traceId.toString()); baggage.setPublicKey(publicKey); diff --git a/sentry/src/test/java/io/sentry/BaggageTest.kt b/sentry/src/test/java/io/sentry/BaggageTest.kt index 1ced5f9fd3..72e977e577 100644 --- a/sentry/src/test/java/io/sentry/BaggageTest.kt +++ b/sentry/src/test/java/io/sentry/BaggageTest.kt @@ -12,16 +12,17 @@ import kotlin.test.assertNotNull import kotlin.test.assertTrue class BaggageTest { - lateinit var logger: ILogger + + lateinit var optionsWithPiiOn: SentryOptions @BeforeTest fun setup() { - logger = NoOpLogger.getInstance() + optionsWithPiiOn = SentryOptions().also { it.isSendDefaultPii = true } } @Test fun `can parse single baggage string with white spaces in it`() { - val baggage = Baggage.fromHeader("userId = alice , serverNode = DF%2028,isProduction=false", logger) + val baggage = Baggage.fromHeader("userId = alice , serverNode = DF%2028,isProduction=false", optionsWithPiiOn) assertEquals("alice", baggage.get("userId")) assertEquals("DF 28", baggage.get("serverNode")) @@ -32,7 +33,7 @@ class BaggageTest { @Test fun `can parse single baggage string`() { - val baggage = Baggage.fromHeader("userId=alice,serverNode=DF%2028,isProduction=false", logger) + val baggage = Baggage.fromHeader("userId=alice,serverNode=DF%2028,isProduction=false", optionsWithPiiOn) assertEquals("alice", baggage.get("userId")) assertEquals("DF 28", baggage.get("serverNode")) @@ -43,7 +44,7 @@ class BaggageTest { @Test fun `keys are encoded and decoded as well`() { - val baggage = Baggage.fromHeader("user%2Bid=alice,server%2Bnode=DF%2028,is%2Bproduction=false", logger) + val baggage = Baggage.fromHeader("user%2Bid=alice,server%2Bnode=DF%2028,is%2Bproduction=false", optionsWithPiiOn) assertEquals("alice", baggage.get("user+id")) assertEquals("DF 28", baggage.get("server+node")) @@ -59,7 +60,7 @@ class BaggageTest { "userId=alice", "serverNode=DF%2028,isProduction=false" ), - logger + optionsWithPiiOn ) assertEquals("alice", baggage.get("userId")) @@ -76,7 +77,7 @@ class BaggageTest { "userId = alice", "serverNode = DF%2028, isProduction = false" ), - logger + optionsWithPiiOn ) assertEquals("alice", baggage.get("userId")) @@ -89,32 +90,32 @@ class BaggageTest { @Test fun `can parse null baggage string`() { val nothing: String? = null - val baggage = Baggage.fromHeader(nothing, logger) + val baggage = Baggage.fromHeader(nothing, optionsWithPiiOn) assertEquals("", baggage.toHeaderString()) } @Test fun `can parse blank baggage string`() { - val baggage = Baggage.fromHeader("", logger) + val baggage = Baggage.fromHeader("", optionsWithPiiOn) assertEquals("", baggage.toHeaderString()) } @Test fun `can parse whitespace only baggage string`() { - val baggage = Baggage.fromHeader(" ", logger) + val baggage = Baggage.fromHeader(" ", optionsWithPiiOn) assertEquals("", baggage.toHeaderString()) } @Test fun `can parse whitespace only baggage strings`() { - val baggage = Baggage.fromHeader(listOf(" ", " "), logger) + val baggage = Baggage.fromHeader(listOf(" ", " "), optionsWithPiiOn) assertEquals("", baggage.toHeaderString()) } @Test fun `single large value is dropped and small values are kept`() { val largeValue = Faker.instance().random().hex(8193) - val baggage = Baggage.fromHeader("smallValue=remains,largeValue=$largeValue,otherValue=kept", logger) + val baggage = Baggage.fromHeader("smallValue=remains,largeValue=$largeValue,otherValue=kept", optionsWithPiiOn) assertEquals("remains", baggage.get("smallValue")) assertNotNull(baggage.get("largeValue")) @@ -126,7 +127,7 @@ class BaggageTest { @Test fun `medium size value can cause small values to be dropped`() { val mediumValue = Faker.instance().random().hex(MAX_BAGGAGE_STRING_LENGTH - 12 - 15 - 1) // 8192 - "mediumValue=" - "otherValue=kept" - "," - val baggage = Baggage.fromHeader("mediumValue=$mediumValue,smallValue=removed,otherValue=kept", logger) + val baggage = Baggage.fromHeader("mediumValue=$mediumValue,smallValue=removed,otherValue=kept", optionsWithPiiOn) assertEquals("removed", baggage.get("smallValue")) assertEquals(mediumValue, baggage.get("mediumValue")) @@ -141,7 +142,7 @@ class BaggageTest { fun `medium size value can cause all values to be dropped`() { // nothing else will fit after mediumValue as the separator + any key/value would exceed the limit val mediumValue = Faker.instance().random().hex(MAX_BAGGAGE_STRING_LENGTH - 12 - 15) // 8192 - "mediumValue=" - "otherValue=lost" - val baggage = Baggage.fromHeader("mediumValue=$mediumValue,smallValue=stripped,otherValue=lost", logger) + val baggage = Baggage.fromHeader("mediumValue=$mediumValue,smallValue=stripped,otherValue=lost", optionsWithPiiOn) assertEquals("stripped", baggage.get("smallValue")) assertEquals(mediumValue, baggage.get("mediumValue")) @@ -154,7 +155,7 @@ class BaggageTest { @Test fun `exceeding entry limit causes values to be dropped`() { - val baggage = Baggage(logger) + val baggage = Baggage(optionsWithPiiOn) val expectedItems = mutableListOf() for (i in 1..100) { @@ -171,7 +172,7 @@ class BaggageTest { @Test fun `null value is omitted from header string`() { - val baggage = Baggage(logger) + val baggage = Baggage(optionsWithPiiOn) baggage.setTraceId(null) baggage.setPublicKey(null) @@ -186,7 +187,7 @@ class BaggageTest { @Test fun `can set values from trace context`() { - val baggage = Baggage(logger) + val baggage = Baggage(optionsWithPiiOn) val publicKey = Dsn(dsnString).publicKey val traceId = SentryId().toString() val userId = UUID.randomUUID().toString() @@ -203,15 +204,34 @@ class BaggageTest { assertEquals("sentry-environment=production,sentry-public_key=$publicKey,sentry-release=1.0-rc.1,sentry-sample_rate=0.3333333333333333,sentry-trace_id=$traceId,sentry-transaction=TX,sentry-user_id=$userId,sentry-user_segment=segmentA", baggage.toHeaderString()) } + @Test + fun `userId is skipped if sendDefaultPii is off`() { + val baggage = Baggage(SentryOptions()) + val publicKey = Dsn(dsnString).publicKey + val traceId = SentryId().toString() + val userId = UUID.randomUUID().toString() + + baggage.setTraceId(traceId) + baggage.setPublicKey(publicKey) + baggage.setRelease("1.0-rc.1") + baggage.setEnvironment("production") + baggage.setTransaction("TX") + baggage.setUserId(userId) + baggage.setUserSegment("segmentA") + baggage.setSampleRate((1.0 / 3.0).toString()) + + assertEquals("sentry-environment=production,sentry-public_key=$publicKey,sentry-release=1.0-rc.1,sentry-sample_rate=0.3333333333333333,sentry-trace_id=$traceId,sentry-transaction=TX,sentry-user_segment=segmentA", baggage.toHeaderString()) + } + @Test fun `duplicate entries are lost`() { - val baggage = Baggage.fromHeader("duplicate=a,duplicate=b", logger) + val baggage = Baggage.fromHeader("duplicate=a,duplicate=b", optionsWithPiiOn) assertEquals("duplicate=b", baggage.toHeaderString()) } @Test fun `setting a value multiple times only keeps the last`() { - val baggage = Baggage.fromHeader("sentry-trace_id=a", logger) + val baggage = Baggage.fromHeader("sentry-trace_id=a", optionsWithPiiOn) baggage.setTraceId("b") baggage.setTraceId("c") @@ -221,7 +241,7 @@ class BaggageTest { @Test fun `value may contain = sign`() { - val baggage = Baggage(logger) + val baggage = Baggage(optionsWithPiiOn) baggage.setTransaction("a=b") @@ -230,19 +250,19 @@ class BaggageTest { @Test fun `corrupted string does not throw out`() { - val baggage = Baggage.fromHeader("a", logger) + val baggage = Baggage.fromHeader("a", optionsWithPiiOn) assertEquals("", baggage.toHeaderString()) } @Test fun `corrupted string does not throw out 2`() { - val baggage = Baggage.fromHeader("a=b=", logger) + val baggage = Baggage.fromHeader("a=b=", optionsWithPiiOn) assertEquals("", baggage.toHeaderString()) } @Test fun `corrupted string can be parsed partially`() { - val baggage = Baggage.fromHeader("a=value,b", logger) + val baggage = Baggage.fromHeader("a=value,b", optionsWithPiiOn) assertEquals("a=value", baggage.toHeaderString()) } @@ -345,7 +365,7 @@ class BaggageTest { val failures = mutableListOf() values.forEach { key, value -> - val baggage = Baggage(logger) + val baggage = Baggage(optionsWithPiiOn) baggage.setTransaction(key) val headerString = baggage.toHeaderString() @@ -353,7 +373,7 @@ class BaggageTest { failures.add("$key should be $value but was >$headerString<") } - val decodedBaggage = Baggage.fromHeader(headerString, logger) + val decodedBaggage = Baggage.fromHeader(headerString, optionsWithPiiOn) decodedBaggage.get("sentry-transaction") } @@ -362,17 +382,17 @@ class BaggageTest { @Test fun `all characters defined as valid for keys can be used`() { - val baggage = Baggage(logger) + val baggage = Baggage(optionsWithPiiOn) val key = validTokenCharacters().joinToString("") baggage.set(key, "value") - val reparsedBaggage = Baggage.fromHeader(baggage.toHeaderString(), logger) + val reparsedBaggage = Baggage.fromHeader(baggage.toHeaderString(), optionsWithPiiOn) assertEquals("value", reparsedBaggage.get(key)) } @Test fun `baggage key replaces invalid characters`() { - val baggage = Baggage(logger) + val baggage = Baggage(optionsWithPiiOn) baggage.set(invalidTokenCharacters().joinToString(""), "value") assertEquals("%22%28%29%2C%2F%3A%3B%3C%3D%3E%3F%40%5B%5C%5D%7B%7D=value", baggage.toHeaderString()) diff --git a/sentry/src/test/java/io/sentry/SentryTracerTest.kt b/sentry/src/test/java/io/sentry/SentryTracerTest.kt index 527a2c9af0..1e76cb9d1d 100644 --- a/sentry/src/test/java/io/sentry/SentryTracerTest.kt +++ b/sentry/src/test/java/io/sentry/SentryTracerTest.kt @@ -525,6 +525,7 @@ class SentryTracerTest { it.isTraceSampling = true it.environment = "production" it.release = "1.0.99-rc.7" + it.isSendDefaultPii = true }) fixture.hub.setUser( From 63d71ad892165c3655ed51cba96c1b44c71a8f15 Mon Sep 17 00:00:00 2001 From: Alexander Dinauer Date: Wed, 29 Jun 2022 18:08:59 +0200 Subject: [PATCH 8/9] Add changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index bd51355d16..4cd02b8103 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ ### Fixes - Filter out app starts with more than 60s ([#2127](https://github.com/getsentry/sentry-java/pull/2127)) +- Only send userid in `baggage` if `sendDefaultPii` is `true` ([#2145](https://github.com/getsentry/sentry-java/pull/2145)) ## Unreleased From 87f91f8bd2cffee2e87acadca7dcbff585c9cb93 Mon Sep 17 00:00:00 2001 From: Alexander Dinauer Date: Thu, 30 Jun 2022 11:19:43 +0200 Subject: [PATCH 9/9] CR changes --- sentry/api/sentry.api | 16 ++++++++-------- sentry/src/main/java/io/sentry/OutboxSender.java | 1 + .../src/main/java/io/sentry/SentryOptions.java | 1 + sentry/src/main/java/io/sentry/TraceContext.java | 1 + .../src/main/java/io/sentry/TracesSampler.java | 6 +++--- .../io/sentry/{ => util}/SampleRateUtil.java | 2 +- .../io/sentry/{ => util}/SampleRateUtilTest.kt | 2 +- 7 files changed, 16 insertions(+), 13 deletions(-) rename sentry/src/main/java/io/sentry/{ => util}/SampleRateUtil.java (97%) rename sentry/src/test/java/io/sentry/{ => util}/SampleRateUtilTest.kt (99%) diff --git a/sentry/api/sentry.api b/sentry/api/sentry.api index 430fc8dcad..7341034af6 100644 --- a/sentry/api/sentry.api +++ b/sentry/api/sentry.api @@ -795,14 +795,6 @@ public final class io/sentry/RequestDetails { public fun getUrl ()Ljava/net/URL; } -public final class io/sentry/SampleRateUtil { - public fun ()V - public static fun isValidSampleRate (Ljava/lang/Double;)Z - public static fun isValidSampleRate (Ljava/lang/Double;Z)Z - public static fun isValidTracesSampleRate (Ljava/lang/Double;)Z - public static fun isValidTracesSampleRate (Ljava/lang/Double;Z)Z -} - public final class io/sentry/SamplingContext { public fun (Lio/sentry/TransactionContext;Lio/sentry/CustomSamplingContext;)V public fun getCustomSamplingContext ()Lio/sentry/CustomSamplingContext; @@ -3042,6 +3034,14 @@ public final class io/sentry/util/Platform { public static fun isJvm ()Z } +public final class io/sentry/util/SampleRateUtil { + public fun ()V + public static fun isValidSampleRate (Ljava/lang/Double;)Z + public static fun isValidSampleRate (Ljava/lang/Double;Z)Z + public static fun isValidTracesSampleRate (Ljava/lang/Double;)Z + public static fun isValidTracesSampleRate (Ljava/lang/Double;Z)Z +} + public final class io/sentry/util/StringUtils { public static fun byteCountToString (J)Ljava/lang/String; public static fun calculateStringHash (Ljava/lang/String;Lio/sentry/ILogger;)Ljava/lang/String; diff --git a/sentry/src/main/java/io/sentry/OutboxSender.java b/sentry/src/main/java/io/sentry/OutboxSender.java index 7017459b58..902e76bfb0 100644 --- a/sentry/src/main/java/io/sentry/OutboxSender.java +++ b/sentry/src/main/java/io/sentry/OutboxSender.java @@ -13,6 +13,7 @@ import io.sentry.util.HintUtils; import io.sentry.util.LogUtils; import io.sentry.util.Objects; +import io.sentry.util.SampleRateUtil; import java.io.BufferedInputStream; import java.io.BufferedReader; import java.io.ByteArrayInputStream; diff --git a/sentry/src/main/java/io/sentry/SentryOptions.java b/sentry/src/main/java/io/sentry/SentryOptions.java index b1eb901113..7892d927ae 100644 --- a/sentry/src/main/java/io/sentry/SentryOptions.java +++ b/sentry/src/main/java/io/sentry/SentryOptions.java @@ -10,6 +10,7 @@ import io.sentry.transport.NoOpEnvelopeCache; import io.sentry.transport.NoOpTransportGate; import io.sentry.util.Platform; +import io.sentry.util.SampleRateUtil; import io.sentry.util.StringUtils; import java.io.File; import java.util.ArrayList; diff --git a/sentry/src/main/java/io/sentry/TraceContext.java b/sentry/src/main/java/io/sentry/TraceContext.java index 3486502ff7..83c2fda212 100644 --- a/sentry/src/main/java/io/sentry/TraceContext.java +++ b/sentry/src/main/java/io/sentry/TraceContext.java @@ -2,6 +2,7 @@ import io.sentry.protocol.SentryId; import io.sentry.protocol.User; +import io.sentry.util.SampleRateUtil; import io.sentry.vendor.gson.stream.JsonToken; import java.io.IOException; import java.text.DecimalFormat; diff --git a/sentry/src/main/java/io/sentry/TracesSampler.java b/sentry/src/main/java/io/sentry/TracesSampler.java index 0658b820f0..c169c18faf 100644 --- a/sentry/src/main/java/io/sentry/TracesSampler.java +++ b/sentry/src/main/java/io/sentry/TracesSampler.java @@ -21,7 +21,7 @@ public TracesSampler(final @NotNull SentryOptions options) { @NotNull TracesSamplingDecision sample(final @NotNull SamplingContext samplingContext) { - TracesSamplingDecision samplingContextSamplingDecision = + final TracesSamplingDecision samplingContextSamplingDecision = samplingContext.getTransactionContext().getSamplingDecision(); if (samplingContextSamplingDecision != null) { return samplingContextSamplingDecision; @@ -34,13 +34,13 @@ TracesSamplingDecision sample(final @NotNull SamplingContext samplingContext) { } } - TracesSamplingDecision parentSamplingDecision = + final TracesSamplingDecision parentSamplingDecision = samplingContext.getTransactionContext().getParentSamplingDecision(); if (parentSamplingDecision != null) { return parentSamplingDecision; } - Double tracesSampleRateFromOptions = options.getTracesSampleRate(); + final Double tracesSampleRateFromOptions = options.getTracesSampleRate(); if (tracesSampleRateFromOptions != null) { return new TracesSamplingDecision( sample(tracesSampleRateFromOptions), tracesSampleRateFromOptions); diff --git a/sentry/src/main/java/io/sentry/SampleRateUtil.java b/sentry/src/main/java/io/sentry/util/SampleRateUtil.java similarity index 97% rename from sentry/src/main/java/io/sentry/SampleRateUtil.java rename to sentry/src/main/java/io/sentry/util/SampleRateUtil.java index ec521f52fd..0035a240be 100644 --- a/sentry/src/main/java/io/sentry/SampleRateUtil.java +++ b/sentry/src/main/java/io/sentry/util/SampleRateUtil.java @@ -1,4 +1,4 @@ -package io.sentry; +package io.sentry.util; import org.jetbrains.annotations.ApiStatus; import org.jetbrains.annotations.Nullable; diff --git a/sentry/src/test/java/io/sentry/SampleRateUtilTest.kt b/sentry/src/test/java/io/sentry/util/SampleRateUtilTest.kt similarity index 99% rename from sentry/src/test/java/io/sentry/SampleRateUtilTest.kt rename to sentry/src/test/java/io/sentry/util/SampleRateUtilTest.kt index 339543f3b5..5cb4268152 100644 --- a/sentry/src/test/java/io/sentry/SampleRateUtilTest.kt +++ b/sentry/src/test/java/io/sentry/util/SampleRateUtilTest.kt @@ -1,4 +1,4 @@ -package io.sentry +package io.sentry.util import kotlin.test.Test import kotlin.test.assertFalse