Sentry-java: 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: https://github.com/getsentry/sentry/issues/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)
        }
      }

[edenman]

fwiw, apparently the way the old Sentry-Android used to do this was it would look at the stacktrace included all wrapped exceptions and find the deepest element and set that as the culprit:

https://github.com/joshdholtz/Sentry-Android/blob/94c957798199e874545d71ced649c2e31a224e2d/sentry-android/src/main/java/com/joshdholtz/sentry/Sentry.java#L273
https://github.com/joshdholtz/Sentry-Android/blob/94c957798199e874545d71ced649c2e31a224e2d/sentry-android/src/main/java/com/joshdholtz/sentry/Sentry.java#L1002

[edenman]

(the more I think about this, the more I think withCulprit(SentryStackTraceElement frame) is the way to go, and I can just duplicate the old logic in my builderhelper)

[bretthoerner]

Can you update this if you find a way to? I'm still lost because,

old Sentry-Android used to do this was it would look at the stacktrace included all wrapped exceptions and find the deepest element and set that as the culprit

Is exactly what the builderHelper I pasted does, I thought?

[edenman]

i'm not 100% sure why this is the case, but apparently withCulprit(String) doesn't work if you're only including the exception message? withCulprit(StackTraceElement) includes a bunch more information, maybe that's why?

[edenman]

(where by "doesn't work" i mean that it doesn't select the full exception type/message using that message, it just shows the culprit message instead of the exception message)

[edenman]

I think there's also a pretty compelling argument to just copy this logic over from https://github.com/joshdholtz/Sentry-Android/blob/94c957798199e874545d71ced649c2e31a224e2d/sentry-android/src/main/java/com/joshdholtz/sentry/Sentry.java#L273 into AndroidSentryClientFactory

[bretthoerner]

Were you able to do all of this in your custom EventBuilderHelper? Can you post the final version that worked for your use case?

[edenman]

For now I'm just using the one from this comment: https://github.com/getsentry/sentry-java/issues/412#issuecomment-308542702

Want me to open a new issue for
1) copying over the "find the deepest stack element from my package name" code
or
2) exposing withCulprit(SentryStackTraceElement frame) so I can do it myself?

[bretthoerner]

What would withCulprit(SentryStackTraceElement frame) do? There is withCulprit(StackTraceElement frame)[0] and all it does is builds up and passes it to the String method, so can't this already be done in an EventBuilderHelper?

[0] https://github.com/getsentry/sentry-java/blob/master/sentry/src/main/java/io/sentry/event/EventBuilder.java#L227

[edenman]

Yeah sorry, I should've been more clear. I can certainly copy the logic of withCulprit(StackTraceElement frame) in my own EventBuilderHelper, I'd just rather not have that code live in my app: it makes things more brittle in case you guys change the expected format of the culprit field down the road.

[bretthoerner]

Ah, that makes sense. Can you review this? https://github.com/getsentry/sentry-java/pull/413

[bretthoerner]

Pushed 1.1.0 out to Maven Central just now.

[edenman]

Thanks! Unfortunately, there's still something screwy here:
https://sentry.io/recharge/consumer-android/issues/296067104/ (new)
https://sentry.io/recharge/consumer-android/issues/296072665/ (old)

Here's my new builder helper:

    sentryClient.addBuilderHelper { builder ->
      // The new sentry-android artifact assumes that the wrapper exception is the interesting one,
      // which is almost never the case on Android (especially with RxJava).  Walk from the bottom
      // of the exception cause tree and find the deepest point where an exception has some of our
      // code in the stacktrace.
      // Context: https://github.com/getsentry/sentry-java/issues/412
      val exception = builder.event.sentryInterfaces[ExceptionInterface.EXCEPTION_INTERFACE]
      if (exception is ExceptionInterface) {
        val culprit = exception.findCulprit()
        if (culprit != null) {
          builder.withCulprit(culprit)
        }
      }
    }

and then

  private fun ExceptionInterface.findCulprit(): SentryStackTraceElement? {
    for (exception in this.exceptions.descendingIterator()) {
      for (stackTraceElement in exception.stackTraceInterface.stackTrace) {
        if (stackTraceElement.module.startsWith(packageName)) {
          return stackTraceElement
        }
      }
    }
    return null
  }

