Internal ExceptionMechanismException type in SentryEvent

[rkishchenko]

Spring Boot version 2.3.6. Sentry SDK version 3.2.0.

Implemented BeforeSendCallback to filter some exceptions from being forwarded to Sentry:

@Component
public class BeforeSendCallbackImpl implements BeforeSendCallback {

  @Override
  public SentryEvent execute(@NotNull SentryEvent event, @Nullable Object hint) {
    var eventThrowable = event.getThrowable();
    // ... some logic that deals with exception
    return event;
  }
}

The throwable object accessible from SentryEvent is ExceptionMechanismException wrapping the underlying exception thrown by the application. The ExceptionMechanismException is marked as @Internal though. There does not appear to be a way to access the underlying exception without peeking through internal class.

[bruno-garcia]

Maybe getThrowable in the event could already unwrap ExceptionMechanismException so it's not a leaky abstraction.

When doing ExceptionMechanismException it wasn't expected that this would have to be dealt with by consumers, it's a way to transport data with the exception through the pipeline in the SDK.

Either that or simply opening it up and documenting it is what I can think of

@marandaneto @maciejwalkowiak ?

[maciejwalkowiak]

I am in for having a method that returns unwrapped exception. We still need another one that returns the wrapped one for internal processing. The question is naming. What do you think about:

• getOriginThrowable - returns unwrapped exception
• getThrowable - returns wrapped exception

[marandaneto]

changing the return value of getThrowable would be a breaking change.

getThrowable keeps as it is, I'd fix the javadocs though.
getOriginThrowable sounds good, which does something like:

  public @Nullable Throwable getOriginThrowable() {
    if (throwable instanceof ExceptionMechanismException) {
      return ((ExceptionMechanismException)throwable).getThrowable();
    }
    return throwable;
  }

I don't think ExceptionMechanismException should be public, all the carried data from ExceptionMechanismException is already available thru the SentryEvent, so offering a method that returns the raw Throwable is useful.

[bruno-garcia]

I am in for having a method that returns unwrapped exception. We still need another one that returns the wrapped one for internal processing. The question is naming. What do you think about:

• getOriginThrowable - returns unwrapped exception
• getThrowable - returns wrapped exception

Can we please reconsider this?

I'm not sure this is a good public API. We should only be returning the "origin" throwable. The Wrapping throwable was a work around to transport data (mechanism info) into the exception and we can reconsider that or reconsider other things but just throwing another method like this, having a second method to get an exception sounds really dirty IMO.

See: getsentry/sentry-dart#311 (comment)

[bruno-garcia]

My vote is for simply unwrapping on getThrowable and adding a package private method that returns the wrapped throwable. This (package private) method the name doesn't matter much. For the users' point of view there's only getThrowable.

I'm happy to adding this "breaking" change on a minor bump even, since it is really only fixing a bug.

[marandaneto]

My vote is for simply unwrapping on getThrowable and adding a package private method that returns the wrapped throwable. This (package private) method the name doesn't matter much. For the users' point of view there's only getThrowable.

I'm happy to adding this "breaking" change on a minor bump even, since it is really only fixing a bug.

that's exactly the reason why we've decided at that time to create a new method, we'd need to end up with 2 public methods anyway.
what you say does make sense and it'd be the correct approach, it'd not solve the problem though (2 public APIs), doing what you say now would let us with 3 public methods doing the same thing, even worse.

getThrowable previously returning the captured Throwable or Mechanism, and now (doing the suggested approach), returning only the Throwable

getOriginThrowable would we remove and break people? probably not, so it'd still return the unwrapped Throwable, maybe we add a @deprecate (please use getThrowable)

getThrowableMechanism or any other name, that always returns the Mechanism if available, again, this must be public anyway, but internal, only used by the SDK itself.

I feel that we are just repeating the same trade-offs btw :) no new arguments to change our decision that has been taken before.
I'm in to do this change, but the chance to break people's beforeSend is high, otherwise, this would've never been an issue.

[bruno-garcia]

We should talk this on a call because I don't understand your points. I don't see where in my suggestions we have 3 method?

I'd like to keep getThrowable to only returns what exception that was used to create the event.

We need to add something package private that returns the wrapped, mechanism one.
I guess that's what you suggested as getThrowableMechanism, if we can't have this "internal" because Java, then @Internal and that's it. Now it's internal.

So we'd have 1 public method, getThrowable as originally intended. Anything else that's used internally for us requires no bikeshedding. getThrowableMechanism sounds good btw.