diff --git a/CHANGELOG.md b/CHANGELOG.md index 0eb9e149b9..49f5d9abfe 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ ### Fixes +- InvalidCastException in SentrySpanProcessor when using the Sentry.OpenTelemetry integration ([#4245](https://github.com/getsentry/sentry-dotnet/pull/4245)) - Fix InApp Exclude for frames without Module by checking against frame's Package ([#4236](https://github.com/getsentry/sentry-dotnet/pull/4236)) ## 5.9.0 diff --git a/src/Sentry.OpenTelemetry/SentrySpanProcessor.cs b/src/Sentry.OpenTelemetry/SentrySpanProcessor.cs index b7942bb21b..b0b747e2d4 100644 --- a/src/Sentry.OpenTelemetry/SentrySpanProcessor.cs +++ b/src/Sentry.OpenTelemetry/SentrySpanProcessor.cs @@ -133,12 +133,15 @@ private void CreateChildSpan(Activity data, ISpan parentSpan, SpanId? parentSpan Instrumenter = Instrumenter.OpenTelemetry }; - var span = (SpanTracer)parentSpan.StartChild(context); - span.Origin = OpenTelemetryOrigin; - span.StartTimestamp = data.StartTimeUtc; - // Used to filter out spans that are not recorded when finishing a transaction. - span.SetFused(data); - span.IsFiltered = () => span.GetFused() is { IsAllDataRequested: false, Recorded: false }; + var span = parentSpan.StartChild(context); + if (span is SpanTracer spanTracer) + { + spanTracer.Origin = OpenTelemetryOrigin; + spanTracer.StartTimestamp = data.StartTimeUtc; + // Used to filter out spans that are not recorded when finishing a transaction. + spanTracer.SetFused(data); + spanTracer.IsFiltered = () => spanTracer.GetFused() is { IsAllDataRequested: false, Recorded: false }; + } _map[data.SpanId] = span; } @@ -161,17 +164,19 @@ private void CreateRootSpan(Activity data) var baggageHeader = data.Baggage.AsBaggageHeader(); var dynamicSamplingContext = baggageHeader.CreateDynamicSamplingContext(_replaySession); - var transaction = (TransactionTracer)_hub.StartTransaction( + var transaction = _hub.StartTransaction( transactionContext, new Dictionary(), dynamicSamplingContext ); - transaction.Contexts.Trace.Origin = OpenTelemetryOrigin; - transaction.StartTimestamp = data.StartTimeUtc; + if (transaction is TransactionTracer tracer) + { + tracer.Contexts.Trace.Origin = OpenTelemetryOrigin; + tracer.StartTimestamp = data.StartTimeUtc; + } _hub.ConfigureScope(scope => scope.Transaction = transaction); transaction.SetFused(data); _map[data.SpanId] = transaction; } - /// public override void OnEnd(Activity data) { @@ -236,15 +241,15 @@ public override void OnEnd(Activity data) // Transactions set otel attributes (and resource attributes) as context. transaction.Contexts["otel"] = GetOtelContext(attributes); } - else + else if (span is SpanTracer spanTracer) { // Use the end timestamp from the activity data. - ((SpanTracer)span).EndTimestamp = data.StartTimeUtc + data.Duration; + spanTracer.EndTimestamp = data.StartTimeUtc + data.Duration; // Spans set otel attributes in extras (passed to Sentry as "data" on the span). // Resource attributes do not need to be set, as they would be identical as those set on the transaction. - span.SetExtras(attributes); - span.SetExtra("otel.kind", data.Kind); + spanTracer.SetExtras(attributes); + spanTracer.SetExtra("otel.kind", data.Kind); } // In ASP.NET Core the middleware finishes up (and the scope gets popped) before the activity is ended. So we diff --git a/src/Sentry/ISpan.cs b/src/Sentry/ISpan.cs index 0259ac02c0..5cf7193a9c 100644 --- a/src/Sentry/ISpan.cs +++ b/src/Sentry/ISpan.cs @@ -70,15 +70,21 @@ public static ISpan StartChild(this ISpan span, string operation, string? descri internal static ISpan StartChild(this ISpan span, SpanContext context) { - var transaction = span.GetTransaction() as TransactionTracer; - if (transaction?.StartChild(context.SpanId, span.SpanId, context.Operation, context.Instrumenter) - is not SpanTracer childSpan) + var transaction = span.GetTransaction(); + if (transaction is TransactionTracer transactionTracer) { - return NoOpSpan.Instance; + var child = transactionTracer.StartChild(context.SpanId, span.SpanId, context.Operation, context.Instrumenter); + if (child is SpanTracer childTracer) + { + childTracer.Description = context.Description; + return childTracer; + } } - - childSpan.Description = context.Description; - return childSpan; + if (transaction is UnsampledTransaction unsampledTransaction) + { + return unsampledTransaction.StartChild(context.Operation, context.SpanId); + } + return NoOpSpan.Instance; } /// @@ -88,7 +94,8 @@ public static ITransactionTracer GetTransaction(this ISpan span) => span switch { ITransactionTracer transaction => transaction, - SpanTracer tracer => tracer.Transaction, + UnsampledSpan unsampledSpan => unsampledSpan.Transaction, + SpanTracer spanTracer => spanTracer.Transaction, _ => throw new ArgumentOutOfRangeException(nameof(span), span, null) }; diff --git a/src/Sentry/Internal/UnsampledSpan.cs b/src/Sentry/Internal/UnsampledSpan.cs new file mode 100644 index 0000000000..3b52bdaa8c --- /dev/null +++ b/src/Sentry/Internal/UnsampledSpan.cs @@ -0,0 +1,9 @@ +namespace Sentry.Internal; + +internal sealed class UnsampledSpan(UnsampledTransaction transaction, SpanId? spanId = null) : NoOpSpan +{ + public override bool? IsSampled => false; + public override SpanId SpanId { get; } = spanId ?? SpanId.Empty; + internal UnsampledTransaction Transaction => transaction; + public override ISpan StartChild(string operation) => transaction.StartChild(operation); +} diff --git a/src/Sentry/Internal/UnsampledTransaction.cs b/src/Sentry/Internal/UnsampledTransaction.cs index 277d909e73..20b8ad013c 100644 --- a/src/Sentry/Internal/UnsampledTransaction.cs +++ b/src/Sentry/Internal/UnsampledTransaction.cs @@ -94,10 +94,10 @@ public override ISpan StartChild(string operation) return span; } - private class UnsampledSpan(UnsampledTransaction transaction) : NoOpSpan + public ISpan StartChild(string operation, SpanId spanId) { - public override ISpan StartChild(string operation) => transaction.StartChild(operation); - - public override bool? IsSampled => false; + var span = new UnsampledSpan(this, spanId); + _spans.Add(span); + return span; } } diff --git a/test/Sentry.OpenTelemetry.Tests/SentrySpanProcessorTests.cs b/test/Sentry.OpenTelemetry.Tests/SentrySpanProcessorTests.cs index 67a13d7da0..15ad64b8e0 100644 --- a/test/Sentry.OpenTelemetry.Tests/SentrySpanProcessorTests.cs +++ b/test/Sentry.OpenTelemetry.Tests/SentrySpanProcessorTests.cs @@ -146,10 +146,11 @@ public void OnStart_Transaction_With_DynamicSamplingContext() } [Fact] - public void OnStart_WithParentSpanId_StartsChildSpan() + public void OnStart_SampledWithParentSpanId_StartsChildSpan() { // Arrange _fixture.Options.Instrumenter = Instrumenter.OpenTelemetry; + _fixture.Options.TracesSampleRate = 1.0; var sut = _fixture.GetSut(); using var parent = Tracer.StartActivity("Parent"); @@ -164,16 +165,18 @@ public void OnStart_WithParentSpanId_StartsChildSpan() Assert.True(sut._map.TryGetValue(data.SpanId, out var span)); using (new AssertionScope()) { - span.Should().BeOfType(); + span.IsSampled.Should().Be(true); span.SpanId.Should().Be(data.SpanId.AsSentrySpanId()); + if (span is not SpanTracer spanTracer) { Assert.Fail("Span is not a span tracer"); return; } + + span.SpanId.Should().Be(data.SpanId.AsSentrySpanId()); using (new AssertionScope()) { - spanTracer.SpanId.Should().Be(data.SpanId.AsSentrySpanId()); spanTracer.ParentSpanId.Should().Be(data.ParentSpanId.AsSentrySpanId()); spanTracer.TraceId.Should().Be(data.TraceId.AsSentryId()); spanTracer.Operation.Should().Be(data.OperationName); @@ -184,6 +187,32 @@ public void OnStart_WithParentSpanId_StartsChildSpan() } } + [Fact] + public void OnStart_NotSampledWithParentSpanId_StartsChildSpan() + { + // Arrange + _fixture.Options.Instrumenter = Instrumenter.OpenTelemetry; + _fixture.Options.TracesSampleRate = 0.0; + var sut = _fixture.GetSut(); + + using var parent = Tracer.StartActivity("Parent"); + sut.OnStart(parent); + + using var data = Tracer.StartActivity("TestActivity"); + + // Act + sut.OnStart(data!); + + // Assert + Assert.True(sut._map.TryGetValue(data.SpanId, out var span)); + using (new AssertionScope()) + { + span.IsSampled.Should().Be(false); + span.SpanId.Should().Be(data.SpanId.AsSentrySpanId()); + span.Should().BeOfType(); + } + } + [Fact] public void OnStart_WithSentryParentSpanId_StartsChildSpan() { @@ -257,10 +286,11 @@ public void StartSpan_UsingSentryTracing_StartsChildSpan() } [Fact] - public void OnStart_WithoutParentSpanId_StartsNewTransaction() + public void OnStart_SampledWithoutParentSpanId_StartsNewTransaction() { // Arrange _fixture.Options.Instrumenter = Instrumenter.OpenTelemetry; + _fixture.Options.TracesSampleRate = 1.0; _fixture.ScopeManager = Substitute.For(); var scope = new Scope(); var clientScope = new KeyValuePair(scope, _fixture.Client); @@ -279,6 +309,7 @@ public void OnStart_WithoutParentSpanId_StartsNewTransaction() Assert.Fail("Span is not a transaction tracer"); return; } + using (new AssertionScope()) { transaction.SpanId.Should().Be(data.SpanId.AsSentrySpanId()); @@ -293,7 +324,39 @@ public void OnStart_WithoutParentSpanId_StartsNewTransaction() } [Fact] - public void OnEnd_FinishesSpan() + public void OnStart_NotSampledWithoutParentSpanId_StartsNewTransaction() + { + // Arrange + _fixture.Options.Instrumenter = Instrumenter.OpenTelemetry; + _fixture.Options.TracesSampleRate = 0.0; + _fixture.ScopeManager = Substitute.For(); + var scope = new Scope(); + var clientScope = new KeyValuePair(scope, _fixture.Client); + _fixture.ScopeManager.GetCurrent().Returns(clientScope); + var sut = _fixture.GetSut(); + + var data = Tracer.StartActivity("test op"); + + // Act + sut.OnStart(data!); + + // Assert + Assert.True(sut._map.TryGetValue(data.SpanId, out var span)); + if (span is not UnsampledTransaction transaction) + { + Assert.Fail("Span is not an unsampled transaction"); + return; + } + + using (new AssertionScope()) + { + transaction.SpanId.Should().Be(data.SpanId.AsSentrySpanId()); + transaction.TraceId.Should().Be(data.TraceId.AsSentryId()); + } + } + + [Fact] + public void OnEnd_Sampled_Span_FinishesSpan() { // Arrange _fixture.Options.Instrumenter = Instrumenter.OpenTelemetry; @@ -338,6 +401,34 @@ public void OnEnd_FinishesSpan() } } + [Fact] + public void OnEnd_Unsampled_Span_DoesNotThrow() + { + // Arrange + _fixture.Options.Instrumenter = Instrumenter.OpenTelemetry; + _fixture.Options.TracesSampleRate = 0.0; + var sut = _fixture.GetSut(); + + var parent = Tracer.StartActivity(name: "transaction")!; + sut.OnStart(parent); + + Dictionary tags = []; + 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 + span.Should().BeOfType(); + + // There's nothing else to assert here, as long as calling OnEnd does not throw an exception, + // UnsampledSpan.Finish() is basically a no-op. + } + [Fact] public void OnEnd_Transaction_RestoresSavedScope() { @@ -418,7 +509,7 @@ public void OnEnd_SpansEnriched() } [Fact] - public void OnEnd_FinishesTransaction() + public void OnEnd_Sampled_FinishesTransaction() { // Arrange _fixture.Options.Instrumenter = Instrumenter.OpenTelemetry; @@ -460,6 +551,33 @@ public void OnEnd_FinishesTransaction() } } + [Fact] + public void OnEnd_NotSampled_FinishesTransaction() + { + // Arrange + _fixture.Options.Instrumenter = Instrumenter.OpenTelemetry; + _fixture.Options.TracesSampleRate = 0.0; + var sut = _fixture.GetSut(); + + Dictionary tags = []; + 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 UnsampledTransaction transaction) + { + Assert.Fail("Span is not an unsampled transaction"); + return; + } + transaction.IsFinished.Should().BeTrue(); + } + [Fact] public void OnEnd_FilteredTransaction_DoesNotFinishTransaction() {