-
-
Notifications
You must be signed in to change notification settings - Fork 241
feat: Allow users to control Mechanism.Handled for captured exceptions #5449
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
base: main
Are you sure you want to change the base?
Changes from all commits
0540a78
40b0bec
4a0037a
6d37227
1d662b6
0dbfc6b
3663ee5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -2,6 +2,7 @@ | |||||
| using Sentry.Infrastructure; | ||||||
| using Sentry.Internal; | ||||||
| using Sentry.Internal.Extensions; | ||||||
| using Sentry.Protocol; | ||||||
|
|
||||||
| namespace Sentry; | ||||||
|
|
||||||
|
|
@@ -268,8 +269,16 @@ public LockedScope(IHub hub) | |||||
| public void Dispose() => _scope.Dispose(); | ||||||
| } | ||||||
|
|
||||||
| internal static SentryId CaptureExceptionInternal(this IHub hub, Exception ex) => | ||||||
| hub.CaptureEvent(new SentryEvent(ex)); | ||||||
| internal static SentryId CaptureExceptionInternal(this IHub hub, Exception ex) | ||||||
| { | ||||||
| // Integrations stamp the flag via SetSentryMechanism before calling this (e.g. WinUI forwards the | ||||||
| // platform's Handled value); only default to unhandled when nothing was declared. | ||||||
| if (!ex.Data.Contains(Mechanism.HandledKey)) | ||||||
| { | ||||||
| ex.Data[Mechanism.HandledKey] = false; | ||||||
| } | ||||||
| return hub.CaptureEvent(new SentryEvent(ex)); | ||||||
| } | ||||||
|
vladbrincoveanu marked this conversation as resolved.
|
||||||
|
|
||||||
| /// <summary> | ||||||
| /// Captures the exception with a configurable scope callback. | ||||||
|
|
@@ -281,6 +290,21 @@ internal static SentryId CaptureExceptionInternal(this IHub hub, Exception ex) = | |||||
| public static SentryId CaptureException(this IHub hub, Exception ex, Action<Scope> configureScope) => | ||||||
| hub.CaptureEvent(new SentryEvent(ex), configureScope); | ||||||
|
|
||||||
| /// <summary> | ||||||
| /// Captures the exception with a configurable scope callback, explicitly marking it as handled or unhandled. | ||||||
| /// </summary> | ||||||
| /// <param name="hub">The Sentry hub.</param> | ||||||
| /// <param name="ex">The exception.</param> | ||||||
| /// <param name="configureScope">The callback to configure the scope.</param> | ||||||
| /// <param name="handled">Whether the exception was handled. Recorded on the exception, overriding any flag | ||||||
| /// previously set on it, including one set via <see cref="SentryExceptionExtensions.SetSentryMechanism"/>.</param> | ||||||
| /// <returns>The Id of the event</returns> | ||||||
| public static SentryId CaptureException(this IHub hub, Exception ex, Action<Scope> configureScope, bool handled) | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd suggest reordering the parameters. The action will likely be a lambda function which could be arbitrarily long/complex. Having the bool as the last parameter would make things harder to read.
Suggested change
|
||||||
| { | ||||||
| ex.Data[Mechanism.HandledKey] = handled; | ||||||
| return hub.CaptureEvent(new SentryEvent(ex), configureScope); | ||||||
| } | ||||||
|
sentry[bot] marked this conversation as resolved.
|
||||||
|
|
||||||
| /// <summary> | ||||||
| /// Captures feedback from the user. | ||||||
| /// </summary> | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -174,17 +174,12 @@ private static Mechanism GetMechanism(Exception exception, int id, int? parentId | |
| mechanism.Handled = handled; | ||
| exception.Data.Remove(Mechanism.HandledKey); | ||
| } | ||
| else if (exception.StackTrace != null) | ||
| { | ||
| // The exception was thrown, but it was caught by the user, not an integration. | ||
| // Thus, we can mark it as handled. | ||
| mechanism.Handled = true; | ||
| } | ||
| else | ||
| { | ||
| // The exception was never thrown. It was just constructed and then captured. | ||
| // Thus, it is neither handled nor unhandled. | ||
| mechanism.Handled = null; | ||
| // https://getsentry.github.io/relay/relay_event_schema/protocol/struct.Mechanism.html#structfield.handled | ||
| // "Exceptions captured using capture_exception (called from user code) are handled=true as the user | ||
| // explicitly captured the exception (and therefore kind of handled it)." | ||
| mechanism.Handled = true; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This would be a change in behaviour. I think it probably makes sense... we can probably do it without making a major release - arguably it's a bug fix rather than an arbitrary change. @dingsdax do you have any steer? |
||
| } | ||
|
|
||
| if (exception.Data[Mechanism.MechanismKey] is string mechanismType) | ||
|
vladbrincoveanu marked this conversation as resolved.
|
||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,5 +1,6 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| using Sentry.Extensibility; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| using Sentry.Internal; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| using Sentry.Protocol; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| namespace Sentry; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -18,6 +19,25 @@ public static class SentryClientExtensions | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| public static SentryId CaptureException(this ISentryClient client, Exception ex) => | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| client.IsEnabled ? client.CaptureEvent(new SentryEvent(ex)) : SentryId.Empty; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// <summary> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// Captures the exception, explicitly marking it as handled or unhandled. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// </summary> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// <param name="client">The Sentry client.</param> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// <param name="ex">The exception.</param> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// <param name="handled">Whether the exception was handled. Recorded on the exception, overriding any flag | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// previously set on it, including one set via <see cref="SentryExceptionExtensions.SetSentryMechanism"/>.</param> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// <returns>The Id of the event</returns> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| public static SentryId CaptureException(this ISentryClient client, Exception ex, bool handled) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!client.IsEnabled) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return SentryId.Empty; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ex.Data[Mechanism.HandledKey] = handled; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return client.CaptureEvent(new SentryEvent(ex)); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unhandled capture crashes sessionsHigh Severity Public Additional Locations (1)Reviewed by Cursor Bugbot for commit 5e8b0b1. Configure here.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirmed, this is the current behavior: Adding terminal control to the (The related WinUI issue
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK this is going down a bit of a 🐇 🕳️ - apologies in advance for the long reply... The TLDR; is that I think we need to expose a way to set both Handled and Terminal (not just Handled). Detailed response
Session HealthSo sentry-dotnet/src/Sentry/SentryClient.cs Lines 392 to 409 in f02bc53
And importantly, unhandled exceptions are terminal by default in the .NET SDK: sentry-dotnet/src/Sentry/SentryEvent.cs Lines 223 to 265 in 29ef560
TransactionsThe impact on transactions is not dependent on the app model and so impacts all apps (including ASP.NET Core). An
What this means for this PR
So I don't think we can expose
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi James, |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// <summary> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// Captures a message. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// </summary> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -524,6 +524,17 @@ public static SentryId CaptureEvent(SentryEvent evt, SentryHint? hint, Action<Sc | |||||
| public static SentryId CaptureException(Exception exception) | ||||||
| => CurrentHub.CaptureException(exception); | ||||||
|
|
||||||
| /// <summary> | ||||||
| /// Captures the exception, explicitly marking it as handled or unhandled. | ||||||
| /// </summary> | ||||||
| /// <param name="exception">The exception.</param> | ||||||
| /// <param name="handled">Whether the exception was handled. Recorded on the exception, overriding any flag | ||||||
| /// previously set on it, including one set via <see cref="SentryExceptionExtensions.SetSentryMechanism"/>.</param> | ||||||
| /// <returns>The Id of the event.</returns> | ||||||
| [DebuggerStepThrough] | ||||||
| public static SentryId CaptureException(Exception exception, bool handled) | ||||||
| => CurrentHub.CaptureException(exception, handled); | ||||||
|
|
||||||
| /// <summary> | ||||||
| /// Captures the exception with a configurable scope. | ||||||
| /// </summary> | ||||||
|
|
@@ -537,6 +548,21 @@ public static SentryId CaptureException(Exception exception) | |||||
| public static SentryId CaptureException(Exception exception, Action<Scope> configureScope) | ||||||
| => CurrentHub.CaptureException(exception, configureScope); | ||||||
|
|
||||||
| /// <summary> | ||||||
| /// Captures the exception with a configurable scope, explicitly marking it as handled or unhandled. | ||||||
| /// </summary> | ||||||
| /// <remarks> | ||||||
| /// This allows modifying a scope without affecting other events. | ||||||
| /// </remarks> | ||||||
| /// <param name="exception">The exception.</param> | ||||||
| /// <param name="configureScope">The callback to configure the scope.</param> | ||||||
| /// <param name="handled">Whether the exception was handled. Recorded on the exception, overriding any flag | ||||||
| /// previously set on it, including one set via <see cref="SentryExceptionExtensions.SetSentryMechanism"/>.</param> | ||||||
| /// <returns>The Id of the event.</returns> | ||||||
| [DebuggerStepThrough] | ||||||
| public static SentryId CaptureException(Exception exception, Action<Scope> configureScope, bool handled) | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See related comment above.
Suggested change
|
||||||
| => CurrentHub.CaptureException(exception, configureScope, handled); | ||||||
|
|
||||||
| /// <summary> | ||||||
| /// Captures the message. | ||||||
| /// </summary> | ||||||
|
|
||||||


There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest we make this a bit clearer. This method is
internaland the mechanism is always set by internal integrations before it's called.We can add it - I think we should note in the comment that it's defensive only, if we do.