https://sentry.io/recharge/consumer-android/issues/296067104/ looks like it has the correct culprit set (oddly, it's showing up in the transaction field on the sentry website) but the exception message still isn't pulled into the top level of the Sentry event :(

[bretthoerner]

There is definitely some confusion (on my/our end) related to culprit vs transaction. I'm going to figure that out.

BUT, can you detail exactly what you mean here?

but the exception message still isn't pulled into the top level of the Sentry event

I guess, what specifically about that issue do you want to be different in the UI?

[edenman]

Here's what I mean.
New:

Old:

Having the exception message there at the top level makes it _way_ easier to triage effectively. Especially on the list view:
New:

Old:

[bretthoerner]

So apparently culprits are now "transactions" and have a different meaning that what I expected: https://forum.sentry.io/t/replacing-culprits-with-transaction-names/105

It doesn't seem like setting the culprit is what we want here. I think I need to look into the backend option for deciding on sort order. 😞 That or we need something like what I expected culprit to be: "hey server, this is the frame I think caused it."

As a short term hack you might have to use the stacktrace reversal helper you pasted above.

[edenman]

Doesn't seem like a server change should be necessary here, since com.joshdholtz.sentry:sentry-android is doing the right thing. But I dunno. Anyway, I'm just gonna stick with com.joshdholtz.sentry:sentry-android for now since it does what I want and still works fine. Lemme know what you figure out!

[bretthoerner]

Sadly the other way is working for 99.99% of people so I need to investigate an option or something.

[Hazer]

@bretthoerner I having a lot of issues with this as well, every time there's a crash on Android UI, I receive a new RuntimeException, because every crash on UI get wrapped by the framework in a RuntimeException, but what I really want to see is its cause. So the issues list becomes a mess... There are tons of issues named RuntimeException with no helpful message until I open one by one to see the root cause.

In Android things like that are pretty common, maybe some kind of mapping for cases like this would be nice. Custom priority rules are in my opinion really a must have feature if possible...

[bretthoerner]

Thanks for reporting, it's good to hear from another Android user (as I'm not an actual Android dev, I just pretend to be one on TV).

Just to be clear, would the inner-most exception be the error you'd like to see (as reported above)? Or is it more complex than that?

[Hazer]

@bretthoerner Actually, I am not really sure... For me, most exceptions come from 2 sources, UI crashes with RuntimeException or manually sent crashes with Sentry.capture(exception), most manually captured exceptions look perfect. But uncaught exceptions from UI or RxJava (we are not using it extensively so at least for me not really a trouble) can get really ugly.

Maybe some kind of "transformation" for uncaught exceptions that we can plugin some rules, but I am really with no idea how to do it right.

I really don't know the best approach here...

What are your thoughts on it?

[bretthoerner]

Do you mind linking me to one on Sentry that you don't think looks right?

[Hazer]

@bretthoerner I will ask here, but I don't have the admin rights here yet so actually I believe I can't. But I can share some screenshots:

Here are 3 issues, one was ClassCastException, another was IllegalStateException and the third was yet another unrelated issue on the same Activity class. But from the outside, there's no way to know what really is the issue.

ClassCastException issue in details, now we can see what really is it:

Its not that it don't looks right, it is in fact a RuntimeException, but the 'culprit priority' looks messy and makes user experience bad in the end.

[bretthoerner]

@Hazer You can actually just paste me the event URL since I work at Sentry -- I can see them all. :)

Thanks for the info though, this is all helpful.

[Hazer]

@bretthoerner Oh, right! haha

Here's one:
https://sentry.io/plugapps/zuk-android-dev/issues/308253723/

[bretthoerner]

Hmm,

The issue you were looking for was not found.

But I can see others in that project, odd?

[Hazer]

@bretthoerner Oh, sorry, another developer took to do a cleanup on it, as it is our internal dev tracker we clean it very often:

Here it is:
https://sentry.io/plugapps/zuk-android-dev/issues/310926476/

This ^ one will stay till we close it

[bretthoerner]

Thanks. So at a glance I'd say you want the reversed order like @edenman.

[edenman]

