Support transaction finishing automatically with 'idle timeout'

CHANGELOG.md

@@ -8,6 +8,7 @@
 - Added a MSBuild property `SentryUploadAndroidProguardMapping` to automatically upload the Proguard mapping file when targeting Android ([#2455](https://github.com/getsentry/sentry-dotnet/pull/2455))
 - Symbolication for Single File Apps ([#2425](https://github.com/getsentry/sentry-dotnet/pull/2425))
 - Add binding to `SwiftAsyncStacktraces` on iOS ([#2436](https://github.com/getsentry/sentry-dotnet/pull/2436))
+- Support transaction finishing automatically with 'idle timeout' ([#2452](https://github.com/getsentry/sentry-dotnet/pull/2452))
 
 ### Fixes
 

CONTRIBUTING.md

@@ -103,6 +103,16 @@ Below that, you'll add the heading 3 mentioned above. For example, if you're add
 
 There's a GitHub action check to verify if an entry was added. If the entry isn't a user-facing change, you can skip the verification with `#skip-changelog` written to the PR description. The bot writes a comment in the PR with a suggestion entry to the changelog based on the PR title.
 
+## Naming tests
+
+Ideally we like tests to be named following the convention `Method_Context_Expectation`.
+
+[For example](https://github.com/getsentry/sentry-dotnet/blob/ebd70ffafd5f8bd5eb6bb9ee1a03cac77ae67b8d/test/Sentry.Tests/HubTests.cs#L43C1-L44C68):
+```csharp
+    [Fact]
+    public void PushScope_BreadcrumbWithinScope_NotVisibleOutside()
+```
+
 ## Verify tests
 
 Some tests use [Verify](https://github.com/VerifyTests/Verify) to check returned objects against snapshots that are part of the repo.

src/Sentry.OpenTelemetry/SentrySpanProcessor.cs

@@ -105,10 +105,20 @@ public override void OnEnd(Activity data)
         if (attributes.TryGetTypedValue("http.url", out string? url) && (_options?.IsSentryRequest(url) ?? false))
         {
             _options?.DiagnosticLogger?.LogDebug($"Ignoring Activity {data.SpanId} for Sentry request.");
-            if (_map.TryRemove(data.SpanId, out var removed) && (removed is SpanTracer spanTracerToRemove))
+
+            if (_map.TryRemove(data.SpanId, out var removed))
             {
-                spanTracerToRemove.IsSentryRequest = true;
+                if (removed is SpanTracer spanTracerToRemove)
+                {
+                    spanTracerToRemove.IsSentryRequest = true;
+                }
+
+                if (removed is TransactionTracer transactionTracer)
+                {
+                    transactionTracer.IsSentryRequest = true;
+                }
             }
+
             return;
         }
 

src/Sentry/TransactionTracer.cs

@@ -10,6 +10,8 @@ namespace Sentry;
 public class TransactionTracer : ITransaction, IHasDistribution, IHasTransactionNameSource, IHasMeasurements
 {
     private readonly IHub _hub;
+    private readonly Timer? _idleTimer;
+    private long _idleTimerStopped;
     private readonly SentryStopwatch _stopwatch = SentryStopwatch.StartNew();
     private readonly Instrumenter _instrumenter = Instrumenter.Sentry;
 
@@ -178,6 +180,15 @@ public IReadOnlyList<string> Fingerprint
 
     internal ITransactionProfiler? TransactionProfiler { get; set; }
 
+    /// <summary>
+    /// Used by the Sentry.OpenTelemetry.SentrySpanProcessor to mark a transaction as a Sentry request. Ideally we wouldn't
+    /// create this transaction but since we can't avoid doing that, once we detect that it's a Sentry request we mark it
+    /// as such so that we can prevent finishing the transaction tracer when idle timeout elapses and the TransactionTracer gets converted into
+    /// a Transaction.
+    /// </summary>
+    internal bool IsSentryRequest { get; set; }
+
+    // TODO: mark as internal in version 4
     /// <summary>
     /// Initializes an instance of <see cref="Transaction"/>.
     /// </summary>
@@ -186,6 +197,7 @@ public TransactionTracer(IHub hub, string name, string operation)
     {
     }
 
+    // TODO: mark as internal in version 4
     /// <summary>
     /// Initializes an instance of <see cref="Transaction"/>.
     /// </summary>
@@ -203,7 +215,14 @@ public TransactionTracer(IHub hub, string name, string operation, TransactionNam
     /// <summary>
     /// Initializes an instance of <see cref="TransactionTracer"/>.
     /// </summary>
-    public TransactionTracer(IHub hub, ITransactionContext context)
+    public TransactionTracer(IHub hub, ITransactionContext context) : this(hub, context, null)
+    {
+    }
+
+    /// <summary>
+    /// Initializes an instance of <see cref="TransactionTracer"/>.
+    /// </summary>
+    internal TransactionTracer(IHub hub, ITransactionContext context, TimeSpan? idleTimeout = null)
     {
         _hub = hub;
         Name = context.Name;
@@ -215,12 +234,25 @@ public TransactionTracer(IHub hub, ITransactionContext context)
         Description = context.Description;
         Status = context.Status;
         IsSampled = context.IsSampled;
-        StartTimestamp = _stopwatch.StartDateTimeOffset;
 
-        if (context is TransactionContext transactionContext)
+		if (context is TransactionContext transactionContext)
         {
             _instrumenter = transactionContext.Instrumenter;
         }
+
+        // Set idle timer only if an idle timeout has been provided directly
+        if (idleTimeout.HasValue)
+        {
+            _idleTimer = new Timer(state =>
+            {
+                if (state is not TransactionTracer transactionTracer)
+                {
+                    return;
+                }
+
+                transactionTracer.Finish(Status ?? SpanStatus.Ok);
+            }, this, idleTimeout.Value, Timeout.InfiniteTimeSpan);
+        }
     }
 
     /// <inheritdoc />
@@ -278,6 +310,18 @@ private void AddChildSpan(SpanTracer span)
     /// <inheritdoc />
     public void Finish()
     {
+        if (Interlocked.Exchange(ref _idleTimerStopped, 1) == 0)
+        {
+            _idleTimer?.Change(Timeout.InfiniteTimeSpan, Timeout.InfiniteTimeSpan);
+
+            _idleTimer?.Dispose();
+        }
+
+        if (IsSentryRequest)
+        {
+            return;
+        }
+
         TransactionProfiler?.Finish();
         Status ??= SpanStatus.Ok;
         EndTimestamp ??= _stopwatch.CurrentDateTimeOffset;

test/Sentry.OpenTelemetry.Tests/SentrySpanProcessorTests.cs

@@ -238,19 +238,17 @@ public void OnEnd_FinishesSpan()
 
         using (new AssertionScope())
         {
-            using (new AssertionScope())
+            spanTracer.ParentSpanId.Should().Be(parent.SpanId.AsSentrySpanId());
+            spanTracer.Operation.Should().Be(data.OperationName);
+            spanTracer.Description.Should().Be(data.DisplayName);
+            spanTracer.EndTimestamp.Should().NotBeNull();
+            spanTracer.Extra["otel.kind"].Should().Be(data.Kind);
+            foreach (var keyValuePair in tags)
             {
-                spanTracer.ParentSpanId.Should().Be(parent.SpanId.AsSentrySpanId());
-                spanTracer.Operation.Should().Be(data.OperationName);
-                spanTracer.Description.Should().Be(data.DisplayName);
-                spanTracer.EndTimestamp.Should().NotBeNull();
-                spanTracer.Extra["otel.kind"].Should().Be(data.Kind);
-                foreach (var keyValuePair in tags)
-                {
-                    span.Extra[keyValuePair.Key].Should().Be(keyValuePair.Value);
-                }
-                spanTracer.Status.Should().Be(SpanStatus.Ok);
+                span.Extra[keyValuePair.Key].Should().Be(keyValuePair.Value);
             }
+
+            spanTracer.Status.Should().Be(SpanStatus.Ok);
         }
     }
 
@@ -282,19 +280,44 @@ public void OnEnd_FinishesTransaction()
 
         using (new AssertionScope())
         {
-            using (new AssertionScope())
+            transaction.ParentSpanId.Should().Be(new ActivitySpanId().AsSentrySpanId());
+            transaction.Operation.Should().Be(data.OperationName);
+            transaction.Description.Should().Be(data.DisplayName);
+            transaction.Name.Should().Be(data.DisplayName);
+            transaction.NameSource.Should().Be(TransactionNameSource.Custom);
+            transaction.EndTimestamp.Should().NotBeNull();
+            transaction.Contexts["otel"].Should().BeEquivalentTo(new Dictionary<string, object>
             {
-                transaction.ParentSpanId.Should().Be(new ActivitySpanId().AsSentrySpanId());
-                transaction.Operation.Should().Be(data.OperationName);
-                transaction.Description.Should().Be(data.DisplayName);
-                transaction.Name.Should().Be(data.DisplayName);
-                transaction.NameSource.Should().Be(TransactionNameSource.Custom);
-                transaction.EndTimestamp.Should().NotBeNull();
-                transaction.Contexts["otel"].Should().BeEquivalentTo(new Dictionary<string, object>{
-                    { "attributes", tags }
-                });
-                transaction.Status.Should().Be(SpanStatus.Ok);
-            }
+                { "attributes", tags }
+            });
+            transaction.Status.Should().Be(SpanStatus.Ok);
+        }
+    }
+
+    [Fact]
+    public void OnEnd_IsSentryRequest_DoesNotFinishTransaction()
+    {
+        // Arrange
+        _fixture.Options.Instrumenter = Instrumenter.OpenTelemetry;
+        var sut = _fixture.GetSut();
+
+        var tags = new Dictionary<string, object> { { "foo", "bar" }, { "http.url", _fixture.Options.Dsn } };
+        var data = Tracer.StartActivity(name: "test operation", kind: ActivityKind.Internal, parentContext: default, tags)!;
+        data.DisplayName = "test display name";
+        sut.OnStart(data);
+
+        sut._map.TryGetValue(data.SpanId, out var span);
+
+        // Act
+        sut.OnEnd(data);
+
+        // Assert
+        if (span is not TransactionTracer transaction)
+        {
+            Assert.Fail("Span is not a transaction tracer");
+            return;
         }
+
+        transaction.IsSentryRequest.Should().BeTrue();
     }
 }

test/Sentry.Tests/Protocol/TransactionTests.cs

@@ -388,6 +388,41 @@ public void Finish_SentryRequestSpansGetIgnored()
         transaction.Spans.Should().NotContain(s => s.Operation == "sentryRequest");
     }
 
+    [Fact]
+    public async Task Finish_SentryRequestTransactionGetsIgnored()
+    {
+        // Arrange
+        var client = Substitute.For<ISentryClient>();
+        var options = new SentryOptions
+        {
+            Dsn = ValidDsn,
+        };
+        var hub = new Hub(options, client);
+        var context = new TransactionContext(
+            SpanId.Create(),
+            SpanId.Create(),
+            SentryId.Create(),
+            "my name",
+            "my operation",
+            "description",
+            SpanStatus.Ok,
+            null,
+            true,
+            TransactionNameSource.Component
+        );
+
+        var transaction = new TransactionTracer(hub, context, TimeSpan.FromMilliseconds(2))
+        {
+            IsSentryRequest = true
+        };
+
+        // Act
+        await Task.Delay(TimeSpan.FromMilliseconds(5));
+
+        // Assert
+        transaction.IsFinished.Should().BeFalse();
+    }
+
     [Fact]
     public void Finish_CapturesTransaction()
     {
@@ -526,4 +561,38 @@ public void ISpan_GetTransaction_FromSpan()
         // Assert
         Assert.Same(transaction, result);
     }
+
+    [Fact]
+    public async Task NewTransactionTracer_IdleTimeoutProvided_AutomaticallyFinishes()
+    {
+        // Arrange
+        var client = Substitute.For<ISentryClient>();
+        var options = new SentryOptions
+        {
+            Dsn = ValidDsn,
+            Debug = true
+        };
+        var hub = new Hub(options, client);
+        var context = new TransactionContext(
+            SpanId.Create(),
+            SpanId.Create(),
+            SentryId.Create(),
+            "my name",
+            "my operation",
+            "description",
+            SpanStatus.Ok,
+            null,
+            true,
+            TransactionNameSource.Component
+        );
+
+        var transaction = new TransactionTracer(hub, context, TimeSpan.FromMilliseconds(2));
+
+        // Act
+        await Task.Delay(TimeSpan.FromSeconds(2));
+
+        // Assert
+        transaction.IsFinished.Should().BeTrue();
+    }
+
 }