Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: Scope not being applied to OpenTelemetry spans in ASP.NET Core #2690

Merged
merged 15 commits into from
Oct 9, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,9 @@

- Release of Azure Functions (Isolated Worker/Out-of-Process) support ([#2686](https://github.com/getsentry/sentry-dotnet/pull/2686))

### Fixes
- Scope is now correctly applied to Transactions when using OpenTelemetry on ASP.NET Core ([#2690](https://github.com/getsentry/sentry-dotnet/pull/2690))

### Dependencies

- Bump CLI from v2.20.7 to v2.21.1 ([#2645](https://github.com/getsentry/sentry-dotnet/pull/2645), [#2647](https://github.com/getsentry/sentry-dotnet/pull/2647))
Expand Down
7 changes: 7 additions & 0 deletions src/Sentry.AspNetCore/SentryMiddleware.cs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
using Microsoft.Extensions.Options;
using Sentry.AspNetCore.Extensions;
using Sentry.Extensibility;
using Sentry.Internal;
using Sentry.Reflection;

namespace Sentry.AspNetCore;
Expand Down Expand Up @@ -132,6 +133,12 @@ public async Task InvokeAsync(HttpContext context, RequestDelegate next)
{
var originalMethod = context.Request.Method;
await next(context).ConfigureAwait(false);
if (_options.Instrumenter == Instrumenter.OpenTelemetry && Activity.Current is {} activity)
{
// The middleware pipeline finishes up before the Otel Activity.OnEnd callback is invoked so we need
// so save a copy of the scope that can be restored by our SentrySpanProcessor
hub.ConfigureScope(scope => activity.SetFused(scope));
}

// When an exception was handled by other component (i.e: UseExceptionHandler feature).
var exceptionFeature = context.Features.Get<IExceptionHandlerFeature?>();
Expand Down
7 changes: 7 additions & 0 deletions src/Sentry.OpenTelemetry/SentrySpanProcessor.cs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
using OpenTelemetry;
using Sentry.Extensibility;
using Sentry.Internal;
using Sentry.Internal.Extensions;
using Sentry.Internal.OpenTelemetry;

Expand Down Expand Up @@ -156,6 +157,12 @@ public override void OnEnd(Activity data)

// Transactions set otel attributes (and resource attributes) as context.
transaction.Contexts["otel"] = GetOtelContext(attributes);
// Events are received/processed in a different AsyncLocal context. Restoring the scope that started it.
var activityScope = data.GetFused<Scope>();
if (activityScope is { } savedScope && _hub is Hub hub)
{
hub.RestoreScope(savedScope);
}
}
else
{
Expand Down
11 changes: 6 additions & 5 deletions src/Sentry/Internal/Hub.cs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
using Sentry.Extensibility;
using Sentry.Infrastructure;
using Sentry.Integrations;
using Sentry.Internal.ScopeStack;

namespace Sentry.Internal;

Expand Down Expand Up @@ -97,6 +98,8 @@ public async Task ConfigureScopeAsync(Func<Scope, Task> configureScope)

public IDisposable PushScope<TState>(TState state) => ScopeManager.PushScope(state);

public void RestoreScope(Scope savedScope) => ScopeManager.RestoreScope(savedScope);

[Obsolete]
public void WithScope(Action<Scope> scopeCallback) => ScopeManager.WithScope(scopeCallback);

Expand Down Expand Up @@ -486,8 +489,7 @@ public void CaptureTransaction(Transaction transaction, Hint? hint)
try
{
// Apply scope data
var currentScopeAndClient = ScopeManager.GetCurrent();
var scope = currentScopeAndClient.Key;
var (scope, client) = ScopeManager.GetCurrent();
scope.Evaluate();
scope.Apply(transaction);

Expand All @@ -513,7 +515,6 @@ public void CaptureTransaction(Transaction transaction, Hint? hint)
}
}

var client = currentScopeAndClient.Value;
client.CaptureTransaction(processedTransaction, hint);
}
catch (Exception e)
Expand Down Expand Up @@ -543,8 +544,8 @@ public async Task FlushAsync(TimeSpan timeout)
{
try
{
var currentScope = ScopeManager.GetCurrent();
await currentScope.Value.FlushAsync(timeout).ConfigureAwait(false);
var (_, client) = ScopeManager.GetCurrent();
await client.FlushAsync(timeout).ConfigureAwait(false);
}
catch (Exception e)
{
Expand Down
1 change: 1 addition & 0 deletions src/Sentry/Internal/IHubEx.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ namespace Sentry.Internal;
internal interface IHubEx : IHub
{
SentryId CaptureEventInternal(SentryEvent evt, Hint? hint, Scope? scope = null);

T? WithScope<T>(Func<Scope, T?> scopeCallback);
Task WithScopeAsync(Func<Scope, Task> scopeCallback);
Task<T?> WithScopeAsync<T>(Func<Scope, Task<T?>> scopeCallback);
Expand Down
2 changes: 2 additions & 0 deletions src/Sentry/Internal/IInternalScopeManager.cs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ namespace Sentry.Internal;
internal interface IInternalScopeManager : ISentryScopeManager, IDisposable
{
KeyValuePair<Scope, ISentryClient> GetCurrent();
void RestoreScope(Scope savedScope);

IScopeStackContainer ScopeStackContainer { get; }

// TODO: Move The following to ISentryScopeManager in a future major version.
Expand Down
49 changes: 32 additions & 17 deletions src/Sentry/Internal/SentryScopeManager.cs
Original file line number Diff line number Diff line change
Expand Up @@ -30,22 +30,18 @@ public SentryScopeManager(SentryOptions options, ISentryClient rootClient)
NewStack = () => new[] { new KeyValuePair<Scope, ISentryClient>(new Scope(options), rootClient) };
}

public KeyValuePair<Scope, ISentryClient> GetCurrent()
{
var current = ScopeAndClientStack;
return current[^1];
}
public KeyValuePair<Scope, ISentryClient> GetCurrent() => ScopeAndClientStack[^1];

public void ConfigureScope(Action<Scope>? configureScope)
{
var scope = GetCurrent();
configureScope?.Invoke(scope.Key);
var (scope, _) = GetCurrent();
configureScope?.Invoke(scope);
}

public Task ConfigureScopeAsync(Func<Scope, Task>? configureScope)
{
var scope = GetCurrent();
return configureScope?.Invoke(scope.Key) ?? Task.CompletedTask;
var (scope, _) = GetCurrent();
return configureScope?.Invoke(scope) ?? Task.CompletedTask;
}

public IDisposable PushScope() => PushScope<object>(null);
Expand Down Expand Up @@ -92,39 +88,58 @@ public IDisposable PushScope<TState>(TState? state)
return scopeSnapshot;
}

public void RestoreScope(Scope savedScope)
jamescrosswell marked this conversation as resolved.
Show resolved Hide resolved
{
if (IsGlobalMode)
{
_options.LogWarning("RestoreScope called in global mode, returning.");
return;
}

var currentScopeAndClientStack = ScopeAndClientStack;
jamescrosswell marked this conversation as resolved.
Show resolved Hide resolved
var (previousScope, client) = currentScopeAndClientStack[^1];

_options.LogDebug("Scope restored");
var newScopeAndClientStack = new KeyValuePair<Scope, ISentryClient>[currentScopeAndClientStack.Length + 1];
Array.Copy(currentScopeAndClientStack, newScopeAndClientStack, currentScopeAndClientStack.Length);
newScopeAndClientStack[^1] = new KeyValuePair<Scope, ISentryClient>(savedScope, client);

ScopeAndClientStack = newScopeAndClientStack;
}

public void WithScope(Action<Scope> scopeCallback)
{
using (PushScope())
{
var scope = GetCurrent();
scopeCallback.Invoke(scope.Key);
var (scope, _) = GetCurrent();
scopeCallback.Invoke(scope);
}
}

public T? WithScope<T>(Func<Scope, T?> scopeCallback)
{
using (PushScope())
{
var scope = GetCurrent();
return scopeCallback.Invoke(scope.Key);
var (scope, _) = GetCurrent();
return scopeCallback.Invoke(scope);
}
}

public async Task WithScopeAsync(Func<Scope, Task> scopeCallback)
{
using (PushScope())
{
var scope = GetCurrent();
await scopeCallback.Invoke(scope.Key).ConfigureAwait(false);
var (scope, _) = GetCurrent();
await scopeCallback.Invoke(scope).ConfigureAwait(false);
}
}

public async Task<T?> WithScopeAsync<T>(Func<Scope, Task<T?>> scopeCallback)
{
using (PushScope())
{
var scope = GetCurrent();
return await scopeCallback.Invoke(scope.Key).ConfigureAwait(false);
var (scope, _) = GetCurrent();
return await scopeCallback.Invoke(scope).ConfigureAwait(false);
}
}

Expand Down
1 change: 1 addition & 0 deletions src/Sentry/TransactionTracer.cs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
using Sentry.Extensibility;
using Sentry.Internal;
using Sentry.Internal.ScopeStack;
using Sentry.Protocol;

namespace Sentry;
Expand Down
24 changes: 24 additions & 0 deletions test/Sentry.AspNetCore.Tests/SentryMiddlewareTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -659,6 +659,30 @@ public async Task InvokeAsync_SetsEventIdOnEvent()
_ = _fixture.Hub.Received().CaptureEvent(Arg.Is<SentryEvent>(e => e.EventId.Equals(eventId)));
}

[Fact]
public async Task InvokeAsync_InstrumenterOpenTelemetry_SavesScope()
{
// Arrange
_fixture.Options.Instrumenter = Instrumenter.OpenTelemetry;
var scope = new Scope();
_fixture.Hub.ConfigureScope(Arg.Do<Action<Scope>>(action => action.Invoke(scope)));
var sut = _fixture.GetSut();
var activity = new Activity("test").Start();

try
{
// Act
await sut.InvokeAsync(_fixture.HttpContext, _fixture.RequestDelegate);

// Assert
activity.GetFused<Scope>().Should().Be(scope);
}
finally
{
activity.Stop();
}
}

[Fact]
public async Task InvokeAsync_RequestContainsSentryHeaders_ContinuesTrace()
{
Expand Down
22 changes: 22 additions & 0 deletions test/Sentry.OpenTelemetry.Tests/SentrySpanProcessorTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,28 @@ public void OnEnd_FinishesSpan()
}
}

[Fact]
public void OnEnd_RestoresSavedScope()
{
// Arrange
_fixture.Options.Instrumenter = Instrumenter.OpenTelemetry;
_fixture.ScopeManager = Substitute.For<IInternalScopeManager>();
var sut = _fixture.GetSut();

var scope = new Scope();
var data = Tracer.StartActivity("transaction")!;
data.SetFused(scope);
sut.OnStart(data);

sut._map.TryGetValue(data.SpanId, out var span);

// Act
sut.OnEnd(data);

// Assert
_fixture.ScopeManager.Received(1).RestoreScope(scope);
}

[Fact]
public void OnEnd_SpansEnriched()
{
Expand Down
Loading