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

Feature request: report suppressed exceptions #542

Closed
fprochazka opened this issue Dec 17, 2017 · 16 comments
Closed

Feature request: report suppressed exceptions #542

fprochazka opened this issue Dec 17, 2017 · 16 comments
Labels

Comments

@fprochazka
Copy link

fprochazka commented Dec 17, 2017

All exceptions in Throwable.getSuppressed() should be reported and visible in Sentry.
They might contain important information, that is not in the "main" exception.

Does Sentry itself have a mechanism for this? Or do they have to be reported separately?

@bretthoerner
Copy link

This seems like a good idea.

At first glance I don't think we should add them to the main event stacktrace as that may be confusing and would affect issue grouping. I'm not even sure where in the existing exception chain they would be placed?

So I see a few options and I'd love some feedback:

  • I'm wrong above and the suppressed exceptions should somehow be combined into the main exception.
  • We could add some (basic, class and line number?) information to the event's extra data.
  • We could add some (basic, class and line number?) breadcrumbs.
  • They could go somewhere else on the event that already exists?
  • We'd need a whole new place to store them on the event. (This one seems unlikely but maybe I'm wrong here too)

@fprochazka
Copy link
Author

fprochazka commented Dec 18, 2017

The main use-case for suppressed exceptions (if I'm not mistaken), is when you're processing some calls, that might produce a lot of errors, but you want to aggregate those errors and not kill the process itself. And at the end, you might want to throw a new exception, informing that part of the process failed and attach all the suppresed exceptions using Throwable.addSuppressed().

I'm wrong above and the suppressed exceptions should somehow be combined into the main exception.

IMHO they can be considered "cause" of the main exception, but because you cannot have an array of causes, they're "suppressed".
IMHO there should be a separate "view" with informations about suppressed exceptions, they should not be rendered as part of stacktrace of the main exception.

We could add some (basic, class and line number?) information to the event's extra data.

"extra" makes sense as a temporary solution - because knowning about them is more important than having them printed nicely.
It would be great, if Sentry itself could somehow render suppressed as separate exception blocks
That would be awesome, but I suppose this request is unique to java and I'm not sure whether other languages have similar needs.

We could add some (basic, class and line number?) breadcrumbs.
They could go somewhere else on the event that already exists?

I don't have opinion on this

We'd need a whole new place to store them on the event. (This one seems unlikely but maybe I'm wrong here too)

Yes, exactly! just like there is extra, there should be suppressed (or some other more generic name?) that Sentry itself could render properly.

@bretthoerner
Copy link

Yeah, adding a new section in Sentry will (AFAIK) be a much larger process. In the short term I can definitely add them to extra or breadcrumbs on the SDK side. Maybe with an option to enable/disable if people really care.

I'll try to take a look at this soon, or as usual we're open to PRs. Thanks for the idea!

@vaibhavagnihotri
Copy link

We're running through the same problem. We're using project reactor with webflux and for some Timeout/Null Pointer Exceptions. We don't get any stack trace but supressed exception contains the detailed info of where the error originated. Currently I'm manually adding it as a breadcrumb. Please let me know if there are any out of the box solution to serve the purpose.

io.sentry - 1.7.30
spring-boot-starter-webflux - 2.3.3

@marandaneto
Copy link
Contributor

@vaibhavagnihotri you could create a chained exception instead of adding breadcrumbs, Sentry already supports chained exceptions thru the cause ctor https://docs.oracle.com/javase/7/docs/api/java/lang/Exception.html#Exception(java.lang.Throwable)

@fprochazka
Copy link
Author

fprochazka commented Mar 12, 2021

@marandaneto yes, but cause and suppressed have a different meaning.

And you can have always only one cause for each throwable, but each throwable can have a list of suppressed throwables.

@marandaneto
Copy link
Contributor

yes, I meant, it's just a workaround, these exceptions would come thru and would be shown and have more info than just breadcrumbs.

@vaibhavagnihotri
Copy link

@marandaneto Some times cause remains null but suppressed exception contains the assembly trace from producer.

Exception class - java.util.concurrent.TimeoutException

Exception.getMessage() - Did not observe any item or terminal signal within 1ms in 'Mono.flatMap ⇢ at org.springframework.web.reactive.function.client.DefaultWebClient$DefaultResponseSpec.bodyToMono(DefaultWebClient.java:471)' (and no fallback has been configured)

Exception.getCause() - null

Exception.getSuppressed()[0].getMessage() - Assembly trace from producer [reactor.core.publisher.MonoLift] : reactor.core.publisher.Mono.timeout ......Trace showing error origin

@hanswesterbeek
Copy link

I encountered this today as well.

I think more users will be affected by this as they migrate away from RestTemplate to WebClient.

@maciejwalkowiak
Copy link
Contributor

@marandaneto do you have any suggestions on how suppressed exceptions should be attached to SentryEvent?

One idea is to add them to exception queue in SentryExceptionFactory#extractExceptionQueue(..) with a mechanism of type "suppressed". What do you think?

@marandaneto
Copy link
Contributor

yep, that's what I had on my mind too

@marandaneto
Copy link
Contributor

be aware that we need an abstraction for Throwable.getSuppressed() because it does not work for all Android versions, only from API 19 on https://developer.android.com/reference/java/lang/Throwable.html#getSuppressed()
so we have to check at runtime the OS version and either set the abstraction that returns nothing or not

@bruno-garcia
Copy link
Member

bruno-garcia commented Sep 27, 2021

Since this will require a lot of code to abstract away what's needed to work around the lack of API on Android, we decided to add this to the core sentry package once sentry-android-core min API level goes to 19 or higher.

For reference, API 16 (which we require for NDK) to 19 isn't a huge jump in terms of Android devices covered and we might be able to do that not too far in the future.

@maciejwalkowiak
Copy link
Contributor

Until we are able to support suppressed exceptions in core, you can write custom EventProcessor that modifies reported exceptions to include not only causes but also suppressed ones. Unfortunately, some code has to be copy pasted, as it is package-private in Sentry SDK codebase. All needed classes are in this gist.

  • SentryStackTraceFactory.java- is a 1:1 copy from Sentry Java SDK codebase
  • SuppressedAwareSentryExceptionFactory.java - is a modified SentryExceptionFactory to extract suppressed exceptions
  • SuppressedAwareEventProcessor.java - event processor that must be added to SentryOptions (done automatically in Spring Boot integration).

@romanbsd
Copy link

romanbsd commented Jan 2, 2024

Hope to see this implemented. I somehow was under the impression that it was supported, until I noticed that it isn't.

@szokeasaurusrex
Copy link
Member

Fixed by #3396

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Status: Done
Development

No branches or pull requests