Allow OpenTelemetry child spans for Sentry Transactions (#3288)

CHANGELOG.md

@@ -2,6 +2,10 @@
 
 ## Unreleased
 
+### Features
+
+- The SDK's performance API now works in conjunction with OpenTelemetry's instrumentation. This means that SentrySpans and OTel spans now show up in the same span-tree. ([#3288](https://github.com/getsentry/sentry-dotnet/pull/3288))
+
 ### Fixes
 
 - `HttpResponse.Content` is no longer disposed by when using `SentryHttpFailedRequestHandler` on .NET Framework, which was causing an ObjectDisposedException when using Sentry with NSwag ([#3306](https://github.com/getsentry/sentry-dotnet/pull/3306))

src/Sentry.OpenTelemetry/SentrySpanProcessor.cs

@@ -82,59 +82,82 @@ internal SentrySpanProcessor(IHub hub, IEnumerable<IOpenTelemetryEnricher>? enri
     /// <inheritdoc />
     public override void OnStart(Activity data)
     {
-        if (data.ParentSpanId != default && _map.TryGetValue(data.ParentSpanId, out var parentSpan))
+        if (data.ParentSpanId != default && _map.TryGetValue(data.ParentSpanId, out var mappedParent))
         {
-            // We can find the parent span - start a child span.
-            var context = new SpanContext(
-                data.OperationName,
-                data.SpanId.AsSentrySpanId(),
-                data.ParentSpanId.AsSentrySpanId(),
-                data.TraceId.AsSentryId(),
-                data.DisplayName,
-                null,
-                null)
-            {
-                Instrumenter = Instrumenter.OpenTelemetry
-            };
-
-            var span = (SpanTracer)parentSpan.StartChild(context);
-            span.StartTimestamp = data.StartTimeUtc;
-            // Used to filter out spans that are not recorded when finishing a transaction.
-            span.SetFused(data);
-            span.IsFiltered = () => span.GetFused<Activity>() is { IsAllDataRequested: false, Recorded: false };
-            _map[data.SpanId] = span;
+            // Explicit ParentSpanId of another activity that we have already mapped
+            CreateChildSpan(data, mappedParent, data.ParentSpanId);
+        }
+        // Note if the current span on the hub is OTel instrumented and is not the parent of `data` then this may be
+        // intentional (see https://opentelemetry.io/docs/languages/net/instrumentation/#creating-new-root-activities)
+        // so we explicitly exclude OTel instrumented spans from the following check.
+        // of Sentry
+        else if (_hub.GetSpan() is IBaseTracer { IsOtelInstrumenter: false } inferredParent)
+        {
+            // When mixing Sentry and OTel instrumentation and we infer that the currently active span is the parent.
+            var inferredParentSpan = (ISpan)inferredParent;
+            CreateChildSpan(data, inferredParentSpan, inferredParentSpan.SpanId);
         }
         else
         {
-            // If a parent exists at all, then copy its sampling decision.
-            bool? isSampled = data.HasRemoteParent ? data.Recorded : null;
-
-            // No parent span found - start a new transaction
-            var transactionContext = new TransactionContext(data.DisplayName,
-                data.OperationName,
-                data.SpanId.AsSentrySpanId(),
-                data.ParentSpanId.AsSentrySpanId(),
-                data.TraceId.AsSentryId(),
-                data.DisplayName, null, isSampled, isSampled)
-            {
-                Instrumenter = Instrumenter.OpenTelemetry
-            };
-
-            var baggageHeader = data.Baggage.AsBaggageHeader();
-            var dynamicSamplingContext = baggageHeader.CreateDynamicSamplingContext();
-            var transaction = (TransactionTracer)_hub.StartTransaction(
-                transactionContext, new Dictionary<string, object?>(), dynamicSamplingContext
-                );
-            transaction.StartTimestamp = data.StartTimeUtc;
-            _hub.ConfigureScope(scope => scope.Transaction = transaction);
-            transaction.SetFused(data);
-            _map[data.SpanId] = transaction;
+            CreateRootSpan(data);
         }
 
         // Housekeeping
         PruneFilteredSpans();
     }
 
+    private void CreateChildSpan(Activity data, ISpan parentSpan, ActivitySpanId? parentSpanId = null)
+        => CreateChildSpan(data, parentSpan, parentSpanId?.AsSentrySpanId());
+
+    private void CreateChildSpan(Activity data, ISpan parentSpan, SpanId? parentSpanId = null)
+    {
+        // We can find the parent span - start a child span.
+        var context = new SpanContext(
+            data.OperationName,
+            data.SpanId.AsSentrySpanId(),
+            parentSpanId,
+            description: data.DisplayName
+        )
+        {
+            Instrumenter = Instrumenter.OpenTelemetry
+        };
+
+        var span = (SpanTracer)parentSpan.StartChild(context);
+        span.StartTimestamp = data.StartTimeUtc;
+        // Used to filter out spans that are not recorded when finishing a transaction.
+        span.SetFused(data);
+        span.IsFiltered = () => span.GetFused<Activity>() is { IsAllDataRequested: false, Recorded: false };
+        _map[data.SpanId] = span;
+    }
+
+    private void CreateRootSpan(Activity data)
+    {
+        // If a parent exists at all, then copy its sampling decision.
+        bool? isSampled = data.HasRemoteParent ? data.Recorded : null;
+
+        // No parent span found - start a new transaction
+        var transactionContext = new TransactionContext(data.DisplayName,
+            data.OperationName,
+            data.SpanId.AsSentrySpanId(),
+            data.ParentSpanId.AsSentrySpanId(),
+            data.TraceId.AsSentryId(),
+            data.DisplayName, null, isSampled, isSampled)
+        {
+            Instrumenter = Instrumenter.OpenTelemetry
+        };
+
+        var baggageHeader = data.Baggage.AsBaggageHeader();
+        var dynamicSamplingContext = baggageHeader.CreateDynamicSamplingContext();
+        var transaction = (TransactionTracer)_hub.StartTransaction(
+            transactionContext, new Dictionary<string, object?>(), dynamicSamplingContext
+        );
+        transaction.StartTimestamp = data.StartTimeUtc;
+        _hub.ConfigureScope(scope => scope.Transaction = transaction);
+        transaction.SetFused(data);
+        _map[data.SpanId] = transaction;
+    }
+
+
     /// <inheritdoc />
     public override void OnEnd(Activity data)
     {

src/Sentry/IBaseTracer.cs

@@ -0,0 +1,6 @@
+namespace Sentry;
+
+internal interface IBaseTracer
+{
+    internal bool IsOtelInstrumenter { get; }
+}

src/Sentry/Internal/Hub.cs

@@ -119,16 +119,6 @@ internal ITransactionTracer StartTransaction(
         IReadOnlyDictionary<string, object?> customSamplingContext,
         DynamicSamplingContext? dynamicSamplingContext)
     {
-        var instrumenter = (context as SpanContext)?.Instrumenter;
-        if (instrumenter != _options.Instrumenter)
-        {
-            _options.LogWarning(
-                $"Attempted to start a transaction via {instrumenter} instrumentation when the SDK is" +
-                $" configured for {_options.Instrumenter} instrumentation.  The transaction will not be created.");
-
-            return NoOpTransaction.Instance;
-        }
-
         var transaction = new TransactionTracer(this, context);
 
         // If the hub is disabled, we will always sample out.  In other words, starting a transaction

src/Sentry/SentryTransaction.cs

@@ -288,7 +288,7 @@ internal static SentrySpan[] FromTracerSpans(ITransactionTracer tracer)
         var nonSentrySpans = tracer.Spans
             .Where(s => s is not SpanTracer { IsSentryRequest: true });
 
-        if (tracer is not TransactionTracer { IsOtelInstrumenter: true })
+        if (tracer is not IBaseTracer { IsOtelInstrumenter: true })
         {
             return nonSentrySpans.Select(s => new SentrySpan(s)).ToArray();
         }

src/Sentry/SpanTracer.cs

@@ -6,11 +6,15 @@ namespace Sentry;
 /// <summary>
 /// Transaction span tracer.
 /// </summary>
-public class SpanTracer : ISpan
+public class SpanTracer : IBaseTracer, ISpan
 {
     private readonly IHub _hub;
     private readonly SentryStopwatch _stopwatch = SentryStopwatch.StartNew();
 
+    private readonly Instrumenter _instrumenter = Instrumenter.Sentry;
+
+    bool IBaseTracer.IsOtelInstrumenter => _instrumenter == Instrumenter.OpenTelemetry;
+
     internal TransactionTracer Transaction { get; }
 
     private readonly Lazy<MetricsSummaryAggregator> _metricsSummary = new();
@@ -113,9 +117,11 @@ internal SpanTracer(
         SpanId spanId,
         SpanId? parentSpanId,
         SentryId traceId,
-        string operation)
+        string operation,
+        Instrumenter instrumenter = Instrumenter.Sentry)
     {
         _hub = hub;
+        _instrumenter = instrumenter;
         Transaction = transaction;
         SpanId = spanId;
         ParentSpanId = parentSpanId;

src/Sentry/TransactionTracer.cs

@@ -7,16 +7,17 @@ namespace Sentry;
 /// <summary>
 /// Transaction tracer.
 /// </summary>
-public class TransactionTracer : ITransactionTracer
+public class TransactionTracer : IBaseTracer, ITransactionTracer
 {
     private readonly IHub _hub;
     private readonly SentryOptions? _options;
     private readonly Timer? _idleTimer;
     private long _cancelIdleTimeout;
     private readonly SentryStopwatch _stopwatch = SentryStopwatch.StartNew();
+
     private readonly Instrumenter _instrumenter = Instrumenter.Sentry;
 
-    internal bool IsOtelInstrumenter => _instrumenter == Instrumenter.OpenTelemetry;
+    bool IBaseTracer.IsOtelInstrumenter => _instrumenter == Instrumenter.OpenTelemetry;
 
     /// <inheritdoc />
     public SpanId SpanId
@@ -281,7 +282,7 @@ internal TransactionTracer(IHub hub, ITransactionContext context, TimeSpan? idle
     internal ISpan StartChild(SpanId? spanId, SpanId parentSpanId, string operation,
         Instrumenter instrumenter = Instrumenter.Sentry)
     {
-        var span = new SpanTracer(_hub, this, parentSpanId, TraceId, operation);
+        var span = new SpanTracer(_hub, this, SpanId.Create(), parentSpanId, TraceId, operation, instrumenter: instrumenter);
         if (spanId is { } id)
         {
             span.SpanId = id;

test/Sentry.OpenTelemetry.Tests/SentrySpanProcessorTests.cs

@@ -164,7 +164,6 @@ public void OnStart_WithParentSpanId_StartsChildSpan()
         {
             span.Should().BeOfType<SpanTracer>();
             span.SpanId.Should().Be(data.SpanId.AsSentrySpanId());
-            span.ParentSpanId.Should().Be(data.ParentSpanId.AsSentrySpanId());
             if (span is not SpanTracer spanTracer)
             {
                 Assert.Fail("Span is not a span tracer");
@@ -183,12 +182,87 @@ public void OnStart_WithParentSpanId_StartsChildSpan()
         }
     }
 
+    [Fact]
+    public void OnStart_WithSentryParentSpanId_StartsChildSpan()
+    {
+        // Arrange
+        _fixture.Options.Instrumenter = Instrumenter.OpenTelemetry;
+        var sut = _fixture.GetSut();
+
+        var parent = _fixture.Hub.StartTransaction("Program", "Main");
+        _fixture.Hub.ConfigureScope(scope => scope.Transaction = parent);
+
+        using var data = Tracer.StartActivity("TestActivity");
+
+        // Act
+        sut.OnStart(data!);
+
+        // Assert
+        ((IBaseTracer)parent).IsOtelInstrumenter.Should().BeFalse();
+        Assert.True(sut._map.TryGetValue(data.SpanId, out var span));
+        using (new AssertionScope())
+        {
+            span.Should().BeOfType<SpanTracer>();
+            span.SpanId.Should().Be(data.SpanId.AsSentrySpanId());
+            if (span is not SpanTracer spanTracer)
+            {
+                Assert.Fail("Span is not a span tracer");
+                return;
+            }
+            using (new AssertionScope())
+            {
+                ((IBaseTracer)spanTracer).IsOtelInstrumenter.Should().BeTrue();
+                spanTracer.SpanId.Should().Be(data.SpanId.AsSentrySpanId());
+                spanTracer.ParentSpanId.Should().Be(parent.SpanId);
+                spanTracer.TraceId.Should().Be(parent.TraceId);
+                spanTracer.Operation.Should().Be(data.OperationName);
+                spanTracer.Description.Should().Be(data.DisplayName);
+                spanTracer.Status.Should().BeNull();
+                spanTracer.StartTimestamp.Should().Be(data.StartTimeUtc);
+            }
+        }
+    }
+
+
+    [Fact]
+    public void StartSpan_UsingSentryTracing_StartsChildSpan()
+    {
+        // Arrange
+        _fixture.Options.Instrumenter = Instrumenter.OpenTelemetry;
+        var sut = _fixture.GetSut();
+
+        using var parent = Tracer.StartActivity("Parent");
+        sut.OnStart(parent);
+
+        // Act
+        var span = _fixture.Hub.StartSpan("foo", "bar");
+
+        // Assert
+        Assert.True(sut._map.TryGetValue(parent.SpanId, out var transaction));
+        using (new AssertionScope())
+        {
+            if (span is not SpanTracer spanTracer)
+            {
+                Assert.Fail("Span is not a span tracer");
+                return;
+            }
+            spanTracer.ParentSpanId.Should().Be(transaction.SpanId);
+            spanTracer.TraceId.Should().Be(transaction.TraceId);
+            spanTracer.Operation.Should().Be("foo");
+            spanTracer.Description.Should().Be("bar");
+            spanTracer.Status.Should().BeNull();
+        }
+    }
+
     [Fact]
     public void OnStart_WithoutParentSpanId_StartsNewTransaction()
     {
         // Arrange
         _fixture.Options.Instrumenter = Instrumenter.OpenTelemetry;
         _fixture.ScopeManager = Substitute.For<IInternalScopeManager>();
+        var scope = new Scope();
+        var clientScope = new KeyValuePair<Scope, ISentryClient>(scope, _fixture.Client);
+        _fixture.ScopeManager.GetCurrent().Returns(clientScope);
         var sut = _fixture.GetSut();
 
         var data = Tracer.StartActivity("test op");
@@ -268,6 +342,9 @@ public void OnEnd_Transaction_RestoresSavedScope()
         // Arrange
         _fixture.Options.Instrumenter = Instrumenter.OpenTelemetry;
         _fixture.ScopeManager = Substitute.For<IInternalScopeManager>();
+        var dummyScope = new Scope();
+        var clientScope = new KeyValuePair<Scope, ISentryClient>(dummyScope, _fixture.Client);
+        _fixture.ScopeManager.GetCurrent().Returns(clientScope);
         var sut = _fixture.GetSut();
 
         var scope = new Scope();
@@ -288,6 +365,9 @@ public void OnEnd_Span_RestoresSavedScope()
         // Arrange
         _fixture.Options.Instrumenter = Instrumenter.OpenTelemetry;
         _fixture.ScopeManager = Substitute.For<IInternalScopeManager>();
+        var dummyScope = new Scope();
+        var clientScope = new KeyValuePair<Scope, ISentryClient>(dummyScope, _fixture.Client);
+        _fixture.ScopeManager.GetCurrent().Returns(clientScope);
         var sut = _fixture.GetSut();
 
         var scope = new Scope();

test/Sentry.Tests/HubTests.cs

@@ -637,7 +637,7 @@ public void StartTransaction_SameInstrumenter_SampledIn()
     }
 
     [Fact]
-    public void StartTransaction_DifferentInstrumenter_NoOp()
+    public void StartTransaction_DifferentInstrumenter_SampledIn()
     {
         // Arrange
         _fixture.Options.EnableTracing = true;
@@ -653,7 +653,7 @@ public void StartTransaction_DifferentInstrumenter_NoOp()
         var transaction = hub.StartTransaction(transactionContext);
 
         // Assert
-        transaction.Should().Be(NoOpTransaction.Instance);
+        transaction.IsSampled.Should().BeTrue();
     }
 
     [Fact]