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

Add an official mean of identifying traceback exceptions #1953

Closed
simonbasle opened this issue Nov 12, 2019 · 6 comments
Closed

Add an official mean of identifying traceback exceptions #1953

simonbasle opened this issue Nov 12, 2019 · 6 comments
Labels
type/enhancement A general enhancement

Comments

@simonbasle
Copy link
Member

simonbasle commented Nov 12, 2019

Motivation

There is no good way to detect if a Throwable is a traceback, which could be useful in cases where eg. such a traceback is added to a CompositeException.

Desired solution

Add an official API to check if a Throwable is a traceback.

Considered alternatives

The OnAssemblyException being package-private means that instanceof cannot be used. Relying on getClass().getCanonicalName() from within user code is an alternative, but less than ideal compared to doing that from core codebase.

Another alternative would be to have a parameter to Exceptions.unwrap to chose wether or not to filter out tracebacks, or having it always filter out tracebacks.

Additional context

checkpoint operator adds the traceback as a suppressed exception. CompositeException uses the same mechanism to gather its composites, so the traceback appears in the list when unwrapping the composite.

@simonbasle simonbasle added this to the 3.2.13.RELEASE milestone Nov 12, 2019
simonbasle added a commit that referenced this issue Nov 12, 2019
@simonbasle simonbasle changed the title Add an official mean of identifying backtrace exceptions Add an official mean of identifying traceback exceptions Nov 12, 2019
@rstoyanchev
Copy link
Contributor

Another alternative would be to have a parameter to Exceptions.unwrap to chose whether or not to filter out backtraces, or having it always filter out backtraces.

Given that composite exceptions overlap with checkpoints in the sense that both rely on suppressed exceptions, I think there should be a first-class way to separate actual errors from checkpoint ones, for exception handling purposes (e.g. spring-projects/spring-framework#23827) as opposed to extra information for debugging purposes. I'd argue that unwrap should do that by default but having an overloaded method (or an argument) would also make this visible enough so it does not come as a surprise at runtime.

@simonbasle
Copy link
Member Author

simonbasle commented Nov 12, 2019

Additional TODO:

  • we need to at least mention composites, tracebacks and how they might seem to overlap in the reference guide.

@simonbasle
Copy link
Member Author

@rstoyanchev I wonder if the suppressed exception implementation of CompositeException should be revisited. I also think that if the checkpoint is there, then it probably has some value and should not implicitly be removed by unwrap, else that value is lost.
So if it needs to be removed (which I highly doubt even in the repro case of spring-projects/spring-framework#23827), it should at least be a very explicit choice by the end user.

I'm not sure how to best communicate about that in the javadoc, seems that attempting to mention isTraceback in every method that can produce a composite is a bit overkill (since there are more cases than just the obvious *DelayError ones). Maybe favor mentioning it in checkpoint() itself, instead? And of course on the Exceptions.unwrapMultiple and Exceptions.multiple methods)...

@rstoyanchev
Copy link
Contributor

What do you have in mind for revisiting the CompositeException implementation?

The checkpoint is there for a reason but I want to make a distinction between value from the perspective of exception handling vs for value for debugging/reporting purposes. If I make calls to remote services that return errors to the effect of some entity does not exist, or the input incomplete/invalid, I may choose to handle those errors in some way like returning a specific status, or perhaps notifying something/someone. Checkpoint exceptions on the other hand have no role to play in such handling. The reporter under the above issue makes it clear they do not want to see the checkpoint exceptions in the @ExceptionHandler method.

In addition, the information value of the checkpoint exception is more clearly suited for server side logging/reporting. It is arguably not something that should leak out in the body of an error response. It likely contains information that was not meant for external consumption.

As for the Javadoc it might be more natural to mention how to extract the composite exception (i.e. a pointer to Exception.unwrap which in turn could have something visible, and it would make the explanation more consolidated.

@simonbasle
Copy link
Member Author

@rstoyanchev maybe something more like a List attribute on the composite. But then it becomes hard for the (package-private) CompositeException to maskerade as a generic exception while showing the components to a general purpose Throwable consumer. So probably not a terribly interesting idea.

simonbasle added a commit that referenced this issue Nov 13, 2019
@simonbasle
Copy link
Member Author

@rstoyanchev I added unwrapMultipleExcluding, which takes an excluding filter Predicate (typically in our case Exceptions.unwrapMultipleExcluding(composite, Exceptions::isTraceback)). wdyt?

simonbasle added a commit that referenced this issue Nov 22, 2019
simonbasle added a commit that referenced this issue Nov 25, 2019
Conflicts:
 - debugging.adoc had typo fixes and new chapters
 - FluxOnAssemblyTest had wrong line asserted in stacktrace as result of
 changes on both branches
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

2 participants