I'm still not convinced that reversing the order is the right move here, fwiw. The custom culprit-setting makes way more sense to me, especially since that doesn't break issue grouping with previous versions. But even ignoring the previous-version-grouping issue, reversing the stacktrace just feels wrong...stacks make sense to display top-down, but (at least for most Android apps) it makes sense to display and group on the message/type of the root exception.

[bretthoerner]

I'm still not convinced that reversing the order is the right move here

But that's exactly what the old Sentry-Android does, which I thought does what you wanted?

especially since that doesn't break issue grouping with previous versions

Maybe I need to re-re-re-remember something from the thread, but if your exceptions are using the "wrong" (outer?) exception as their culprit now, and you change that (via any means) then they aren't going to group properly.

stacks make sense to display top-down

To be clear, I don't mean I want to actually re-order the traces, or even display them differently. What I meant was that the "other side" (inner) exception should be "chosen" as the culprit. Right now we do traces[0] and it seems like y'all want traces[-1] to be the message/name/group? (Just to repeat what I said before, though: this isn't something we can globally change for Sentry, it's going to need to be an option or something)

[edenman]

Oh yeah, sorry, I had to go re-read some of this thread myself. I forgot about the whole culprit/transaction business....com.joshdholtz.sentry doesn't just use traces[-1], it actually walks down the exception chain and finds the topmost Exception that includes the app's package name. Which is great, because if I choose to wrap exceptions, it'll give me the wrapper exception, but if it's Android wrapping everything in a RuntimeException, I'll get the one below that.

I just wrote ^ and realized it's really dense, so a couple examples, given that my app's package name is com.example.foo, and I have these custom exception classes: com.example.foo.TheActualCauseException, com.example.foo.MyWrapperException.

1. The Android-wrapped-me case: RuntimeException -> com.example.foo.TheActualCauseException. This should log TheActualCauseException to Sentry.
2. The I-wrapped-it-myself case: RuntimeException -> com.example.foo.MyWrapperException ->com.example.foo.TheActualCauseException. This should log MyWrapperException to Sentry (presumably because I've included additional information that wasn't part of TheActualCauseException).

[bretthoerner]

it actually walks down the exception chain and finds the topmost Exception that includes the app's package name

Oh interesting, I think I missed that. Thanks much for the examples.

OK, with the random stuff I had to get to and 1.3.0 out the door I will give this one some real time next week.

[bretthoerner]

OK, a few things:

I verified that PHP and Python 3 (other languages with chained exceptions) actually send the exceptions as documented, like sentry-java does. And in those languages the "wrapper exception" is displayed, which is suboptimal (depending on the project).

I am confident that the difference in stacktrace ordering (between this library and the deprecated Sentry-Android) is the most important thing to "fix."

The reason the culprit logic isn't important (for a proper "fix") is that the culprit should rarely be set in the client at all. To explain: the same logic is already applied on the server side, you just need to configure your application packages. This is available via the stacktrace.app.packages option, which I will try to call out better in the documentation. (I tested this out with an example project and the location displayed is in my application even though my exception originates in an external library.)

In addition, we can probably automatically set the packages on Android by using the same logic as Sentry-Android:

PackageInfo info = context.getPackageManager().getPackageInfo(context.getPackageName(), 0);
String packageName = info.packageName

I'll make a PR for that.

Now, as for the stacktrace ordering. I still want to make this a server side option. We have progress on the server-side to allow for more complex event grouping logic and I'll make this one of the options on that page.

In the meantime, if this is a burning need, the best solution is probably to send a reversed stacktrace (and set your app packages), which can be achieved by the (Java-fied) example that Eric pasted above. The problem is I'm not sure what this may break now or in the future (and it will certainly break grouping when/if you disable it). But... it's a temporary hack while we figure out the server side.

Sentry.getStoredClient().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();
                        Stack<SentryException> stack = new Stack<SentryException>();

                        while (!exceptions.isEmpty()) {
                            stack.push(exceptions.removeFirst());
                        }

                        for (SentryException exception : stack) {
                            exceptions.push(exception);
                        }
                    }
                }
            });

[marandaneto]

Closing the issue as a part of large repository cleanup, due to it being inactive and/or outdated. There's a large chance that this issue has been solved since.
Please do not hesitate to ping me if it is still relevant, and I will happily reopen and work on it.
Cheers!