feat: create Exception from tracing event by seanpianka · Pull Request #359 · getsentry/sentry-rust

seanpianka · 2021-09-10T06:12:34Z

This PR relates to my message in #180

This change will build an exception differently than event. It will look
for two hardcoded keys in the "extras" of an event:

"error_kind" - this is optional and defaults to "Unknown"
"error" - this is optional and defaults to the event's message

I am open to ideas for improving this! This PR simply gets exception
handling off the ground, but makes arbitrary assumptions about how the
tracing event will communicate the error information.

flub

I like the general approach to this, thanks for starting to work on it. Some high level questions I have though:

I would kind of expect to be able to pass an std::error::Error type in the error field of an trancing span/event and for the sentry integration to extract the type and value and backtrace just like it would do for directly capturing such error types. Can this be done?
I think the crate's doc comment should clearly document that the integration treats those tracing fields specially and how to use them to create sentry errors.
I'm not entirely sure if I'm reading this code correctly, but does this try to create an exception for any tracing event? Should it only do this for error-level events maybe?

flub · 2021-09-10T09:19:49Z

+    let culprit = match (event.metadata().file(), event.metadata().line()) {
+        (Some(f), Some(l)) => Some(format!("{}L{}", f, l)),
+        _ => None,
+    };


I've been trying to find out what this field is for and so far the SDK folks seem to be telling me that this is an internal field that's set during processing and also that it is deprecated. So we shouldn't set it here.
(see also getsentry/relay#1082)

Removed!

I tried to shoehorn the filename and line number of the span into the exception somehow, but perhaps this information will be included elsewhere? I am unsure about this, since there's no stacktrace available.

Yes, it would be nice to have this somewhere. But indeed without a stacktrace that seems tricky. I'm not very familiar with the event data format, https://develop.sentry.dev/sdk/event-payloads/#other-interfaces might have info on where else could be suitable.

seanpianka · 2021-09-10T18:26:25Z

I would kind of expect to be able to pass an std::error::Error type in the error field of an trancing span/event and for the sentry integration to extract the type and value and backtrace just like it would do for directly capturing such error types. Can this be done?

Yes, retrieving the error from the event is possible. The Visit implementation doesn't include the record_error method currently, so implementing that should give the layer access to this value.

However, Error::backtrace is unstable, and the error is supplied as a dyn Error, so there's no straightforward way to get the name of the concrete type (without nightly). But, the value is easy as .to_string(), and we can emulate a backtrace with Error::source.

I think the crate's doc comment should clearly document that the integration treats those tracing fields specially and how to use them to create sentry errors.

Agreed. My choice of field names is arbitrary, and I'd like to align with whichever fields #[tracing::instrument(err)] uses for errors or ignore the fields entirely if there's an dyn Error available.

I'm not entirely sure if I'm reading this code correctly, but does this try to create an exception for any tracing event? Should it only do this for error-level events maybe?

Perhaps I'm misunderstanding the on_event method in the layer, but I don't think it's creating an exception for any tracing event, only those that are ERROR level or above.

seanpianka · 2021-09-12T21:02:04Z

So, I've re-worked the record_error impl using @leops' fork as an example. Now, issues reported in Sentry have their own exceptions with relevant type, title/message, and a (semi-relevant) backtrace.

The "ty" of the exception is still not great. For an error event, the type is Event path/to/file.rs:LineNumber. It's better than "Unknown", but still not great. If the underlying line of code was to move, the line number would be different and then Sentry would no longer group events for the same underlying issue.

Also, I do not see the "extras" in the issue (i.e. the latest event) in Sentry. In my testing, when annotating a function with #[tracing::instrument(err, fields(data = 1))], the field data = 1 does not appear on the event display in Sentry.

It seems some more debugging is needed 😅

flub

Apologies for the delay in getting back!

flub · 2021-09-20T10:28:56Z

+        let value = visitor
+            .json_values
+            .remove(DEFAULT_ERROR_VALUE_KEY)
+            .map(|v| v.as_str().map(|s| s.to_owned()))
+            .flatten()
+            .or_else(|| message.clone());


Is this whole thing still needed now that you discovered record_error?

I left this branch as a fallback in the case that the event does not have any exceptions. This would at least create a single exception using what data is available from either the error or message field (in that order).

Thoughts on handling this explicitly, versus assuming that at least one exception will always be present?

are sentry events required to always include an exception? We could document to use some_field_name=anyhow::Error::msg("oops") instead of error="oops"` if you need an exception? I fear that leaving in a special field name could be surprising at times?

Swatinem

lgtm, but it would be nice to have a test for this.

Swatinem · 2021-10-04T07:26:47Z

+sentry-core = { version = "0.23.0", path = "../sentry-core", features = ["client"] }
+sentry-backtrace = { version = "0.23.0", path = "../sentry-backtrace" }


why was this change necessary? I don’t see these imports being used? Please revert this in that case.

Sorry for the delay. This dependency is used in a particular scenario where the event visitor parsed no events from an event handled as an exception: source.

What are your thoughts on handling this scenario where the event visitor parsed no events for an exception? It seems like a particularly unlikely scenario, and handling it in the current way requiring pulling in these two extra crates.

seanpianka · 2021-10-27T17:51:50Z

lgtm, but it would be nice to have a test for this.

Which parts did you have in mind, @Swatinem? I suppose it makes sense to test the event_from_event and exception_from_event methods for the expected results, but is there anything else you think should be covered?

seanpianka · 2021-10-27T20:01:24Z

Rebased again and force-pushed the changes.

Swatinem · 2021-11-03T08:36:32Z

I think a simple test where you supply some kind of Error type, and have that be captured correctly by sentry would be nice.

This change will build an exception differently than event. It will look for two hardcoded keys in the "extras" of an event: 1. "error_kind" - this is optional and defaults to "Unknown" 2. "error" - this is optional and defaults to the event's message Signed-off-by: Sean Pianka <pianka@eml.cc>

Signed-off-by: Sean Pianka <pianka@eml.cc>

refactor: reduce public api surface refactor: rename visitor type refactor: re-use parent context/transaction handling chore: reorder trait impl to match trait Signed-off-by: Sean Pianka <pianka@eml.cc>

Signed-off-by: Sean Pianka <pianka@eml.cc>

Swatinem · 2022-01-03T11:38:03Z

@seanpianka 👋🏻 what is still needed to push this over the finish line?

I might be a bit tempted to just merge this as is, as we are currently adding manual performance monitoring support and will then probably overhaul the tracing integration completely.

Swatinem · 2022-01-20T16:18:34Z

I went ahead and merged the code with the latest upstream changes in #412

flub reviewed Sep 10, 2021

View reviewed changes

seanpianka changed the title ~~feat: create Exception from keys in event extras~~ feat: create Exception from tracing event Sep 10, 2021

seanpianka force-pushed the master branch 2 times, most recently from 18ee3ed to fede6f2 Compare September 12, 2021 19:43

seanpianka marked this pull request as draft September 14, 2021 01:51

flub reviewed Sep 20, 2021

View reviewed changes

seanpianka force-pushed the master branch from ba2ebd3 to 72ce55f Compare October 3, 2021 18:06

Swatinem reviewed Oct 4, 2021

View reviewed changes

seanpianka force-pushed the master branch from 4df1fef to ae0ab1b Compare October 26, 2021 22:48

Swatinem approved these changes Oct 27, 2021

View reviewed changes

seanpianka force-pushed the master branch from 153c246 to a08c9b0 Compare October 27, 2021 17:48

seanpianka added 8 commits November 27, 2021 18:27

fix: add missing "client" sentry-core feature

1651983

Signed-off-by: Sean Pianka <pianka@eml.cc>

fix: don't explicitly set culprit

afb30d3

Signed-off-by: Sean Pianka <pianka@eml.cc>

feat: add record_error impl for use in exception

1613af3

Signed-off-by: Sean Pianka <pianka@eml.cc>

feat: use sentry_backtrace, re-use sentry_core Exception parsing

06d4af2

refactor: reduce public api surface refactor: rename visitor type refactor: re-use parent context/transaction handling chore: reorder trait impl to match trait Signed-off-by: Sean Pianka <pianka@eml.cc>

fix: only initialize values when there's no exceptions

b1c3150

Signed-off-by: Sean Pianka <pianka@eml.cc>

fix: use all fields available for a trace in its context

e58722c

Signed-off-by: Sean Pianka <pianka@eml.cc>

fix(sentry-tracing): use FieldVisitor after rebase

bd3dd78

Signed-off-by: Sean Pianka <pianka@eml.cc>

seanpianka force-pushed the master branch from a08c9b0 to bd3dd78 Compare November 27, 2021 23:31

seanpianka mentioned this pull request Dec 22, 2021

tracing integration #180

Closed

Merge branch 'master' into master

0f8f4b5

Swatinem mentioned this pull request Jan 20, 2022

feat: Support errors in tracing integration (NATIVE-310) #412

Merged

Swatinem closed this Jan 20, 2022

		sentry-core = { version = "0.23.0", path = "../sentry-core", features = ["client"] }
		sentry-backtrace = { version = "0.23.0", path = "../sentry-backtrace" }

Uh oh!

Conversation

seanpianka commented Sep 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flub left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seanpianka commented Sep 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

seanpianka commented Sep 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flub left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Swatinem left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seanpianka commented Oct 27, 2021

Uh oh!

seanpianka commented Oct 27, 2021

Uh oh!

Swatinem commented Nov 3, 2021

Uh oh!

Swatinem commented Jan 3, 2022

Uh oh!

Swatinem commented Jan 20, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

seanpianka commented Sep 10, 2021 •

edited

Loading

seanpianka commented Sep 10, 2021 •

edited

Loading

seanpianka commented Sep 12, 2021 •

edited

Loading