From d3a69f8a449de25622da6cbdc9aed5dc73a1de6f Mon Sep 17 00:00:00 2001 From: Matt Johnson-Pint Date: Sat, 31 Dec 2022 09:20:32 -0800 Subject: [PATCH 1/8] Check enabled on hub, not on extensions --- src/Sentry/HubExtensions.cs | 34 +++++++++++----------- src/Sentry/Internal/Hub.cs | 25 +++++++++++++++++ src/Sentry/SentryClientExtensions.cs | 42 ++++++++++++---------------- 3 files changed, 60 insertions(+), 41 deletions(-) diff --git a/src/Sentry/HubExtensions.cs b/src/Sentry/HubExtensions.cs index cfe63f57c4..333e2d9efc 100644 --- a/src/Sentry/HubExtensions.cs +++ b/src/Sentry/HubExtensions.cs @@ -162,10 +162,8 @@ public LockedScope(IHub hub) /// The exception. /// The callback to configure the scope. /// The Id of the event - public static SentryId CaptureException(this IHub hub, Exception ex, Action configureScope) - => !hub.IsEnabled - ? new SentryId() - : hub.CaptureEvent(new SentryEvent(ex), configureScope); + public static SentryId CaptureException(this IHub hub, Exception ex, Action configureScope) => + hub.CaptureEvent(new SentryEvent(ex), configureScope); /// /// Captures a message with a configurable scope callback. @@ -175,20 +173,22 @@ public static SentryId CaptureException(this IHub hub, Exception ex, ActionThe callback to configure the scope. /// The message level. /// The Id of the event - public static SentryId CaptureMessage( - this IHub hub, - string message, - Action configureScope, + public static SentryId CaptureMessage(this IHub hub, string message, Action configureScope, SentryLevel level = SentryLevel.Info) - => !hub.IsEnabled || string.IsNullOrWhiteSpace(message) - ? new SentryId() - : hub.CaptureEvent( - new SentryEvent - { - Message = message, - Level = level - }, - configureScope); + { + if (string.IsNullOrWhiteSpace(message)) + { + return new SentryId(); + } + + var sentryEvent = new SentryEvent + { + Message = message, + Level = level + }; + + return hub.CaptureEvent(sentryEvent, configureScope); + } internal static ITransaction StartTransaction( this IHub hub, diff --git a/src/Sentry/Internal/Hub.cs b/src/Sentry/Internal/Hub.cs index 506d8e8ce3..85193e768b 100644 --- a/src/Sentry/Internal/Hub.cs +++ b/src/Sentry/Internal/Hub.cs @@ -281,6 +281,11 @@ public void EndSession(SessionEndStatus status = SessionEndStatus.Exited) => public SentryId CaptureEvent(SentryEvent evt, Action configureScope) { + if (!IsEnabled) + { + return SentryId.Empty; + } + try { var clonedScope = ScopeManager.GetCurrent().Key.Clone(); @@ -297,6 +302,11 @@ public SentryId CaptureEvent(SentryEvent evt, Action configureScope) public SentryId CaptureEvent(SentryEvent evt, Scope? scope = null) { + if (!IsEnabled) + { + return SentryId.Empty; + } + try { var currentScope = ScopeManager.GetCurrent(); @@ -347,6 +357,11 @@ public SentryId CaptureEvent(SentryEvent evt, Scope? scope = null) public void CaptureUserFeedback(UserFeedback userFeedback) { + if (!IsEnabled) + { + return; + } + try { _ownedClient.CaptureUserFeedback(userFeedback); @@ -359,6 +374,11 @@ public void CaptureUserFeedback(UserFeedback userFeedback) public void CaptureTransaction(Transaction transaction) { + if (!IsEnabled) + { + return; + } + try { // Apply scope data @@ -395,6 +415,11 @@ public void CaptureTransaction(Transaction transaction) public void CaptureSession(SessionUpdate sessionUpdate) { + if (!IsEnabled) + { + return; + } + try { _ownedClient.CaptureSession(sessionUpdate); diff --git a/src/Sentry/SentryClientExtensions.cs b/src/Sentry/SentryClientExtensions.cs index 6828357060..febc39f4fb 100644 --- a/src/Sentry/SentryClientExtensions.cs +++ b/src/Sentry/SentryClientExtensions.cs @@ -15,12 +15,8 @@ public static class SentryClientExtensions /// The Sentry client. /// The exception. /// The Id of the event - public static SentryId CaptureException(this ISentryClient client, Exception ex) - { - return !client.IsEnabled - ? new SentryId() - : client.CaptureEvent(new SentryEvent(ex)); - } + public static SentryId CaptureException(this ISentryClient client, Exception ex) => + client.CaptureEvent(new SentryEvent(ex)); /// /// Captures a message. @@ -29,19 +25,21 @@ public static SentryId CaptureException(this ISentryClient client, Exception ex) /// The message to send. /// The message level. /// The Id of the event - public static SentryId CaptureMessage( - this ISentryClient client, - string message, + public static SentryId CaptureMessage(this ISentryClient client, string message, SentryLevel level = SentryLevel.Info) { - return !client.IsEnabled || string.IsNullOrWhiteSpace(message) - ? new SentryId() - : client.CaptureEvent( - new SentryEvent - { - Message = message, - Level = level - }); + if (string.IsNullOrWhiteSpace(message)) + { + return new SentryId(); + } + + var sentryEvent = new SentryEvent + { + Message = message, + Level = level + }; + + return client.CaptureEvent(sentryEvent); } /// @@ -52,13 +50,9 @@ public static SentryId CaptureMessage( /// The user email. /// The user comments. /// The optional username. - public static void CaptureUserFeedback(this ISentryClient client, SentryId eventId, string email, string comments, string? name = null) - { - if (client.IsEnabled) - { - client.CaptureUserFeedback(new UserFeedback(eventId, name, email, comments)); - } - } + public static void CaptureUserFeedback(this ISentryClient client, SentryId eventId, string email, string comments, + string? name = null) => + client.CaptureUserFeedback(new UserFeedback(eventId, name, email, comments)); /// /// Flushes the queue of captured events until the timeout set in From 49cee039a43beb079dbb402459f0fe4dff4afdc9 Mon Sep 17 00:00:00 2001 From: Matt Johnson-Pint Date: Sat, 31 Dec 2022 09:20:58 -0800 Subject: [PATCH 2/8] Update tests --- .../Extensibility/HubAdapterTests.cs | 2 +- test/Sentry.Tests/HubTests.cs | 81 +++++++++++++++++++ .../SentryClientExtensionsTests.cs | 44 +--------- 3 files changed, 86 insertions(+), 41 deletions(-) diff --git a/test/Sentry.Tests/Extensibility/HubAdapterTests.cs b/test/Sentry.Tests/Extensibility/HubAdapterTests.cs index de675260d0..414437e257 100644 --- a/test/Sentry.Tests/Extensibility/HubAdapterTests.cs +++ b/test/Sentry.Tests/Extensibility/HubAdapterTests.cs @@ -33,7 +33,7 @@ public void CaptureException_MockInvoked() { var expected = new Exception(); _ = HubAdapter.Instance.CaptureException(expected); - _ = Hub.Received(1).CaptureException(expected); + _ = Hub.Received(1).CaptureEvent(Arg.Is(s => s.Exception == expected)); } [Fact] diff --git a/test/Sentry.Tests/HubTests.cs b/test/Sentry.Tests/HubTests.cs index 4170323a55..d1b1f73537 100644 --- a/test/Sentry.Tests/HubTests.cs +++ b/test/Sentry.Tests/HubTests.cs @@ -964,6 +964,87 @@ public void CaptureEvent_MessageOnlyEvent_SpanLinkedToEventContext(SentryLevel l Assert.Null(child.Status); } + [Theory] + [InlineData(true)] + [InlineData(false)] + public void CaptureEvent_HubEnabled(bool enabled) + { + // Arrange + var hub = _fixture.GetSut(); + if (!enabled) + { + hub.Dispose(); + } + + var evt = new SentryEvent(); + + // Act + hub.CaptureEvent(evt); + + // Assert + _fixture.Client.Received(enabled ? 1 : 0).CaptureEvent(Arg.Any(), Arg.Any()); + } + + [Theory] + [InlineData(true)] + [InlineData(false)] + public void CaptureUserFeedback_HubEnabled(bool enabled) + { + // Arrange + var hub = _fixture.GetSut(); + if (!enabled) + { + hub.Dispose(); + } + + var feedback = new UserFeedback(SentryId.Create(), "foo", "bar", "baz"); + + // Act + hub.CaptureUserFeedback(feedback); + + // Assert + _fixture.Client.Received(enabled ? 1 : 0).CaptureUserFeedback(Arg.Any()); + } + + [Theory] + [InlineData(true)] + [InlineData(false)] + public void CaptureSession_HubEnabled(bool enabled) + { + // Arrange + var hub = _fixture.GetSut(); + if (!enabled) + { + hub.Dispose(); + } + + // Act + hub.StartSession(); + + // Assert + _fixture.Client.Received(enabled ? 1 : 0).CaptureSession(Arg.Any()); + } + + [Theory] + [InlineData(true)] + [InlineData(false)] + public void CaptureTransaction_HubEnabled(bool enabled) + { + // Arrange + var hub = _fixture.GetSut(); + if (!enabled) + { + hub.Dispose(); + } + + // Act + var t = hub.StartTransaction("test", "test"); + t.Finish(); + + // Assert + _fixture.Client.Received(enabled ? 1 : 0).CaptureTransaction(Arg.Any()); + } + [Theory] [InlineData(false)] [InlineData(true)] diff --git a/test/Sentry.Tests/SentryClientExtensionsTests.cs b/test/Sentry.Tests/SentryClientExtensionsTests.cs index c88a4a7472..bdb434f836 100644 --- a/test/Sentry.Tests/SentryClientExtensionsTests.cs +++ b/test/Sentry.Tests/SentryClientExtensionsTests.cs @@ -5,37 +5,15 @@ public class SentryClientExtensionsTests private readonly ISentryClient _sut = Substitute.For(); [Fact] - public void CaptureException_DisabledClient_DoesNotCaptureEvent() + public void CaptureException_CapturesEvent() { - _ = _sut.IsEnabled.Returns(false); - var id = _sut.CaptureException(new Exception()); - - _ = _sut.DidNotReceive().CaptureEvent(Arg.Any()); - Assert.Equal(default, id); - } - - [Fact] - public void CaptureException_EnabledClient_CapturesEvent() - { - _ = _sut.IsEnabled.Returns(true); _ = _sut.CaptureException(new Exception()); _ = _sut.Received(1).CaptureEvent(Arg.Any()); } [Fact] - public void CaptureMessage_DisabledClient_DoesNotCaptureEvent() + public void CaptureMessage_CapturesEvent() { - _ = _sut.IsEnabled.Returns(false); - var id = _sut.CaptureMessage("Message"); - - _ = _sut.DidNotReceive().CaptureEvent(Arg.Any()); - Assert.Equal(default, id); - } - - [Fact] - public void CaptureMessage_EnabledClient_CapturesEvent() - { - _ = _sut.IsEnabled.Returns(true); _ = _sut.CaptureMessage("Message"); _ = _sut.Received(1).CaptureEvent(Arg.Any()); } @@ -44,7 +22,6 @@ public void CaptureMessage_EnabledClient_CapturesEvent() public void CaptureMessage_Level_CapturesEventWithLevel() { const SentryLevel expectedLevel = SentryLevel.Fatal; - _ = _sut.IsEnabled.Returns(true); _ = _sut.CaptureMessage("Message", expectedLevel); _ = _sut.Received(1).CaptureEvent(Arg.Is(e => e.Level == expectedLevel)); } @@ -53,7 +30,6 @@ public void CaptureMessage_Level_CapturesEventWithLevel() public void CaptureMessage_Message_CapturesEventWithMessage() { const string expectedMessage = "message"; - _ = _sut.IsEnabled.Returns(true); _ = _sut.CaptureMessage(expectedMessage); _ = _sut.Received(1).CaptureEvent(Arg.Is(e => e.Message.Message == expectedMessage)); } @@ -61,7 +37,6 @@ public void CaptureMessage_Message_CapturesEventWithMessage() [Fact] public void CaptureMessage_WhitespaceMessage_DoesNotCapturesEventWithMessage() { - _ = _sut.IsEnabled.Returns(true); var id = _sut.CaptureMessage(" "); _ = _sut.DidNotReceive().CaptureEvent(Arg.Any()); @@ -71,30 +46,19 @@ public void CaptureMessage_WhitespaceMessage_DoesNotCapturesEventWithMessage() [Fact] public void CaptureMessage_NullMessage_DoesNotCapturesEventWithMessage() { - _ = _sut.IsEnabled.Returns(true); - var id = _sut.CaptureMessage(null); + var id = _sut.CaptureMessage(null!); _ = _sut.DidNotReceive().CaptureEvent(Arg.Any()); Assert.Equal(default, id); } [Fact] - public void CaptureUserFeedback_EnabledClient_CapturesUserFeedback() + public void CaptureUserFeedback_CapturesUserFeedback() { - _ = _sut.IsEnabled.Returns(true); _sut.CaptureUserFeedback(Guid.Parse("1ec19311a7c048818de80b18dcc43eaa"), "email@email.com", "comments"); _sut.Received(1).CaptureUserFeedback(Arg.Any()); } - [Fact] - public void CaptureUserFeedback_DisabledClient_DoesNotCaptureUserFeedback() - { - _ = _sut.IsEnabled.Returns(false); - _sut.CaptureUserFeedback(Guid.Parse("1ec19311a7c048818de80b18dcc43eea"), "email@email.com", "comments"); - - _sut.DidNotReceive().CaptureUserFeedback(Arg.Any()); - } - [Fact] public async Task FlushAsync_NoTimeoutSpecified_UsesFlushTimeoutFromOptions() { From f14da92f9c834446b789f9becfb69ef9cf08d6cf Mon Sep 17 00:00:00 2001 From: Matt Johnson-Pint Date: Sat, 31 Dec 2022 10:04:36 -0800 Subject: [PATCH 3/8] Don't check enabled when exception unhandled --- src/Sentry/HubExtensions.cs | 6 ++++++ .../AppDomainUnhandledExceptionIntegration.cs | 9 +++++++-- .../UnobservedTaskExceptionIntegration.cs | 4 +++- .../WinUIUnhandledExceptionIntegration.cs | 4 +++- src/Sentry/Internal/Hub.cs | 12 +++++------- src/Sentry/Internal/IHubEx.cs | 6 ++++++ 6 files changed, 30 insertions(+), 11 deletions(-) create mode 100644 src/Sentry/Internal/IHubEx.cs diff --git a/src/Sentry/HubExtensions.cs b/src/Sentry/HubExtensions.cs index 333e2d9efc..5a575723e8 100644 --- a/src/Sentry/HubExtensions.cs +++ b/src/Sentry/HubExtensions.cs @@ -155,6 +155,12 @@ public LockedScope(IHub hub) public void Dispose() => _scope.Dispose(); } + internal static SentryId CaptureExceptionInternal(this IHub hub, Exception ex) => + hub.CaptureEventInternal(new SentryEvent(ex)); + + internal static SentryId CaptureEventInternal(this IHub hub, SentryEvent evt) => + hub is IHubEx hubEx ? hubEx.CaptureEventInternal(evt) : hub.CaptureEvent(evt); + /// /// Captures the exception with a configurable scope callback. /// diff --git a/src/Sentry/Integrations/AppDomainUnhandledExceptionIntegration.cs b/src/Sentry/Integrations/AppDomainUnhandledExceptionIntegration.cs index 08e3ec85a6..9f948da005 100644 --- a/src/Sentry/Integrations/AppDomainUnhandledExceptionIntegration.cs +++ b/src/Sentry/Integrations/AppDomainUnhandledExceptionIntegration.cs @@ -1,3 +1,4 @@ +using Sentry.Extensibility; using Sentry.Internal; using Sentry.Protocol; @@ -26,11 +27,15 @@ public void Register(IHub hub, SentryOptions options) [SecurityCritical] internal void Handle(object sender, UnhandledExceptionEventArgs e) { + _options?.LogDebug("AppDomain Unhandled Exception"); + if (e.ExceptionObject is Exception ex) { ex.Data[Mechanism.HandledKey] = false; ex.Data[Mechanism.MechanismKey] = "AppDomain.UnhandledException"; - _ = _hub?.CaptureException(ex); + + // Call the internal implementation, so that we still capture even if the hub has been disabled. + _hub?.CaptureExceptionInternal(ex); } if (e.IsTerminating) @@ -38,4 +43,4 @@ internal void Handle(object sender, UnhandledExceptionEventArgs e) _hub?.Flush(_options!.ShutdownTimeout); } } -} \ No newline at end of file +} diff --git a/src/Sentry/Integrations/UnobservedTaskExceptionIntegration.cs b/src/Sentry/Integrations/UnobservedTaskExceptionIntegration.cs index 4d0d7a8d3c..b6c45e3c22 100644 --- a/src/Sentry/Integrations/UnobservedTaskExceptionIntegration.cs +++ b/src/Sentry/Integrations/UnobservedTaskExceptionIntegration.cs @@ -34,6 +34,8 @@ internal void Handle(object? sender, UnobservedTaskExceptionEventArgs e) #endif ex.Data[Mechanism.HandledKey] = false; ex.Data[Mechanism.MechanismKey] = "UnobservedTaskException"; - _hub.CaptureException(ex); + + // Call the internal implementation, so that we still capture even if the hub has been disabled. + _hub.CaptureExceptionInternal(ex); } } diff --git a/src/Sentry/Integrations/WinUIUnhandledExceptionIntegration.cs b/src/Sentry/Integrations/WinUIUnhandledExceptionIntegration.cs index 774d1acd68..e65ef416cf 100644 --- a/src/Sentry/Integrations/WinUIUnhandledExceptionIntegration.cs +++ b/src/Sentry/Integrations/WinUIUnhandledExceptionIntegration.cs @@ -118,7 +118,9 @@ private void WinUIUnhandledExceptionHandler(object sender, object e) // Set some useful data and capture the exception exception.Data[Protocol.Mechanism.HandledKey] = handled; exception.Data[Protocol.Mechanism.MechanismKey] = "Microsoft.UI.Xaml.UnhandledException"; - _hub.CaptureException(exception); + + // Call the internal implementation, so that we still capture even if the hub has been disabled. + _hub.CaptureExceptionInternal(exception); if (!handled) { diff --git a/src/Sentry/Internal/Hub.cs b/src/Sentry/Internal/Hub.cs index 85193e768b..c8fa437a90 100644 --- a/src/Sentry/Internal/Hub.cs +++ b/src/Sentry/Internal/Hub.cs @@ -3,7 +3,7 @@ namespace Sentry.Internal; -internal class Hub : IHub, IDisposable +internal class Hub : IHubEx, IDisposable { private readonly object _sessionPauseLock = new(); @@ -300,13 +300,11 @@ public SentryId CaptureEvent(SentryEvent evt, Action configureScope) } } - public SentryId CaptureEvent(SentryEvent evt, Scope? scope = null) - { - if (!IsEnabled) - { - return SentryId.Empty; - } + public SentryId CaptureEvent(SentryEvent evt, Scope? scope = null) => + IsEnabled ? ((IHubEx)this).CaptureEventInternal(evt, scope) : SentryId.Empty; + SentryId IHubEx.CaptureEventInternal(SentryEvent evt, Scope? scope) + { try { var currentScope = ScopeManager.GetCurrent(); diff --git a/src/Sentry/Internal/IHubEx.cs b/src/Sentry/Internal/IHubEx.cs new file mode 100644 index 0000000000..e912cf32a8 --- /dev/null +++ b/src/Sentry/Internal/IHubEx.cs @@ -0,0 +1,6 @@ +namespace Sentry.Internal; + +internal interface IHubEx : IHub +{ + SentryId CaptureEventInternal(SentryEvent evt, Scope? scope = null); +} From c1d486b5a32a96b0dcc0a9cc3db2f97b1766c97c Mon Sep 17 00:00:00 2001 From: Matt Johnson-Pint Date: Sat, 31 Dec 2022 10:04:45 -0800 Subject: [PATCH 4/8] Update tests --- ...omainUnhandledExceptionIntegrationTests.cs | 21 ++++++++----------- ...UnobservedTaskExceptionIntegrationTests.cs | 6 +++--- 2 files changed, 12 insertions(+), 15 deletions(-) diff --git a/test/Sentry.Tests/AppDomainUnhandledExceptionIntegrationTests.cs b/test/Sentry.Tests/AppDomainUnhandledExceptionIntegrationTests.cs index 2ff82b981c..d67b15426f 100644 --- a/test/Sentry.Tests/AppDomainUnhandledExceptionIntegrationTests.cs +++ b/test/Sentry.Tests/AppDomainUnhandledExceptionIntegrationTests.cs @@ -4,17 +4,14 @@ public class AppDomainUnhandledExceptionIntegrationTests { private class Fixture { - public IHub Hub { get; set; } = Substitute.For(); - public IAppDomain AppDomain { get; set; } = Substitute.For(); + public IHubEx Hub { get; } = Substitute.For(); + public IAppDomain AppDomain { get; } = Substitute.For(); - public Fixture() => Hub.IsEnabled.Returns(true); - - public AppDomainUnhandledExceptionIntegration GetSut() - => new(AppDomain); + public AppDomainUnhandledExceptionIntegration GetSut() => new(AppDomain); } private readonly Fixture _fixture = new(); - public SentryOptions SentryOptions { get; set; } = new(); + private SentryOptions SentryOptions { get; } = new(); [Fact] public void Handle_WithException_CaptureEvent() @@ -24,7 +21,7 @@ public void Handle_WithException_CaptureEvent() sut.Handle(this, new UnhandledExceptionEventArgs(new Exception(), true)); - _ = _fixture.Hub.Received(1).CaptureEvent(Arg.Any()); + _fixture.Hub.Received(1).CaptureEventInternal(Arg.Any()); } [Fact] @@ -35,7 +32,7 @@ public void Handle_NoException_NoCaptureEvent() sut.Handle(this, new UnhandledExceptionEventArgs(new object(), true)); - _ = _fixture.Hub.DidNotReceive().CaptureEvent(Arg.Any()); + _fixture.Hub.DidNotReceive().CaptureEventInternal(Arg.Any()); } [Fact] @@ -57,7 +54,7 @@ public void Handle_TerminatingTrue_IsHandledFalse() var exception = new Exception(); sut.Handle(this, new UnhandledExceptionEventArgs(exception, true)); - Assert.False((bool)exception.Data[Mechanism.HandledKey]); + Assert.Equal(false, exception.Data[Mechanism.HandledKey]); Assert.True(exception.Data.Contains(Mechanism.MechanismKey)); var stackTraceFactory = Substitute.For(); @@ -65,7 +62,7 @@ public void Handle_TerminatingTrue_IsHandledFalse() var @event = new SentryEvent(exception); exceptionProcessor.Process(exception, @event); - Assert.NotNull(@event.SentryExceptions.ToList().Single(p => p.Mechanism.Handled == false)); + Assert.NotNull(@event.SentryExceptions?.ToList().Single(p => p.Mechanism?.Handled == false)); } [Fact] @@ -74,7 +71,7 @@ public void Handle_TerminatingTrue_NoException_FlushesHub() var sut = _fixture.GetSut(); sut.Register(_fixture.Hub, SentryOptions); - sut.Handle(this, new UnhandledExceptionEventArgs(null, true)); + sut.Handle(this, new UnhandledExceptionEventArgs(null!, true)); _fixture.Hub.Received(1).FlushAsync(Arg.Any()); } diff --git a/test/Sentry.Tests/UnobservedTaskExceptionIntegrationTests.cs b/test/Sentry.Tests/UnobservedTaskExceptionIntegrationTests.cs index 1a40d7a6bd..9a09d6689f 100644 --- a/test/Sentry.Tests/UnobservedTaskExceptionIntegrationTests.cs +++ b/test/Sentry.Tests/UnobservedTaskExceptionIntegrationTests.cs @@ -4,7 +4,7 @@ public class UnobservedTaskExceptionIntegrationTests { private class Fixture { - public IHub Hub { get; set; } = Substitute.For(); + public IHubEx Hub { get; set; } = Substitute.For(); public IAppDomain AppDomain { get; set; } = Substitute.For(); public Fixture() => Hub.IsEnabled.Returns(true); @@ -24,7 +24,7 @@ public void Handle_WithException_CaptureEvent() sut.Handle(this, new UnobservedTaskExceptionEventArgs(new AggregateException())); - _ = _fixture.Hub.Received(1).CaptureEvent(Arg.Any()); + _ = _fixture.Hub.Received(1).CaptureEventInternal(Arg.Any()); } // Test is flaky on mobile in CI. @@ -35,7 +35,7 @@ public void Handle_UnobservedTaskException_CaptureEvent() _fixture.AppDomain = AppDomainAdapter.Instance; var captureCalledEvent = new ManualResetEvent(false); SentryEvent capturedEvent = null; - _fixture.Hub.When(x => x.CaptureEvent(Arg.Any())) + _fixture.Hub.When(x => x.CaptureEventInternal(Arg.Any())) .Do(callInfo => { capturedEvent = callInfo.Arg(); From 96f8ac8dbf42e267c7e793cbab469f6e20f51c8f Mon Sep 17 00:00:00 2001 From: Matt Johnson-Pint Date: Sat, 31 Dec 2022 10:10:15 -0800 Subject: [PATCH 5/8] Update CHANGELOG.md --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index f4b7433edf..1162a83230 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ ### Fixes - Logging info instead of warning when skipping debug images ([#2101](https://github.com/getsentry/sentry-dotnet/pull/2101)) +- Fix unhandled exception not captured when hub disabled ([#2103](https://github.com/getsentry/sentry-dotnet/pull/2103)) ## 3.25.0 From 0b622f667be728c3afc751c9b9643a5cb34cf402 Mon Sep 17 00:00:00 2001 From: Matt Johnson-Pint Date: Sat, 31 Dec 2022 10:13:36 -0800 Subject: [PATCH 6/8] Update sample --- samples/Sentry.Samples.Console.Basic/Program.cs | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/samples/Sentry.Samples.Console.Basic/Program.cs b/samples/Sentry.Samples.Console.Basic/Program.cs index 3338bc29ef..c42c429cfc 100644 --- a/samples/Sentry.Samples.Console.Basic/Program.cs +++ b/samples/Sentry.Samples.Console.Basic/Program.cs @@ -1,7 +1,13 @@ using Sentry; -using (SentrySdk.Init("https://eb18e953812b41c3aeb042e666fd3b5c@o447951.ingest.sentry.io/5428537")) +using var _ = SentrySdk.Init(o => { - // The following exception is captured and sent to Sentry - throw null; -} + // The DSN is required. + o.Dsn = "https://eb18e953812b41c3aeb042e666fd3b5c@o447951.ingest.sentry.io/5428537"; + + // When debug is enabled, the Sentry client will emit detailed debugging information to the console. + o.Debug = true; +}); + +// The following unhandled exception will be captured and sent to Sentry. +throw new Exception("test"); From c4ebc449c9210ce110299c50676c9199cfd5a957 Mon Sep 17 00:00:00 2001 From: Matt Johnson-Pint Date: Sat, 31 Dec 2022 17:35:37 -0800 Subject: [PATCH 7/8] . --- test/Sentry.Tests/HubTests.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/test/Sentry.Tests/HubTests.cs b/test/Sentry.Tests/HubTests.cs index d1b1f73537..ca8f7f6631 100644 --- a/test/Sentry.Tests/HubTests.cs +++ b/test/Sentry.Tests/HubTests.cs @@ -1012,6 +1012,7 @@ public void CaptureUserFeedback_HubEnabled(bool enabled) public void CaptureSession_HubEnabled(bool enabled) { // Arrange + _fixture.Options.Release = "release"; var hub = _fixture.GetSut(); if (!enabled) { From 54c13d68072e643da1a95013da61308aab6c1bc7 Mon Sep 17 00:00:00 2001 From: Matt Johnson-Pint Date: Tue, 3 Jan 2023 10:09:24 -0800 Subject: [PATCH 8/8] . --- src/Sentry/SentryClientExtensions.cs | 34 ++++++++++----- src/Sentry/SentryId.cs | 2 +- .../Extensibility/HubAdapterTests.cs | 41 +++++++++--------- .../SentryClientExtensionsTests.cs | 42 +++++++++++++++++-- 4 files changed, 83 insertions(+), 36 deletions(-) diff --git a/src/Sentry/SentryClientExtensions.cs b/src/Sentry/SentryClientExtensions.cs index febc39f4fb..7eea18d247 100644 --- a/src/Sentry/SentryClientExtensions.cs +++ b/src/Sentry/SentryClientExtensions.cs @@ -16,7 +16,7 @@ public static class SentryClientExtensions /// The exception. /// The Id of the event public static SentryId CaptureException(this ISentryClient client, Exception ex) => - client.CaptureEvent(new SentryEvent(ex)); + client.IsEnabled ? client.CaptureEventInternal(new SentryEvent(ex)) : SentryId.Empty; /// /// Captures a message. @@ -28,18 +28,16 @@ public static SentryId CaptureException(this ISentryClient client, Exception ex) public static SentryId CaptureMessage(this ISentryClient client, string message, SentryLevel level = SentryLevel.Info) { - if (string.IsNullOrWhiteSpace(message)) + if (client.IsEnabled && !string.IsNullOrWhiteSpace(message)) { - return new SentryId(); + return client.CaptureEventInternal(new SentryEvent + { + Message = message, + Level = level + }); } - var sentryEvent = new SentryEvent - { - Message = message, - Level = level - }; - - return client.CaptureEvent(sentryEvent); + return SentryId.Empty; } /// @@ -51,8 +49,22 @@ public static SentryId CaptureMessage(this ISentryClient client, string message, /// The user comments. /// The optional username. public static void CaptureUserFeedback(this ISentryClient client, SentryId eventId, string email, string comments, - string? name = null) => + string? name = null) + { + if (!client.IsEnabled) + { + return; + } + client.CaptureUserFeedback(new UserFeedback(eventId, name, email, comments)); + } + + private static SentryId CaptureEventInternal(this ISentryClient client, SentryEvent sentryEvent) => + client switch + { + IHubEx hub => hub.CaptureEventInternal(sentryEvent), + _ => client.CaptureEvent(sentryEvent) + }; /// /// Flushes the queue of captured events until the timeout set in diff --git a/src/Sentry/SentryId.cs b/src/Sentry/SentryId.cs index fd23afda8b..955702e07f 100644 --- a/src/Sentry/SentryId.cs +++ b/src/Sentry/SentryId.cs @@ -12,7 +12,7 @@ namespace Sentry; /// /// An empty sentry id. /// - public static readonly SentryId Empty = Guid.Empty; + public static readonly SentryId Empty = default; /// /// Creates a new instance of a Sentry Id. diff --git a/test/Sentry.Tests/Extensibility/HubAdapterTests.cs b/test/Sentry.Tests/Extensibility/HubAdapterTests.cs index 414437e257..8ca4314741 100644 --- a/test/Sentry.Tests/Extensibility/HubAdapterTests.cs +++ b/test/Sentry.Tests/Extensibility/HubAdapterTests.cs @@ -3,20 +3,20 @@ namespace Sentry.Tests.Extensibility; [Collection(nameof(SentrySdkCollection))] public class HubAdapterTests : IDisposable { - public IHub Hub { get; set; } + private IHub Hub { get; } public HubAdapterTests() { Hub = Substitute.For(); - _ = SentrySdk.UseHub(Hub); + SentrySdk.UseHub(Hub); } [Fact] public void CaptureEvent_MockInvoked() { var expected = new SentryEvent(); - _ = HubAdapter.Instance.CaptureEvent(expected); - _ = Hub.Received(1).CaptureEvent(expected); + HubAdapter.Instance.CaptureEvent(expected); + Hub.Received(1).CaptureEvent(expected); } [Fact] @@ -24,16 +24,17 @@ public void CaptureEvent_WithScope_MockInvoked() { var expectedEvent = new SentryEvent(); var expectedScope = new Scope(); - _ = HubAdapter.Instance.CaptureEvent(expectedEvent, expectedScope); - _ = Hub.Received(1).CaptureEvent(expectedEvent, expectedScope); + HubAdapter.Instance.CaptureEvent(expectedEvent, expectedScope); + Hub.Received(1).CaptureEvent(expectedEvent, expectedScope); } [Fact] public void CaptureException_MockInvoked() { var expected = new Exception(); - _ = HubAdapter.Instance.CaptureException(expected); - _ = Hub.Received(1).CaptureEvent(Arg.Is(s => s.Exception == expected)); + Hub.IsEnabled.Returns(true); + HubAdapter.Instance.CaptureException(expected); + Hub.Received(1).CaptureEvent(Arg.Is(s => s.Exception == expected)); } [Fact] @@ -65,15 +66,14 @@ public void ConfigureScopeAsync_MockInvoked() { static Task Expected(Scope _) => Task.CompletedTask; - _ = HubAdapter.Instance.ConfigureScopeAsync(Expected); - _ = Hub.Received(1).ConfigureScopeAsync(Expected); + HubAdapter.Instance.ConfigureScopeAsync(Expected); + Hub.Received(1).ConfigureScopeAsync(Expected); } [Fact] public void ConfigureScope_MockInvoked() { - void Expected(Scope _) - { } + void Expected(Scope _) { } HubAdapter.Instance.ConfigureScope(Expected); Hub.Received(1).ConfigureScope(Expected); } @@ -82,8 +82,7 @@ void Expected(Scope _) [Fact] public void WithScope_MockInvoked() { - void Expected(Scope _) - { } + void Expected(Scope _) { } HubAdapter.Instance.WithScope(Expected); Hub.Received(1).WithScope(Expected); } @@ -91,16 +90,16 @@ void Expected(Scope _) [Fact] public void PushScope_MockInvoked() { - _ = HubAdapter.Instance.PushScope(); - _ = Hub.Received(1).PushScope(); + HubAdapter.Instance.PushScope(); + Hub.Received(1).PushScope(); } [Fact] public void PushScope_State_MockInvoked() { var expected = new object(); - _ = HubAdapter.Instance.PushScope(expected); - _ = Hub.Received(1).PushScope(expected); + HubAdapter.Instance.PushScope(expected); + Hub.Received(1).PushScope(expected); } [Fact] @@ -121,7 +120,7 @@ public void AddBreadcrumb_BreadcrumbInstanceCreated() public void AddBreadcrumb_WithClock_BreadcrumbInstanceCreated() { var clock = Substitute.For(); - _ = clock.GetUtcNow().Returns(DateTimeOffset.MaxValue); + clock.GetUtcNow().Returns(DateTimeOffset.MaxValue); TestAddBreadcrumbExtension((message, category, type, data, level) => HubAdapter.Instance.AddBreadcrumb( @@ -132,7 +131,7 @@ public void AddBreadcrumb_WithClock_BreadcrumbInstanceCreated() data, level)); - _ = clock.Received(1).GetUtcNow(); + clock.Received(1).GetUtcNow(); } private void TestAddBreadcrumbExtension( @@ -164,7 +163,7 @@ private void TestAddBreadcrumbExtension( Assert.Equal(type, crumb.Type); Assert.Equal(category, crumb.Category); Assert.Equal(level, crumb.Level); - Assert.Equal(data.Count, crumb.Data.Count); + Assert.Equal(data.Count, crumb.Data?.Count); Assert.Equal(data.ToImmutableDictionary(), crumb.Data); } diff --git a/test/Sentry.Tests/SentryClientExtensionsTests.cs b/test/Sentry.Tests/SentryClientExtensionsTests.cs index bdb434f836..9210c7c62f 100644 --- a/test/Sentry.Tests/SentryClientExtensionsTests.cs +++ b/test/Sentry.Tests/SentryClientExtensionsTests.cs @@ -5,15 +5,37 @@ public class SentryClientExtensionsTests private readonly ISentryClient _sut = Substitute.For(); [Fact] - public void CaptureException_CapturesEvent() + public void CaptureException_DisabledClient_DoesNotCaptureEvent() { + _ = _sut.IsEnabled.Returns(false); + var id = _sut.CaptureException(new Exception()); + + _ = _sut.DidNotReceive().CaptureEvent(Arg.Any()); + Assert.Equal(default, id); + } + + [Fact] + public void CaptureException_EnabledClient_CapturesEvent() + { + _ = _sut.IsEnabled.Returns(true); _ = _sut.CaptureException(new Exception()); _ = _sut.Received(1).CaptureEvent(Arg.Any()); } [Fact] - public void CaptureMessage_CapturesEvent() + public void CaptureMessage_DisabledClient_DoesNotCaptureEvent() { + _ = _sut.IsEnabled.Returns(false); + var id = _sut.CaptureMessage("Message"); + + _ = _sut.DidNotReceive().CaptureEvent(Arg.Any()); + Assert.Equal(default, id); + } + + [Fact] + public void CaptureMessage_EnabledClient_CapturesEvent() + { + _ = _sut.IsEnabled.Returns(true); _ = _sut.CaptureMessage("Message"); _ = _sut.Received(1).CaptureEvent(Arg.Any()); } @@ -22,6 +44,7 @@ public void CaptureMessage_CapturesEvent() public void CaptureMessage_Level_CapturesEventWithLevel() { const SentryLevel expectedLevel = SentryLevel.Fatal; + _ = _sut.IsEnabled.Returns(true); _ = _sut.CaptureMessage("Message", expectedLevel); _ = _sut.Received(1).CaptureEvent(Arg.Is(e => e.Level == expectedLevel)); } @@ -30,6 +53,7 @@ public void CaptureMessage_Level_CapturesEventWithLevel() public void CaptureMessage_Message_CapturesEventWithMessage() { const string expectedMessage = "message"; + _ = _sut.IsEnabled.Returns(true); _ = _sut.CaptureMessage(expectedMessage); _ = _sut.Received(1).CaptureEvent(Arg.Is(e => e.Message.Message == expectedMessage)); } @@ -37,6 +61,7 @@ public void CaptureMessage_Message_CapturesEventWithMessage() [Fact] public void CaptureMessage_WhitespaceMessage_DoesNotCapturesEventWithMessage() { + _ = _sut.IsEnabled.Returns(true); var id = _sut.CaptureMessage(" "); _ = _sut.DidNotReceive().CaptureEvent(Arg.Any()); @@ -46,6 +71,7 @@ public void CaptureMessage_WhitespaceMessage_DoesNotCapturesEventWithMessage() [Fact] public void CaptureMessage_NullMessage_DoesNotCapturesEventWithMessage() { + _ = _sut.IsEnabled.Returns(true); var id = _sut.CaptureMessage(null!); _ = _sut.DidNotReceive().CaptureEvent(Arg.Any()); @@ -53,12 +79,22 @@ public void CaptureMessage_NullMessage_DoesNotCapturesEventWithMessage() } [Fact] - public void CaptureUserFeedback_CapturesUserFeedback() + public void CaptureUserFeedback_EnabledClient_CapturesUserFeedback() { + _ = _sut.IsEnabled.Returns(true); _sut.CaptureUserFeedback(Guid.Parse("1ec19311a7c048818de80b18dcc43eaa"), "email@email.com", "comments"); _sut.Received(1).CaptureUserFeedback(Arg.Any()); } + [Fact] + public void CaptureUserFeedback_DisabledClient_DoesNotCaptureUserFeedback() + { + _ = _sut.IsEnabled.Returns(false); + _sut.CaptureUserFeedback(Guid.Parse("1ec19311a7c048818de80b18dcc43eea"), "email@email.com", "comments"); + + _sut.DidNotReceive().CaptureUserFeedback(Arg.Any()); + } + [Fact] public async Task FlushAsync_NoTimeoutSpecified_UsesFlushTimeoutFromOptions() {