Exception cause hierarchy seems to be reversed on Android

[edenman]

1. https://sentry.io/recharge/consumer-android/issues/293914847/ (using com.joshdholtz.sentry:sentry-android:1.6.0)
2. https://sentry.io/recharge/consumer-android/issues/293913146/ (using io.sentry:sentry-android:1.0.0)

Note that the message in #2 is a red herring, it's the wrapping exception's message which is almost never the useful one. #1 is correctly showing the root cause exception type/message.

[bretthoerner]

Hmm, I do see what you mean. The problem is I can see people wanting it the current way (which is how it has been forever in sentry-java) too. Changing this would break grouping for all existing events with chained exceptions...

I'm going to see what we do in other SDKs.

[bretthoerner]

The Python SDK has the same behavior (as sentry-java, or number 2 above) with chained exceptions, for what it's worth.

I can make it an option at the SDK level, I think that would cover your needs @edenman?

[edenman]

An option works for me, thanks @bretthoerner!

[bretthoerner]

So this is a little more complex than I expected. We don't want to change the order of the stacktraces because that is well defined and the server expects it a certain way.

In addition, the server is in charge of setting the "real" message.

In short, we need a new per-project setting to do this the correct way. Discussion/tracking is being moved here, I'll try to check out an implementation soon: getsentry/sentry#5565

[edenman]

Ok. I'm a little confused by "that is well defined and the server expects it a certain way" because obviously the old sentry-android library was sending up in the reverse order and the server accepted it just fine.

If we make this a server setting, that means it'll break exception tracking for all my existing sentry events, which is unfortunate (but not the end of the world)

[bretthoerner]

The server accepts it because it can't know the correct order, but the order was a bug in Sentry-Android and not something we want to spread further.

From the SDK docs: "The list of frames should be ordered by the oldest call first."

We need to maintain sanity in the order across all languages/events so that sane decisions can be made server side. It seems it was just luck that Sentry-Android did it the "wrong" way and you happen to be in the group of users that prefers it that way (you're the first we've ever heard this, fwiw). As noted in the Sentry issue preferring the outer most exception is just as valid, so it's lose/lose without an option.

And ya, sorry it'll break existing stuff. :(

[edenman]

Right, but the old Sentry-Android had plenty of users, and none of them ever complained about it either, right? Seems unlikely to me that it was a "bug" in that library.

If there's a try/catch/wrap-and-rethrow, I guess I can't imagine a scenario where I'd care more about the wrapping exception. And when you're using RxJava, just about every exception gets wrapped a half-dozen times before it actually gets to the uncaught exception handler :/

[bretthoerner]

I totally understand, it's a difference between libraries like RxJava and others that wrap in more helpful exceptions and we can't make a single decision server-side. We definitely don't want to have each SDK send events in different formats either, though.

Can you try this out, though? It may cover your needs without reordering the stacktraces.

String sentryDsn = "https://publicKey:secretKey@host:port/1?options";
Context ctx = this.getApplicationContext();
SentryClient client = Sentry.init(sentryDsn, new AndroidSentryClientFactory(ctx));

client.addBuilderHelper(new EventBuilderHelper() {
    public void helpBuildingEvent(EventBuilder eventBuilder) {
        Map<String, SentryInterface> eventInterfaces = eventBuilder.getEvent().getSentryInterfaces();
        SentryInterface exceptionInterface = eventInterfaces.get(ExceptionInterface.EXCEPTION_INTERFACE);
        if (exceptionInterface != null) {
            Deque<SentryException> exceptions = ((ExceptionInterface) exceptionInterface).getExceptions();
            SentryException sentryException = exceptions.peekLast();
            if (sentryException != null) {
                eventBuilder.withCulprit(sentryException.getExceptionMessage());
            }
        }
    }
});

[bretthoerner]

Please let me know if that works, by the way. If it does, that may be a valid option (instead of reordering the stacktraces) that I didn't remember until just now. 😞

[edenman]

Nope, that doesn't work. It seems to set the "transaction" field: https://sentry.io/recharge/consumer-android/issues/293913146/

However, I can get what I want by just reversing the exception queue myself:
https://sentry.io/recharge/consumer-android/issues/294580114/

If anybody else comes upon this, here's how I'm doing it:

    val sentryClient = Sentry.init(dsn, AndroidSentryClientFactory(this))
    sentryClient.addBuilderHelper { builder ->
      val exception = builder.event.sentryInterfaces[ExceptionInterface.EXCEPTION_INTERFACE]
      if (exception is ExceptionInterface) {
        val stack = Stack<SentryException>()
        while (exception.exceptions.isNotEmpty()) {
          stack.push(exception.exceptions.removeFirst())
        }
        for (sentryException in stack) {
          exception.exceptions.push(sentryException)
        }
      }
    }

[bretthoerner]

I guess I'm confused still, when I look at your project listing for the one you linked I see the "OMG TESTING" stuff. Was your goal not to set the message value to the first exception?

Re-ordering the stack thing in your own project is fine, but I can't guarantee it won't break other things.

[edenman]

Weird. I'm almost certain that wasn't showing up when I first viewed 293913146.

That said, it's still showing the exception type as IllegalStateException which is wrong, so I think I'll stick with reversing the exception stack?

Alternatively, if you want to expose a withCulprit(SentryStackTraceElement frame) then I could do something like:

      val exception = builder.event.sentryInterfaces[ExceptionInterface.EXCEPTION_INTERFACE]
      if (exception is ExceptionInterface) {
        val rootCauseStack = exception.exceptions.peekLast()?.stackTraceInterface
        if (rootCauseStack != null) {
          val topStackElement: SentryStackTraceElement = rootCauseStack.stackTrace[0]
          builder.withCulprit(topStackElement)
        }
      }