From 28ec35d278b2fa15d356af41079ab81a956fe364 Mon Sep 17 00:00:00 2001 From: Alexey Golub Date: Thu, 15 Jul 2021 16:35:05 -0700 Subject: [PATCH] Introduce scope stack abstraction (#1124) --- CHANGELOG.md | 4 + .../HttpContextScopeStackContainer.cs | 17 ++ .../SentryAspNetOptionsExtensions.cs | 3 + src/Sentry/Internal/Hub.cs | 7 +- .../AsyncLocalScopeStackContainer.cs | 16 ++ .../ScopeStack/GlobalScopeStackContainer.cs | 9 + .../ScopeStack/IScopeStackContainer.cs | 9 + src/Sentry/Internal/SentryScopeManager.cs | 26 ++- src/Sentry/SentryOptions.cs | 12 ++ .../AsyncLocalScopeStackContainerTests.cs | 161 ++++++++++++++++++ .../GlobalScopeStackContainerTests.cs | 42 +++++ .../Internals/SentryScopeManagerTests.cs | 27 ++- test/Sentry.Tests/SentryOptionsTests.cs | 2 + test/Sentry.Tests/SentrySdkTests.cs | 35 ++++ 14 files changed, 361 insertions(+), 9 deletions(-) create mode 100644 src/Sentry.AspNet/Internal/HttpContextScopeStackContainer.cs create mode 100644 src/Sentry/Internal/ScopeStack/AsyncLocalScopeStackContainer.cs create mode 100644 src/Sentry/Internal/ScopeStack/GlobalScopeStackContainer.cs create mode 100644 src/Sentry/Internal/ScopeStack/IScopeStackContainer.cs create mode 100644 test/Sentry.Tests/Internals/ScopeStack/AsyncLocalScopeStackContainerTests.cs create mode 100644 test/Sentry.Tests/Internals/ScopeStack/GlobalScopeStackContainerTests.cs diff --git a/CHANGELOG.md b/CHANGELOG.md index 29081eea08..c1355f4fc9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,10 @@ - Persisted Sessions logging ([#1125](https://github.com/getsentry/sentry-dotnet/pull/1125)) - Don't log an error when attempting to recover a persisted session but none exists ([#1123](https://github.com/getsentry/sentry-dotnet/pull/1123)) +### Features + +- Introduce scope stack abstraction to support global scope on desktop and mobile applications and `HttpContext`-backed scoped on legacy ASP.NET ([#1124](https://github.com/getsentry/sentry-dotnet/pull/1124)) + ## 3.8.0 ### Fixes diff --git a/src/Sentry.AspNet/Internal/HttpContextScopeStackContainer.cs b/src/Sentry.AspNet/Internal/HttpContextScopeStackContainer.cs new file mode 100644 index 0000000000..a936ed4154 --- /dev/null +++ b/src/Sentry.AspNet/Internal/HttpContextScopeStackContainer.cs @@ -0,0 +1,17 @@ +using System.Collections.Generic; +using System.Web; +using Sentry.Internal.ScopeStack; + +namespace Sentry.AspNet.Internal +{ + internal class HttpContextScopeStackContainer : IScopeStackContainer + { + private const string FieldName = "__SentryScopeStack"; + + public KeyValuePair[]? Stack + { + get => HttpContext.Current.Items[FieldName] as KeyValuePair[]; + set => HttpContext.Current.Items[FieldName] = value; + } + } +} diff --git a/src/Sentry.AspNet/SentryAspNetOptionsExtensions.cs b/src/Sentry.AspNet/SentryAspNetOptionsExtensions.cs index 6094dd9b1a..14e04ceb5f 100644 --- a/src/Sentry.AspNet/SentryAspNetOptionsExtensions.cs +++ b/src/Sentry.AspNet/SentryAspNetOptionsExtensions.cs @@ -23,6 +23,9 @@ public static void AddAspNet(this SentryOptions options, RequestSize maxRequestB var eventProcessor = new SystemWebRequestEventProcessor(payloadExtractor, options); + // Ignore options.IsGlobalModeEnable, we always want to use HttpContext as backing store here + options.ScopeStackContainer ??= new HttpContextScopeStackContainer(); + options.DiagnosticLogger ??= new TraceDiagnosticLogger(options.DiagnosticLevel); options.Release ??= SystemWebVersionLocator.Resolve(options, HttpContext.Current); options.AddEventProcessor(eventProcessor); diff --git a/src/Sentry/Internal/Hub.cs b/src/Sentry/Internal/Hub.cs index f4cefb0829..279fc34c71 100644 --- a/src/Sentry/Internal/Hub.cs +++ b/src/Sentry/Internal/Hub.cs @@ -7,6 +7,7 @@ using Sentry.Extensibility; using Sentry.Infrastructure; using Sentry.Integrations; +using Sentry.Internal.ScopeStack; namespace Sentry.Internal { @@ -48,7 +49,11 @@ internal Hub(ISentryClient client, ISystemClock clock, ISessionManager sessionMa options.DiagnosticLogger?.LogDebug("Initializing Hub for Dsn: '{0}'.", options.Dsn); - ScopeManager = new SentryScopeManager(options, _ownedClient); + ScopeManager = new SentryScopeManager( + options.ScopeStackContainer ?? new AsyncLocalScopeStackContainer(), + options, + _ownedClient + ); _integrations = options.Integrations; diff --git a/src/Sentry/Internal/ScopeStack/AsyncLocalScopeStackContainer.cs b/src/Sentry/Internal/ScopeStack/AsyncLocalScopeStackContainer.cs new file mode 100644 index 0000000000..a5364f500d --- /dev/null +++ b/src/Sentry/Internal/ScopeStack/AsyncLocalScopeStackContainer.cs @@ -0,0 +1,16 @@ +using System.Collections.Generic; +using System.Threading; + +namespace Sentry.Internal.ScopeStack +{ + internal class AsyncLocalScopeStackContainer : IScopeStackContainer + { + private readonly AsyncLocal[]?> _asyncLocalScope = new(); + + public KeyValuePair[]? Stack + { + get => _asyncLocalScope.Value; + set => _asyncLocalScope.Value = value; + } + } +} diff --git a/src/Sentry/Internal/ScopeStack/GlobalScopeStackContainer.cs b/src/Sentry/Internal/ScopeStack/GlobalScopeStackContainer.cs new file mode 100644 index 0000000000..b279535457 --- /dev/null +++ b/src/Sentry/Internal/ScopeStack/GlobalScopeStackContainer.cs @@ -0,0 +1,9 @@ +using System.Collections.Generic; + +namespace Sentry.Internal.ScopeStack +{ + internal class GlobalScopeStackContainer : IScopeStackContainer + { + public KeyValuePair[]? Stack { get; set; } + } +} diff --git a/src/Sentry/Internal/ScopeStack/IScopeStackContainer.cs b/src/Sentry/Internal/ScopeStack/IScopeStackContainer.cs new file mode 100644 index 0000000000..5fc028e695 --- /dev/null +++ b/src/Sentry/Internal/ScopeStack/IScopeStackContainer.cs @@ -0,0 +1,9 @@ +using System.Collections.Generic; + +namespace Sentry.Internal.ScopeStack +{ + internal interface IScopeStackContainer + { + KeyValuePair[]? Stack { get; set; } + } +} diff --git a/src/Sentry/Internal/SentryScopeManager.cs b/src/Sentry/Internal/SentryScopeManager.cs index 8f08a7ef33..4973089260 100644 --- a/src/Sentry/Internal/SentryScopeManager.cs +++ b/src/Sentry/Internal/SentryScopeManager.cs @@ -1,28 +1,34 @@ using System; using System.Collections.Generic; -using System.Threading; using System.Threading.Tasks; using Sentry.Extensibility; +using Sentry.Internal.ScopeStack; namespace Sentry.Internal { internal sealed class SentryScopeManager : IInternalScopeManager, IDisposable { + // Internal for testing + internal IScopeStackContainer ScopeStackContainer { get; } + private readonly SentryOptions _options; - private readonly AsyncLocal[]?> _asyncLocalScope = new(); internal KeyValuePair[] ScopeAndClientStack { - get => _asyncLocalScope.Value ?? (_asyncLocalScope.Value = NewStack()); - set => _asyncLocalScope.Value = value; + get => ScopeStackContainer.Stack ??= NewStack(); + set => ScopeStackContainer.Stack = value; } private Func[]> NewStack { get; } + private bool IsGlobalMode => ScopeStackContainer is GlobalScopeStackContainer; + public SentryScopeManager( + IScopeStackContainer scopeStackContainer, SentryOptions options, ISentryClient rootClient) { + ScopeStackContainer = scopeStackContainer; _options = options; NewStack = () => new [] { new KeyValuePair(new Scope(options), rootClient) }; } @@ -47,10 +53,16 @@ public Task ConfigureScopeAsync(Func? configureScope) return configureScope?.Invoke(scope.Key) ?? Task.CompletedTask; } - public IDisposable PushScope() => PushScope(null!); // NRTs don't work well with generics + public IDisposable PushScope() => PushScope(null); - public IDisposable PushScope(TState state) + public IDisposable PushScope(TState? state) { + if (IsGlobalMode) + { + _options.DiagnosticLogger?.LogWarning("Push scope called in global mode, returning."); + return DisabledHub.Instance; + } + var currentScopeAndClientStack = ScopeAndClientStack; var scope = currentScopeAndClientStack[currentScopeAndClientStack.Length - 1]; @@ -145,7 +157,7 @@ public void Dispose() public void Dispose() { _options.DiagnosticLogger?.LogDebug($"Disposing {nameof(SentryScopeManager)}."); - _asyncLocalScope.Value = null; + ScopeStackContainer.Stack = null; } } } diff --git a/src/Sentry/SentryOptions.cs b/src/Sentry/SentryOptions.cs index 781d2f174b..c70060324f 100644 --- a/src/Sentry/SentryOptions.cs +++ b/src/Sentry/SentryOptions.cs @@ -8,6 +8,7 @@ using Sentry.Http; using Sentry.Integrations; using Sentry.Internal; +using Sentry.Internal.ScopeStack; using static Sentry.Internal.Constants; using static Sentry.Constants; using Runtime = Sentry.PlatformAbstractions.Runtime; @@ -21,6 +22,17 @@ public class SentryOptions { private Dictionary? _defaultTags; + internal IScopeStackContainer? ScopeStackContainer { get; set; } + + /// + /// Specifies whether to use global scope management mode. + /// + public bool IsGlobalModeEnabled + { + get => ScopeStackContainer is GlobalScopeStackContainer; + set => ScopeStackContainer = value ? new GlobalScopeStackContainer() : new AsyncLocalScopeStackContainer(); + } + // Override for tests internal ITransport? Transport { get; set; } diff --git a/test/Sentry.Tests/Internals/ScopeStack/AsyncLocalScopeStackContainerTests.cs b/test/Sentry.Tests/Internals/ScopeStack/AsyncLocalScopeStackContainerTests.cs new file mode 100644 index 0000000000..26999f284f --- /dev/null +++ b/test/Sentry.Tests/Internals/ScopeStack/AsyncLocalScopeStackContainerTests.cs @@ -0,0 +1,161 @@ +using System.Collections.Generic; +using System.Threading.Tasks; +using FluentAssertions; +using NSubstitute; +using Sentry.Internal.ScopeStack; +using Xunit; + +namespace Sentry.Tests.Internals.ScopeStack +{ + public class AsyncLocalScopeStackContainerTests + { + [Fact] + public async Task Scopes_are_not_shared_between_parallel_async_executions() + { + // Arrange + var container = new AsyncLocalScopeStackContainer(); + + var scope1 = new KeyValuePair( + Substitute.For(), + Substitute.For() + ); + + var scope2 = new KeyValuePair( + Substitute.For(), + Substitute.For() + ); + + // Act & assert + var task1 = Task.Run(async () => + { + container.Stack.Should().BeNull(); + + await Task.Yield(); + + container.Stack.Should().BeNull(); + }); + + var task2 = Task.Run(async () => + { + container.Stack.Should().BeNull(); + + container.Stack = new[] {scope1}; + await Task.Yield(); + + container.Stack.Should().BeEquivalentTo(new[] {scope1}); + }); + + var task3 = Task.Run(async () => + { + container.Stack.Should().BeNull(); + + container.Stack = new[] {scope2}; + await Task.Yield(); + + container.Stack.Should().BeEquivalentTo(new[] {scope2}); + }); + + await Task.WhenAll(task1, task2, task3); + + container.Stack.Should().BeNull(); + } + + [Fact] + public async Task Scopes_are_not_shared_between_sequential_async_executions() + { + // Arrange + var container = new AsyncLocalScopeStackContainer(); + + var scope1 = new KeyValuePair( + Substitute.For(), + Substitute.For() + ); + + var scope2 = new KeyValuePair( + Substitute.For(), + Substitute.For() + ); + + // Act & assert + await Task.Run(async () => + { + container.Stack.Should().BeNull(); + + await Task.Yield(); + + container.Stack.Should().BeNull(); + }); + + await Task.Run(async () => + { + container.Stack.Should().BeNull(); + + container.Stack = new[] {scope1}; + await Task.Yield(); + + container.Stack.Should().BeEquivalentTo(new[] {scope1}); + }); + + await Task.Run(async () => + { + container.Stack.Should().BeNull(); + + container.Stack = new[] {scope2}; + await Task.Yield(); + + container.Stack.Should().BeEquivalentTo(new[] {scope2}); + }); + + container.Stack.Should().BeNull(); + } + + [Fact] + public async Task Scopes_are_shared_between_nested_async_executions() + { + // Arrange + var container = new AsyncLocalScopeStackContainer(); + + var scope1 = new KeyValuePair( + Substitute.For(), + Substitute.For() + ); + + var scope2 = new KeyValuePair( + Substitute.For(), + Substitute.For() + ); + + // Act & assert + await Task.Run(async () => + { + container.Stack.Should().BeNull(); + + await Task.Yield(); + + container.Stack.Should().BeNull(); + + await Task.Run(async () => + { + container.Stack.Should().BeNull(); + + container.Stack = new[] {scope1}; + await Task.Yield(); + + container.Stack.Should().BeEquivalentTo(new[] {scope1}); + + await Task.Run(async () => + { + container.Stack.Should().BeEquivalentTo(new[] {scope1}); + + await Task.Yield(); + container.Stack = new[] {scope2}; + + container.Stack.Should().BeEquivalentTo(new[] {scope2}); + }).ConfigureAwait(false); + }).ConfigureAwait(false); + }); + + container.Stack.Should().BeNull(); + } + } +} diff --git a/test/Sentry.Tests/Internals/ScopeStack/GlobalScopeStackContainerTests.cs b/test/Sentry.Tests/Internals/ScopeStack/GlobalScopeStackContainerTests.cs new file mode 100644 index 0000000000..8df826f10d --- /dev/null +++ b/test/Sentry.Tests/Internals/ScopeStack/GlobalScopeStackContainerTests.cs @@ -0,0 +1,42 @@ +using System.Collections.Generic; +using System.Threading.Tasks; +using FluentAssertions; +using NSubstitute; +using Sentry.Internal.ScopeStack; +using Xunit; + +namespace Sentry.Tests.Internals.ScopeStack +{ + public class GlobalScopeStackContainerTests + { + [Fact] + public async Task Scopes_are_shared_between_parallel_async_executions() + { + // Arrange + var container = new GlobalScopeStackContainer(); + + var scope1 = new KeyValuePair( + Substitute.For(), + Substitute.For() + ); + + var scope2 = new KeyValuePair( + Substitute.For(), + Substitute.For() + ); + + // Act & assert + await Task.Run(async () => + { + container.Stack.Should().BeNull(); + + container.Stack = new[] {scope1, scope2}; + await Task.Yield(); + + container.Stack.Should().BeEquivalentTo(new[] {scope1, scope2}); + }); + + container.Stack.Should().BeEquivalentTo(new[] {scope1, scope2}); + } + } +} diff --git a/test/Sentry.Tests/Internals/SentryScopeManagerTests.cs b/test/Sentry.Tests/Internals/SentryScopeManagerTests.cs index 7729aad3f8..4b700f16ba 100644 --- a/test/Sentry.Tests/Internals/SentryScopeManagerTests.cs +++ b/test/Sentry.Tests/Internals/SentryScopeManagerTests.cs @@ -2,9 +2,11 @@ using System.Linq; using System.Threading; using System.Threading.Tasks; +using FluentAssertions; using NSubstitute; using Sentry.Extensibility; using Sentry.Internal; +using Sentry.Internal.ScopeStack; using Xunit; namespace Sentry.Tests.Internals @@ -14,8 +16,14 @@ public class SentryScopeManagerTests private class Fixture { public SentryOptions SentryOptions { get; set; } = new(); + public ISentryClient Client { get; set; } = Substitute.For(); - public SentryScopeManager GetSut() => new(SentryOptions, Client); + + public SentryScopeManager GetSut() => new( + SentryOptions.ScopeStackContainer ?? new AsyncLocalScopeStackContainer(), + SentryOptions, + Client + ); } private readonly Fixture _fixture = new(); @@ -312,5 +320,22 @@ await Task.WhenAll(Enumerable.Range(1, 5) Assert.Equal(root, sut.GetCurrent()); } + + [Fact] + public void GlobalMode_PushScope_SameScope() + { + // Arrange + _fixture.SentryOptions.ScopeStackContainer = new GlobalScopeStackContainer(); + var sut = _fixture.GetSut(); + + // Act + var (scope1, client1) = sut.GetCurrent(); + using var _ = sut.PushScope(); + var (scope2, client2) = sut.GetCurrent(); + + // Assert + scope1.Should().BeSameAs(scope2); + client1.Should().BeSameAs(client2); + } } } diff --git a/test/Sentry.Tests/SentryOptionsTests.cs b/test/Sentry.Tests/SentryOptionsTests.cs index c49754ac18..8ce60c3ab6 100644 --- a/test/Sentry.Tests/SentryOptionsTests.cs +++ b/test/Sentry.Tests/SentryOptionsTests.cs @@ -17,12 +17,14 @@ public void DecompressionMethods_ByDefault_AllBitsSet() var sut = new SentryOptions(); Assert.Equal(~DecompressionMethods.None, sut.DecompressionMethods); } + [Fact] public void RequestBodyCompressionLevel_ByDefault_Optimal() { var sut = new SentryOptions(); Assert.Equal(CompressionLevel.Optimal, sut.RequestBodyCompressionLevel); } + #if NET461 [SkippableFact(typeof(IsTypeException))] public void StackTraceFactory_RunningOnMono_HasMonoStackTraceFactory() diff --git a/test/Sentry.Tests/SentrySdkTests.cs b/test/Sentry.Tests/SentrySdkTests.cs index ab17b33610..0d8fd7ec27 100644 --- a/test/Sentry.Tests/SentrySdkTests.cs +++ b/test/Sentry.Tests/SentrySdkTests.cs @@ -6,9 +6,12 @@ using FluentAssertions; using NSubstitute; using Sentry.Extensibility; +using Sentry.Internal; using Sentry.Internal.Http; +using Sentry.Internal.ScopeStack; using Sentry.Protocol.Envelopes; using Sentry.Testing; +using Sentry.Tests.Internals.ScopeStack; using Xunit; using Xunit.Abstractions; using static Sentry.Internal.Constants; @@ -493,5 +496,37 @@ public async Task InitHub_NoDsn_FlushAsyncDoesNotThrow() var sut = SentrySdk.InitHub(new SentryOptions()); await sut.FlushAsync(TimeSpan.FromDays(1)); } + + [Fact] + public void InitHub_GlobalModeOff_AsyncLocalContainer() + { + // Act + var sut = SentrySdk.InitHub(new SentryOptions + { + Dsn = ValidDsnWithoutSecret, + IsGlobalModeEnabled = false + }); + + var hub = (Hub)sut; + + // Assert + hub.ScopeManager.ScopeStackContainer.Should().BeOfType(); + } + + [Fact] + public void InitHub_GlobalModeOn_GlobalContainer() + { + // Act + var sut = SentrySdk.InitHub(new SentryOptions + { + Dsn = ValidDsnWithoutSecret, + IsGlobalModeEnabled = true + }); + + var hub = (Hub)sut; + + // Assert + hub.ScopeManager.ScopeStackContainer.Should().BeOfType(); + } } }