diff --git a/CHANGELOG.md b/CHANGELOG.md index 02a52e09dd..ece24602a0 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 0ffe15b279..6e7096ff48 100644 --- a/CONTRIBUTING.md +++ b/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. diff --git a/src/Sentry.OpenTelemetry/SentrySpanProcessor.cs b/src/Sentry.OpenTelemetry/SentrySpanProcessor.cs index 0f7f1801b0..0e42cfba99 100644 --- a/src/Sentry.OpenTelemetry/SentrySpanProcessor.cs +++ b/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; } diff --git a/src/Sentry/TransactionTracer.cs b/src/Sentry/TransactionTracer.cs index 3cf14d2a7e..ac4bc64d02 100644 --- a/src/Sentry/TransactionTracer.cs +++ b/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 Fingerprint internal ITransactionProfiler? TransactionProfiler { get; set; } + /// + /// 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. + /// + internal bool IsSentryRequest { get; set; } + + // TODO: mark as internal in version 4 /// /// Initializes an instance of . /// @@ -186,6 +197,7 @@ public TransactionTracer(IHub hub, string name, string operation) { } + // TODO: mark as internal in version 4 /// /// Initializes an instance of . /// @@ -203,7 +215,14 @@ public TransactionTracer(IHub hub, string name, string operation, TransactionNam /// /// Initializes an instance of . /// - public TransactionTracer(IHub hub, ITransactionContext context) + public TransactionTracer(IHub hub, ITransactionContext context) : this(hub, context, null) + { + } + + /// + /// Initializes an instance of . + /// + 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); + } } /// @@ -278,6 +310,18 @@ private void AddChildSpan(SpanTracer span) /// 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; diff --git a/test/Sentry.OpenTelemetry.Tests/SentrySpanProcessorTests.cs b/test/Sentry.OpenTelemetry.Tests/SentrySpanProcessorTests.cs index e23e6a525b..b90e96f15c 100644 --- a/test/Sentry.OpenTelemetry.Tests/SentrySpanProcessorTests.cs +++ b/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 { - 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{ - { "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 { { "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(); } } diff --git a/test/Sentry.Tests/Protocol/TransactionTests.cs b/test/Sentry.Tests/Protocol/TransactionTests.cs index 6a523c8433..c48ba870c0 100644 --- a/test/Sentry.Tests/Protocol/TransactionTests.cs +++ b/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(); + 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(); + 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(); + } + }