Fix EnableTracing option conflict with TracesSampleRate (#2368)

* Fix tracing options

* Update tests

* Update API snapshots

* Fix unrelated solution filter

* Update CHANGELOG.md

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))

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"
     ]

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;
             }

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)

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)

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;

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; }

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; }

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; }

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; }

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]