Fix EnableTracing option conflict with TracesSampleRate (#2368)

mattjohnsonpint · web-flow · commit 4e947d8aa2a0 · 2023-05-13T22:15:12.000-06:00
* Fix tracing options

* Update tests

* Update API snapshots

* Fix unrelated solution filter

* Update CHANGELOG.md
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -9,6 +9,13 @@
 
 - Add tag filters to `SentryOptions` ([#2367](https://github.com/getsentry/sentry-dotnet/pull/2367))
 
+### Fixes
+
+- Fix `EnableTracing` option conflict with `TracesSampleRate` ([#2368](https://github.com/getsentry/sentry-dotnet/pull/2368))
+ - NOTE: This is a potentially breaking change, as the `TracesSampleRate` property has been made nullable.
+   Though extremely uncommon, if you are _retrieving_ the `TracesSampleRate` property for some reason, you will need to account for nulls.
+   However, there is no change to the behavior or _typical_ usage of either of these properties.
+
 ### Dependencies
 
 - Bump Cocoa SDK from v8.6.0 to v8.7.0 ([#2359](https://github.com/getsentry/sentry-dotnet/pull/2359))
diff --git a/SentryMobile.slnf b/SentryMobile.slnf
@@ -17,7 +17,6 @@
       "test\\Sentry.Extensions.Logging.Tests\\Sentry.Extensions.Logging.Tests.csproj",
       "test\\Sentry.Maui.Device.TestApp\\Sentry.Maui.Device.TestApp.csproj",
       "test\\Sentry.Maui.Tests\\Sentry.Maui.Tests.csproj",
-      "test\\Sentry.Testing.CrashableApp\\Sentry.Testing.CrashableApp.csproj",
       "test\\Sentry.Testing\\Sentry.Testing.csproj",
       "test\\Sentry.Tests\\Sentry.Tests.csproj"
     ]
diff --git a/src/Sentry/Internal/Hub.cs b/src/Sentry/Internal/Hub.cs
@@ -148,7 +148,7 @@ internal ITransaction StartTransaction(
             // Random sampling runs only if the sampling decision hasn't been made already.
             if (transaction.IsSampled == null)
             {
-                var sampleRate = _options.TracesSampleRate;
+                var sampleRate = _options.TracesSampleRate ?? (_options.EnableTracing is true ? 1.0 : 0.0);
                 transaction.IsSampled = _randomValuesFactory.NextBool(sampleRate);
                 transaction.SampleRate = sampleRate;
             }
diff --git a/src/Sentry/Platforms/Android/SentrySdk.cs b/src/Sentry/Platforms/Android/SentrySdk.cs
@@ -4,7 +4,6 @@
 using Sentry.Android.Callbacks;
 using Sentry.Android.Extensions;
 using Sentry.JavaSdk.Android.Core;
-using Sentry.Protocol;
 
 // ReSharper disable once CheckNamespace
 namespace Sentry;
@@ -108,8 +107,9 @@ private static void InitSentryAndroidSdk(SentryOptions options)
             }
 
             // These options we have behind feature flags
-            if (options.IsTracingEnabled && options.Android.EnableAndroidSdkTracing)
+            if (options is {IsTracingEnabled: true, Android.EnableAndroidSdkTracing: true})
             {
+                o.EnableTracing = (JavaBoolean?)options.EnableTracing;
                 o.TracesSampleRate = (JavaDouble?)options.TracesSampleRate;
 
                 if (options.TracesSampler is { } tracesSampler)
diff --git a/src/Sentry/Platforms/iOS/SentrySdk.cs b/src/Sentry/Platforms/iOS/SentrySdk.cs
@@ -66,8 +66,13 @@ private static void InitSentryCocoaSdk(SentryOptions options)
         }
 
         // These options we have behind feature flags
-        if (options.IsTracingEnabled && options.iOS.EnableCocoaSdkTracing)
+        if (options is {IsTracingEnabled: true, iOS.EnableCocoaSdkTracing: true})
         {
+            if (options.EnableTracing != null)
+            {
+                cocoaOptions.EnableTracing = options.EnableTracing.Value;
+            }
+
             cocoaOptions.TracesSampleRate = options.TracesSampleRate;
 
             if (options.TracesSampler is { } tracesSampler)
diff --git a/src/Sentry/SentryOptions.cs b/src/Sentry/SentryOptions.cs
@@ -585,7 +585,12 @@ public IList<SubstringOrRegexPattern> FailedRequestTargets {
     /// Indicates whether tracing is enabled, via any combination of
     /// <see cref="EnableTracing"/>, <see cref="TracesSampleRate"/>, or <see cref="TracesSampler"/>.
     /// </summary>
-    internal bool IsTracingEnabled => EnableTracing ?? (_tracesSampleRate > 0.0 || TracesSampler is not null);
+    internal bool IsTracingEnabled => EnableTracing switch
+    {
+        false => false,
+        null => TracesSampler is not null || TracesSampleRate is > 0.0,
+        true => TracesSampler is not null || TracesSampleRate is > 0.0 or null
+    };
 
     /// <summary>
     /// Simplified option for enabling or disabling tracing.
@@ -622,25 +627,43 @@ public IList<SubstringOrRegexPattern> FailedRequestTargets {
 
     /// <summary>
     /// Indicates the percentage of the tracing data that is collected.
-    /// Setting this to <c>0.0</c> discards all trace data.
-    /// Setting this to <c>1.0</c> collects all trace data.
-    /// Values outside of this range are invalid.
-    /// The default value is either <c>0.0</c> or <c>1.0</c>, depending on the <see cref="EnableTracing"/> property.
+    /// <list type="table">
+    ///   <listheader>
+    ///     <term>Value</term>
+    ///     <description>Effect</description>
+    ///   </listheader>
+    ///   <item>
+    ///     <term><c>&gt;= 0.0 and &lt;=1.0</c></term>
+    ///     <description>
+    ///       A custom sample rate is used unless <see cref="EnableTracing"/> is <c>false</c>,
+    ///       or unless overriden by a <see cref="TracesSampler"/> function.
+    ///       Values outside of this range are invalid.
+    ///     </description>
+    ///   </item>
+    ///   <item>
+    ///     <term><c>null</c></term>
+    ///     <description>
+    ///       <b>The default setting.</b>
+    ///       The tracing sample rate is determined by the <see cref="EnableTracing"/> property,
+    ///       unless overriden by a <see cref="TracesSampler"/> function.
+    ///     </description>
+    ///   </item>
+    /// </list>
     /// </summary>
     /// <remarks>
     /// Random sampling rate is only applied to transactions that don't already
     /// have a sampling decision set by other means, such as through <see cref="TracesSampler"/>,
     /// by inheriting it from an incoming trace header, or by copying it from <see cref="TransactionContext"/>.
     /// </remarks>
-    public double TracesSampleRate
+    public double? TracesSampleRate
     {
-        get => _tracesSampleRate ?? (EnableTracing is true ? 1.0 : 0.0);
+        get => _tracesSampleRate;
         set
         {
             if (value is < 0.0 or > 1.0)
             {
-                throw new InvalidOperationException(
-                    $"The value {value} is not a valid tracing sample rate. Use values between 0.0 and 1.0.");
+                throw new ArgumentOutOfRangeException(nameof(value), value,
+                    "The traces sample rate must be between 0.0 and 1.0, inclusive.");
             }
 
             _tracesSampleRate = value;
diff --git a/test/Sentry.Tests/ApiApprovalTests.Run.Core3_1.verified.txt b/test/Sentry.Tests/ApiApprovalTests.Run.Core3_1.verified.txt
@@ -617,7 +617,7 @@ namespace Sentry
         public Sentry.StackTraceMode StackTraceMode { get; set; }
         public System.Collections.Generic.ICollection<Sentry.SubstringOrRegexPattern> TagFilters { get; set; }
         public System.Collections.Generic.IList<Sentry.TracePropagationTarget> TracePropagationTargets { get; set; }
-        public double TracesSampleRate { get; set; }
+        public double? TracesSampleRate { get; set; }
         public System.Func<Sentry.TransactionSamplingContext, double?>? TracesSampler { get; set; }
         public Sentry.Extensibility.ITransport? Transport { get; set; }
         public bool UseAsyncFileIO { get; set; }
diff --git a/test/Sentry.Tests/ApiApprovalTests.Run.DotNet6_0.verified.txt b/test/Sentry.Tests/ApiApprovalTests.Run.DotNet6_0.verified.txt
@@ -618,7 +618,7 @@ namespace Sentry
         public Sentry.StackTraceMode StackTraceMode { get; set; }
         public System.Collections.Generic.ICollection<Sentry.SubstringOrRegexPattern> TagFilters { get; set; }
         public System.Collections.Generic.IList<Sentry.TracePropagationTarget> TracePropagationTargets { get; set; }
-        public double TracesSampleRate { get; set; }
+        public double? TracesSampleRate { get; set; }
         public System.Func<Sentry.TransactionSamplingContext, double?>? TracesSampler { get; set; }
         public Sentry.Extensibility.ITransport? Transport { get; set; }
         public bool UseAsyncFileIO { get; set; }
diff --git a/test/Sentry.Tests/ApiApprovalTests.Run.DotNet7_0.verified.txt b/test/Sentry.Tests/ApiApprovalTests.Run.DotNet7_0.verified.txt
@@ -618,7 +618,7 @@ namespace Sentry
         public Sentry.StackTraceMode StackTraceMode { get; set; }
         public System.Collections.Generic.ICollection<Sentry.SubstringOrRegexPattern> TagFilters { get; set; }
         public System.Collections.Generic.IList<Sentry.TracePropagationTarget> TracePropagationTargets { get; set; }
-        public double TracesSampleRate { get; set; }
+        public double? TracesSampleRate { get; set; }
         public System.Func<Sentry.TransactionSamplingContext, double?>? TracesSampler { get; set; }
         public Sentry.Extensibility.ITransport? Transport { get; set; }
         public bool UseAsyncFileIO { get; set; }
diff --git a/test/Sentry.Tests/ApiApprovalTests.Run.Net4_8.verified.txt b/test/Sentry.Tests/ApiApprovalTests.Run.Net4_8.verified.txt
@@ -616,7 +616,7 @@ namespace Sentry
         public Sentry.StackTraceMode StackTraceMode { get; set; }
         public System.Collections.Generic.ICollection<Sentry.SubstringOrRegexPattern> TagFilters { get; set; }
         public System.Collections.Generic.IList<Sentry.TracePropagationTarget> TracePropagationTargets { get; set; }
-        public double TracesSampleRate { get; set; }
+        public double? TracesSampleRate { get; set; }
         public System.Func<Sentry.TransactionSamplingContext, double?>? TracesSampler { get; set; }
         public Sentry.Extensibility.ITransport? Transport { get; set; }
         public bool UseAsyncFileIO { get; set; }
diff --git a/test/Sentry.Tests/SentryOptionsTests.cs b/test/Sentry.Tests/SentryOptionsTests.cs
@@ -1,8 +1,3 @@
-#if NETFRAMEWORK
-using Sentry.PlatformAbstractions;
-using Xunit.Sdk;
-#endif
-
 namespace Sentry.Tests;
 
 public class SentryOptionsTests
@@ -42,6 +37,20 @@ public void EnableTracing_Default_Null()
         Assert.Null(sut.EnableTracing);
     }
 
+    [Fact]
+    public void TracesSampleRate_Default_Null()
+    {
+        var sut = new SentryOptions();
+        Assert.Null(sut.TracesSampleRate);
+    }
+
+    [Fact]
+    public void TracesSampler_Default_Null()
+    {
+        var sut = new SentryOptions();
+        Assert.Null(sut.TracesSampler);
+    }
+
     [Fact]
     public void IsTracingEnabled_Default_False()
     {
@@ -50,39 +59,119 @@ public void IsTracingEnabled_Default_False()
     }
 
     [Fact]
-    public void EnableTracing_WhenNull()
+    public void IsTracingEnabled_EnableTracing_True()
     {
         var sut = new SentryOptions
         {
-            EnableTracing = null
+            EnableTracing = true
         };
 
-        Assert.Null(sut.EnableTracing);
-        Assert.Equal(0.0, sut.TracesSampleRate);
+        Assert.True(sut.IsTracingEnabled);
     }
 
     [Fact]
-    public void EnableTracing_WhenFalse()
+    public void IsTracingEnabled_EnableTracing_False()
     {
         var sut = new SentryOptions
         {
             EnableTracing = false
         };
 
-        Assert.False(sut.EnableTracing);
-        Assert.Equal(0.0, sut.TracesSampleRate);
+        Assert.False(sut.IsTracingEnabled);
     }
 
     [Fact]
-    public void EnableTracing_WhenTrue()
+    public void IsTracingEnabled_TracesSampleRate_Zero()
     {
         var sut = new SentryOptions
         {
-            EnableTracing = true
+            TracesSampleRate = 0.0
+        };
+
+        Assert.False(sut.IsTracingEnabled);
+    }
+
+    [Fact]
+    public void IsTracingEnabled_TracesSampleRate_GreaterThanZero()
+    {
+        var sut = new SentryOptions
+        {
+            TracesSampleRate = double.Epsilon
+        };
+
+        Assert.True(sut.IsTracingEnabled);
+    }
+
+    [Fact]
+    public void IsTracingEnabled_TracesSampleRate_LessThanZero()
+    {
+        var sut = new SentryOptions();
+        Assert.Throws<ArgumentOutOfRangeException>(() =>
+            sut.TracesSampleRate = -double.Epsilon);
+    }
+
+    [Fact]
+    public void IsTracingEnabled_TracesSampleRate_GreaterThanOne()
+    {
+        var sut = new SentryOptions();
+        Assert.Throws<ArgumentOutOfRangeException>(() =>
+            sut.TracesSampleRate = 1.0000000000000002);
+    }
+
+    [Fact]
+    public void IsTracingEnabled_TracesSampler_Provided()
+    {
+        var sut = new SentryOptions
+        {
+            TracesSampler = _ => null
+        };
+
+        Assert.True(sut.IsTracingEnabled);
+    }
+
+    [Fact]
+    public void IsTracingEnabled_EnableTracing_True_TracesSampleRate_Zero()
+    {
+        // Edge Case:
+        //   Tracing enabled, but sample rate set to zero, and no sampler function, should be treated as disabled.
+
+        var sut = new SentryOptions
+        {
+            EnableTracing = true,
+            TracesSampleRate = 0.0
+        };
+
+        Assert.False(sut.IsTracingEnabled);
+    }
+
+    [Fact]
+    public void IsTracingEnabled_EnableTracing_False_TracesSampleRate_One()
+    {
+        // Edge Case:
+        //   Tracing disabled should be treated as disabled regardless of sample rate set.
+
+        var sut = new SentryOptions
+        {
+            EnableTracing = false,
+            TracesSampleRate = 1.0
+        };
+
+        Assert.False(sut.IsTracingEnabled);
+    }
+
+    [Fact]
+    public void IsTracingEnabled_EnableTracing_False_TracesSampler_Provided()
+    {
+        // Edge Case:
+        //   Tracing disabled should be treated as disabled regardless of sampler function set.
+
+        var sut = new SentryOptions
+        {
+            EnableTracing = false,
+            TracesSampler = _ => null
         };
 
-        Assert.True(sut.EnableTracing);
-        Assert.Equal(1.0, sut.TracesSampleRate);
+        Assert.False(sut.IsTracingEnabled);
     }
 
     [Fact]

Original file line number	Diff line number	Diff line change
`@@ -17,7 +17,6 @@`
`17`	`17`	`"test\\Sentry.Extensions.Logging.Tests\\Sentry.Extensions.Logging.Tests.csproj",`
`18`	`18`	`"test\\Sentry.Maui.Device.TestApp\\Sentry.Maui.Device.TestApp.csproj",`
`19`	`19`	`"test\\Sentry.Maui.Tests\\Sentry.Maui.Tests.csproj",`
`20`		`- "test\\Sentry.Testing.CrashableApp\\Sentry.Testing.CrashableApp.csproj",`
`21`	`20`	`"test\\Sentry.Testing\\Sentry.Testing.csproj",`
`22`	`21`	`"test\\Sentry.Tests\\Sentry.Tests.csproj"`
`23`	`22`	`]`
Original file line number	Diff line number	Diff line change
`@@ -148,7 +148,7 @@ internal ITransaction StartTransaction(`
`148`	`148`	`// Random sampling runs only if the sampling decision hasn't been made already.`
`149`	`149`	`if (transaction.IsSampled == null)`
`150`	`150`	`{`
`151`		`- var sampleRate = _options.TracesSampleRate;`
	`151`	`+ var sampleRate = _options.TracesSampleRate ?? (_options.EnableTracing is true ? 1.0 : 0.0);`
`152`	`152`	`transaction.IsSampled = _randomValuesFactory.NextBool(sampleRate);`
`153`	`153`	`transaction.SampleRate = sampleRate;`
`154`	`154`	`}`
Original file line number	Diff line number	Diff line change
`@@ -66,8 +66,13 @@ private static void InitSentryCocoaSdk(SentryOptions options)`
`66`	`66`	`}`
`67`	`67`
`68`	`68`	`// These options we have behind feature flags`
`69`		`- if (options.IsTracingEnabled && options.iOS.EnableCocoaSdkTracing)`
	`69`	`+ if (options is {IsTracingEnabled: true, iOS.EnableCocoaSdkTracing: true})`
`70`	`70`	`{`
	`71`	`+ if (options.EnableTracing != null)`
	`72`	`+ {`
	`73`	`+ cocoaOptions.EnableTracing = options.EnableTracing.Value;`
	`74`	`+ }`
	`75`	`+`
`71`	`76`	`cocoaOptions.TracesSampleRate = options.TracesSampleRate;`
`72`	`77`
`73`	`78`	`if (options.TracesSampler is { } tracesSampler)`