-
Notifications
You must be signed in to change notification settings - Fork 215
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
feat: add support for exception mechanism metadata #564
Conversation
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.
meta
is missing https://github.com/getsentry/relay/blob/e785f9a22adef018cec8785a7dec289a6ae74165/relay-general/src/protocol/mechanism.rs#L154
There are also some docs that might explain all of this in more detail https://develop.sentry.dev/sdk/event-payloads/exception/
Codecov ReportBase: 79.14% // Head: 79.16% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #564 +/- ##
==========================================
+ Coverage 79.14% 79.16% +0.01%
==========================================
Files 38 38
Lines 3856 3859 +3
==========================================
+ Hits 3052 3055 +3
Misses 703 703
Partials 101 101
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
Ah, yeah. That was an intentional omission because it didn't look like any of the |
Ok, we can also start with |
I'll add some tests. Are you opposed to keeping |
17cbc3d
to
643f952
Compare
// SetUnhandled indicates that the exception is an unhandled exception, i.e. | ||
// from a panic. | ||
func (m *Mechanism) SetUnhandled() { | ||
h := false | ||
m.Handled = &h | ||
} |
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.
I wanted to add some way to make it easier to set the Handled
field, since it's a bit annoying otherwise. AWS SDKs, for instance, have utility functions like func Bool(b bool) *bool
, but that felt like overkill here. Adding a corresponding SetHandled()
also felt like it wouldn't be useful. Happy to go either (or any other) way, though.
@cleptric updated with tests and a comment. |
643f952
to
8a5ac5a
Compare
Looks like the CI lint failure might be erroneous/in need of a re-run? Lints fine locally for me. |
8a5ac5a
to
c8777d8
Compare
Could you add a small snippet of how someone can use this feature to set an error as unhandled as well? |
Something beyond the tests? I can add it if you can point me to where you'd like me to add it. Documentation? Nothing else in |
Just as a comment here, I'll need to update our docs :) |
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.
Merging for now. @wyattanderson would still be cool if you could comment with a snippet of how you use it in your application. Thanks for the contribution!
This adds support for the exception
mechanism
field, based on therelay
schema of the field.Primarily, I'd like to be able to annotate exceptions with this data to make the UI more useful. For instance, we have a gRPC interceptor that recovers from
panic
, and I think it would be useful to set thehandled
bit tofalse
so that we get the additional UI embellishment (the red skull and "Unhandled" flag) when endpoints panic (versus returning an explicit status or error). This would likely be useful to other HTTP integrations as well.