Skip to content
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

Merged
merged 1 commit into from Feb 16, 2023

Conversation

wyattanderson
Copy link
Contributor

This adds support for the exception mechanism field, based on the relay 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 the handled bit to false 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.

Copy link
Member

@cleptric cleptric left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@codecov
Copy link

codecov bot commented Feb 2, 2023

Codecov Report

Base: 79.14% // Head: 79.16% // Increases project coverage by +0.01% 🎉

Coverage data is based on head (c8777d8) compared to base (f03b31a).
Patch coverage: 100.00% of modified lines in pull request are covered.

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              
Impacted Files Coverage Δ
interfaces.go 93.33% <100.00%> (+0.11%) ⬆️

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.
📢 Do you have feedback about the report comment? Let us know in this issue.

@wyattanderson
Copy link
Contributor Author

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/

Ah, yeah. That was an intentional omission because it didn't look like any of the meta fields would be relevant in a Go runtime, since it seems like most of the meta fields are specific to Apple, POSIX, or C syscalls. I'm not opposed to adding the relevant structs if you think they're worth adding, or I could leave a comment explaining the omission.

@cleptric
Copy link
Member

cleptric commented Feb 2, 2023

Ok, we can also start with type, handled, and data for now. Not sure if the others will add much value.
Some tests would be great as well 🙂

@wyattanderson
Copy link
Contributor Author

Ok, we can also start with type, handled, and data for now. Not sure if the others will add much value. Some tests would be great as well 🙂

I'll add some tests.

Are you opposed to keeping help_link and description? At least for my own use cases, I could see some value in being able to populate those (since they're rendered in the UI) for common internal errors so that we could point to internal documentation, etc.

Comment on lines +216 to +221
// SetUnhandled indicates that the exception is an unhandled exception, i.e.
// from a panic.
func (m *Mechanism) SetUnhandled() {
h := false
m.Handled = &h
}
Copy link
Contributor Author

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.

@wyattanderson
Copy link
Contributor Author

@cleptric updated with tests and a comment.

@wyattanderson
Copy link
Contributor Author

Looks like the CI lint failure might be erroneous/in need of a re-run? Lints fine locally for me.

@cleptric
Copy link
Member

Could you add a small snippet of how someone can use this feature to set an error as unhandled as well?

@wyattanderson
Copy link
Contributor Author

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 interfaces.go has anything like this, so I'm not quite sure what you're asking for. If you could be specific, that would be very helpful. I'd like to get this merged so we can avoid the need to maintain an internal fork of this package to take full advantage of Sentry's data model.

@cleptric
Copy link
Member

cleptric commented Feb 13, 2023

Just as a comment here, I'll need to update our docs :)

Copy link
Member

@cleptric cleptric left a 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!

@cleptric cleptric merged commit 2a20584 into getsentry:master Feb 16, 2023
@cleptric cleptric added this to the 0.19.0 milestone Mar 5, 2023
@wyattanderson wyattanderson deleted the wa/mechanism branch March 7, 2023 18:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants