From bcf94161090b45c41fdb900bbfca898a305f573d Mon Sep 17 00:00:00 2001 From: James Crosswell Date: Tue, 4 Apr 2023 11:17:47 +1200 Subject: [PATCH 1/4] Add Scope.Clear and Scope.ClearBreadcrumbs methods See https://github.com/getsentry/sentry-dotnet/issues/2272 for full context. --- src/Sentry/Scope.cs | 38 ++++++++++++++++++++ test/Sentry.Tests/ScopeTests.cs | 63 +++++++++++++++++++++++++++++++++ 2 files changed, 101 insertions(+) diff --git a/src/Sentry/Scope.cs b/src/Sentry/Scope.cs index d44069fc65..c220e1d157 100644 --- a/src/Sentry/Scope.cs +++ b/src/Sentry/Scope.cs @@ -195,7 +195,11 @@ public User User /// public IReadOnlyList Fingerprint { get; set; } = Array.Empty(); +#if NETSTANDARD2_0 || NETFRAMEWORK + private ConcurrentQueue _breadcrumbs = new(); +#else private readonly ConcurrentQueue _breadcrumbs = new(); +#endif /// public IReadOnlyCollection Breadcrumbs => _breadcrumbs; @@ -305,6 +309,28 @@ public void UnsetTag(string key) /// public void AddAttachment(Attachment attachment) => _attachments.Add(attachment); + /// + /// Resets all the properties and collections within the scope to their default values. + /// + internal void Clear() + { + Level = default; + Request = new(); + Contexts.Clear(); + User = new(); + Platform = default; + Release = default; + Distribution = default; + Environment = default; + TransactionName = default; + Transaction = default; + Fingerprint = Array.Empty(); + ClearBreadcrumbs(); + _extra.Clear(); + _tags.Clear(); + ClearAttachments(); + } + /// /// Clear all Attachments. /// @@ -317,6 +343,18 @@ public void ClearAttachments() #endif } + /// + internal void ClearBreadcrumbs() + { +#if NETSTANDARD2_0 || NETFRAMEWORK + // No Clear method on ConcurrentQueue for these target frameworks + Interlocked.Exchange(ref _breadcrumbs, new()); +#else + _breadcrumbs.Clear(); +#endif + } + + /// /// Applies the data from this scope to another event-like object. /// diff --git a/test/Sentry.Tests/ScopeTests.cs b/test/Sentry.Tests/ScopeTests.cs index 3a76f33a9a..e116e36439 100644 --- a/test/Sentry.Tests/ScopeTests.cs +++ b/test/Sentry.Tests/ScopeTests.cs @@ -1,3 +1,4 @@ +using FluentAssertions.Execution; namespace Sentry.Tests; public class ScopeTests @@ -232,6 +233,51 @@ public void AddAttachment_AddAttachments() scope.Attachments.Should().Contain(attachment2, "Attachment2 was not found."); } + [Fact] + public void Clear_SetsPropertiesToDefaultValues() + { + // Arrange + _sut.Level = SentryLevel.Warning; + _sut.Request = new() { Data = "request to be cleared" }; + _sut.Contexts.Add("context to be cleared", "{}"); + _sut.User = new User() { Username = "username to be cleared" }; + _sut.Platform = "platform to be cleared"; + _sut.Release = "release to be cleared"; + _sut.Distribution = "distribution to be cleared"; + _sut.Environment = "environment to be cleared"; + _sut.TransactionName = "transaction name to be cleared"; + _sut.Transaction = Substitute.For(); + _sut.Fingerprint = new[] { "fingerprint to be cleared" }; + _sut.AddBreadcrumb(new(message: "breadcrumb to be cleared")); + _sut.SetExtra("extra", "extra to be cleared"); + _sut.SetTag("tag", "tag to be cleared"); + _sut.AddAttachment(new Attachment(default, default, default, "attachment to be cleared")); + + // Act + _sut.Clear(); + + // Assert + var newScope = new Scope(); + using (new AssertionScope()) + { + _sut.Level.Should().Be(newScope.Level); + _sut.Request.Should().BeEquivalentTo(newScope.Request); + _sut.Contexts.Should().BeEquivalentTo(newScope.Contexts); + _sut.User.Should().BeEquivalentTo(newScope.User); + _sut.Platform.Should().Be(newScope.Platform); + _sut.Release.Should().Be(newScope.Release); + _sut.Distribution.Should().Be(newScope.Distribution); + _sut.Environment.Should().Be(newScope.Environment); + _sut.TransactionName.Should().Be(newScope.TransactionName); + _sut.Transaction.Should().Be(newScope.Transaction); + _sut.Fingerprint.Should().BeEquivalentTo(newScope.Fingerprint); + _sut.Breadcrumbs.Should().BeEquivalentTo(newScope.Breadcrumbs); + _sut.Extra.Should().BeEquivalentTo(newScope.Extra); + _sut.Tags.Should().BeEquivalentTo(newScope.Tags); + _sut.Attachments.Should().BeEquivalentTo(newScope.Attachments); + } + } + [Fact] public void ClearAttachments_HasAttachments_EmptyList() { @@ -250,6 +296,23 @@ public void ClearAttachments_HasAttachments_EmptyList() scope.Attachments.Should().BeEmpty(); } + [Fact] + public void ClearBreadcrumbs_Breadcrumbs_EmptyList() + { + // Arrange + for (var i = 0; i < 5; i++) + { + _sut.AddBreadcrumb(new Breadcrumb()); + } + _sut.Breadcrumbs.Should().NotBeEmpty("Sanity check: Arrange failed to configure Breadcrumbs"); + + // Act + _sut.ClearBreadcrumbs(); + + // Assert + _sut.Breadcrumbs.Should().BeEmpty(); + } + [Theory] [InlineData(0, -2, 0)] [InlineData(0, -1, 0)] From 194315eb96c51f92c1e4c6cd787a5822d936bd68 Mon Sep 17 00:00:00 2001 From: James Crosswell Date: Wed, 5 Apr 2023 17:22:45 +1200 Subject: [PATCH 2/4] - updated Clear and ClearBreadcrumbs methods to be public - Provided summary documentation for the ClearBreadcrumbs method - Committed updated verified.txt files for API approval tests --- src/Sentry/Scope.cs | 8 +++++--- .../ApiApprovalTests.Run.Core3_1.verified.txt | 2 ++ .../ApiApprovalTests.Run.DotNet6_0.verified.txt | 2 ++ .../ApiApprovalTests.Run.DotNet7_0.verified.txt | 2 ++ .../Sentry.Tests/ApiApprovalTests.Run.Net4_8.verified.txt | 2 ++ 5 files changed, 13 insertions(+), 3 deletions(-) diff --git a/src/Sentry/Scope.cs b/src/Sentry/Scope.cs index c220e1d157..1d2dfffd6d 100644 --- a/src/Sentry/Scope.cs +++ b/src/Sentry/Scope.cs @@ -312,7 +312,7 @@ public void UnsetTag(string key) /// /// Resets all the properties and collections within the scope to their default values. /// - internal void Clear() + public void Clear() { Level = default; Request = new(); @@ -343,8 +343,10 @@ public void ClearAttachments() #endif } - /// - internal void ClearBreadcrumbs() + /// + /// Removes all Breadcrumbs from the scope. + /// + public void ClearBreadcrumbs() { #if NETSTANDARD2_0 || NETFRAMEWORK // No Clear method on ConcurrentQueue for these target frameworks diff --git a/test/Sentry.Tests/ApiApprovalTests.Run.Core3_1.verified.txt b/test/Sentry.Tests/ApiApprovalTests.Run.Core3_1.verified.txt index bf54b411e0..9891075c4d 100644 --- a/test/Sentry.Tests/ApiApprovalTests.Run.Core3_1.verified.txt +++ b/test/Sentry.Tests/ApiApprovalTests.Run.Core3_1.verified.txt @@ -395,7 +395,9 @@ namespace Sentry public void Apply(Sentry.IEventLike other) { } public void Apply(Sentry.Scope other) { } public void Apply(object state) { } + public void Clear() { } public void ClearAttachments() { } + public void ClearBreadcrumbs() { } public Sentry.Scope Clone() { } public Sentry.ISpan? GetSpan() { } public void SetExtra(string key, object? value) { } diff --git a/test/Sentry.Tests/ApiApprovalTests.Run.DotNet6_0.verified.txt b/test/Sentry.Tests/ApiApprovalTests.Run.DotNet6_0.verified.txt index bf54b411e0..9891075c4d 100644 --- a/test/Sentry.Tests/ApiApprovalTests.Run.DotNet6_0.verified.txt +++ b/test/Sentry.Tests/ApiApprovalTests.Run.DotNet6_0.verified.txt @@ -395,7 +395,9 @@ namespace Sentry public void Apply(Sentry.IEventLike other) { } public void Apply(Sentry.Scope other) { } public void Apply(object state) { } + public void Clear() { } public void ClearAttachments() { } + public void ClearBreadcrumbs() { } public Sentry.Scope Clone() { } public Sentry.ISpan? GetSpan() { } public void SetExtra(string key, object? value) { } diff --git a/test/Sentry.Tests/ApiApprovalTests.Run.DotNet7_0.verified.txt b/test/Sentry.Tests/ApiApprovalTests.Run.DotNet7_0.verified.txt index bf54b411e0..9891075c4d 100644 --- a/test/Sentry.Tests/ApiApprovalTests.Run.DotNet7_0.verified.txt +++ b/test/Sentry.Tests/ApiApprovalTests.Run.DotNet7_0.verified.txt @@ -395,7 +395,9 @@ namespace Sentry public void Apply(Sentry.IEventLike other) { } public void Apply(Sentry.Scope other) { } public void Apply(object state) { } + public void Clear() { } public void ClearAttachments() { } + public void ClearBreadcrumbs() { } public Sentry.Scope Clone() { } public Sentry.ISpan? GetSpan() { } public void SetExtra(string key, object? value) { } diff --git a/test/Sentry.Tests/ApiApprovalTests.Run.Net4_8.verified.txt b/test/Sentry.Tests/ApiApprovalTests.Run.Net4_8.verified.txt index b96457a8b1..d335930e45 100644 --- a/test/Sentry.Tests/ApiApprovalTests.Run.Net4_8.verified.txt +++ b/test/Sentry.Tests/ApiApprovalTests.Run.Net4_8.verified.txt @@ -394,7 +394,9 @@ namespace Sentry public void Apply(Sentry.IEventLike other) { } public void Apply(Sentry.Scope other) { } public void Apply(object state) { } + public void Clear() { } public void ClearAttachments() { } + public void ClearBreadcrumbs() { } public Sentry.Scope Clone() { } public Sentry.ISpan? GetSpan() { } public void SetExtra(string key, object? value) { } From dcc17a4bf551aa7a425bd94ed3656a76f1e34a8e Mon Sep 17 00:00:00 2001 From: James Crosswell Date: Wed, 5 Apr 2023 21:36:21 +1200 Subject: [PATCH 3/4] Added details of Scope.Clear and Scope.ClearBreadcrumbs to changelog --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6e7daa4937..dd831e120c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,8 @@ ## Unreleased +- Add Scope.Clear and Scope.ClearBreadcrumbs methods ([#2284](https://github.com/getsentry/sentry-dotnet/pull/2284)) + ### Features - Add `FileDiagnosticLogger` to assist with debugging the SDK ([#2242](https://github.com/getsentry/sentry-dotnet/pull/2242)) From 7de046fdc935e37a013e38202e7933a1b4651e8c Mon Sep 17 00:00:00 2001 From: James Crosswell Date: Thu, 6 Apr 2023 17:06:18 +1200 Subject: [PATCH 4/4] - Added test to ensure Scope.Clear doesn't affect parent scopes in the SentryScopeManager - Added extension method to set fake values on a Scope during testing - Added extension method to ensure Scopes are equivalent during testing --- .../Internals/SentryScopeManagerTests.cs | 40 ++++++++++ test/Sentry.Tests/ScopeTests.cs | 73 +++++++++++-------- 2 files changed, 82 insertions(+), 31 deletions(-) diff --git a/test/Sentry.Tests/Internals/SentryScopeManagerTests.cs b/test/Sentry.Tests/Internals/SentryScopeManagerTests.cs index 2f0482e7d1..e25da4f30e 100644 --- a/test/Sentry.Tests/Internals/SentryScopeManagerTests.cs +++ b/test/Sentry.Tests/Internals/SentryScopeManagerTests.cs @@ -1,3 +1,4 @@ +using FluentAssertions.Execution; using Sentry.Internal.ScopeStack; namespace Sentry.Tests.Internals; @@ -206,6 +207,44 @@ public void Scope_DisposedOutOfOrder() Assert.Equal(root, sut.GetCurrent()); } + [Fact] + public void Scope_Clear_DoesntAffectParentScope() + { + var sut = _fixture.GetSut(); + + //Set some values on the scope (and store these to compare later) + sut.GetCurrent().Key.ApplyFakeValues(); + Scope fakeValues = sut.GetCurrent().Key.Clone(); + fakeValues.Transaction.Should().NotBeNull(); // Sanity check... make sure it's not nulls everywhere + + //Push a new scope + var root = sut.PushScope(); + + // Ensure the values are there + using (new AssertionScope()) + { + sut.GetCurrent().Key.ShouldBeEquivalentTo(fakeValues); + } + + // Clear the child scope + sut.GetCurrent().Key.Clear(); + + //Ensure the values are gone + using (new AssertionScope()) + { + sut.GetCurrent().Key.ShouldBeEquivalentTo(new Scope()); + } + + //Pop the scope + root.Dispose(); + + //Ensure the value have returned + using (new AssertionScope()) + { + sut.GetCurrent().Key.ShouldBeEquivalentTo(fakeValues); + } + } + [Fact] public async Task AsyncTasks_IsolatedScopes() { @@ -330,6 +369,7 @@ public void GlobalMode_PushScope_SameScope() client1.Should().BeSameAs(client2); } + [Fact] public void GlobalMode_Disabled_Uses_AsyncLocalScopeStackContainer() { diff --git a/test/Sentry.Tests/ScopeTests.cs b/test/Sentry.Tests/ScopeTests.cs index e116e36439..d7de09a391 100644 --- a/test/Sentry.Tests/ScopeTests.cs +++ b/test/Sentry.Tests/ScopeTests.cs @@ -237,44 +237,15 @@ public void AddAttachment_AddAttachments() public void Clear_SetsPropertiesToDefaultValues() { // Arrange - _sut.Level = SentryLevel.Warning; - _sut.Request = new() { Data = "request to be cleared" }; - _sut.Contexts.Add("context to be cleared", "{}"); - _sut.User = new User() { Username = "username to be cleared" }; - _sut.Platform = "platform to be cleared"; - _sut.Release = "release to be cleared"; - _sut.Distribution = "distribution to be cleared"; - _sut.Environment = "environment to be cleared"; - _sut.TransactionName = "transaction name to be cleared"; - _sut.Transaction = Substitute.For(); - _sut.Fingerprint = new[] { "fingerprint to be cleared" }; - _sut.AddBreadcrumb(new(message: "breadcrumb to be cleared")); - _sut.SetExtra("extra", "extra to be cleared"); - _sut.SetTag("tag", "tag to be cleared"); - _sut.AddAttachment(new Attachment(default, default, default, "attachment to be cleared")); + _sut.ApplyFakeValues(); // Act _sut.Clear(); // Assert - var newScope = new Scope(); using (new AssertionScope()) { - _sut.Level.Should().Be(newScope.Level); - _sut.Request.Should().BeEquivalentTo(newScope.Request); - _sut.Contexts.Should().BeEquivalentTo(newScope.Contexts); - _sut.User.Should().BeEquivalentTo(newScope.User); - _sut.Platform.Should().Be(newScope.Platform); - _sut.Release.Should().Be(newScope.Release); - _sut.Distribution.Should().Be(newScope.Distribution); - _sut.Environment.Should().Be(newScope.Environment); - _sut.TransactionName.Should().Be(newScope.TransactionName); - _sut.Transaction.Should().Be(newScope.Transaction); - _sut.Fingerprint.Should().BeEquivalentTo(newScope.Fingerprint); - _sut.Breadcrumbs.Should().BeEquivalentTo(newScope.Breadcrumbs); - _sut.Extra.Should().BeEquivalentTo(newScope.Extra); - _sut.Tags.Should().BeEquivalentTo(newScope.Tags); - _sut.Attachments.Should().BeEquivalentTo(newScope.Attachments); + _sut.ShouldBeEquivalentTo(new Scope()); } } @@ -492,3 +463,43 @@ public void AddBreadcrumb_ObserverExist_ObserverAddsBreadcrumbIfEnabled(bool obs observer.Received(expectedCount).AddBreadcrumb(Arg.Is(breadcrumb)); } } + +public static class ScopeTestExtensions +{ + public static void ApplyFakeValues(this Scope scope, string salt = "fake") + { + scope.Request = new() { Data = $"{salt} request" }; + scope.Contexts.Add($"{salt} context", "{}"); + scope.User = new User() { Username = $"{salt} username" }; + scope.Platform = $"{salt} platform"; + scope.Release = $"{salt} release"; + scope.Distribution = $"{salt} distribution"; + scope.Environment = $"{salt} environment"; + scope.TransactionName = $"{salt} transaction"; + scope.Transaction = Substitute.For(); + scope.Fingerprint = new[] { $"{salt} fingerprint" }; + scope.AddBreadcrumb(new(message: $"{salt} breadcrumb")); + scope.SetExtra("extra", $"{salt} extra"); + scope.SetTag("tag", $"{salt} tag"); + scope.AddAttachment(new Attachment(default, default, default, $"{salt} attachment")); + } + + public static void ShouldBeEquivalentTo(this Scope source, Scope target) + { + source.Level.Should().Be(target.Level); + source.Request.Should().BeEquivalentTo(target.Request); + source.Contexts.Should().BeEquivalentTo(target.Contexts); + source.User.Should().BeEquivalentTo(target.User); + source.Platform.Should().Be(target.Platform); + source.Release.Should().Be(target.Release); + source.Distribution.Should().Be(target.Distribution); + source.Environment.Should().Be(target.Environment); + source.TransactionName.Should().Be(target.TransactionName); + source.Transaction.Should().Be(target.Transaction); + source.Fingerprint.Should().BeEquivalentTo(target.Fingerprint); + source.Breadcrumbs.Should().BeEquivalentTo(target.Breadcrumbs); + source.Extra.Should().BeEquivalentTo(target.Extra); + source.Tags.Should().BeEquivalentTo(target.Tags); + source.Attachments.Should().BeEquivalentTo(target.Attachments); + } +}