sentry-java icon indicating copy to clipboard operation
sentry-java copied to clipboard

FEAT: Add version of withSentryObservableEffect that has better inter…

Open designedbyz opened this issue 1 year ago • 1 comments

:scroll: Description

This PR adds a version of NavHostController.withSentryObservableEffect that accepts a SentryNavigationListener as a parameter to support better navigation tracing in apps that make use of both Fragments and Compose.

:bulb: Motivation and Context

A challenge with the existing Navigation and Compose Navigation integrations is that, in the testing I've done, they seem to clobber each other. My guess as to why is because each listener will try to register itself as the integration, meaning that you only get one. Additionally, even if the instances didn't clobber each other, navigation history will end up separated between instances, as the Compose version will only have the nav destinations for Compose, whereas the fragment version will only have the destinations for fragments. This makes it hard to put traces back together for apps that make use of both fragments and compose navigation and navigate between the two.

Thankfully, the fix is pretty simple and is implemented here. I added a new version of withSentryObservableEffect that accepts a SentryNavigationListener as a parameter, which allows consumers of the SDK to pass the same version of the SentryNavigationListener to withSentryObservableEffectwhen using the compose integration as they do tonavController.addOnDestinationChangedListener` when adding the integration for fragment navigation tracing.

There is one possible downside to this approach. Specifically, the trace origin will show up as navigation for compose navigation instead of compose, but I personally don't see this as an issue.

:green_heart: How did you test it?

Tested locally; and we'll likely ship an internal version of this to prod at Maven before this is merged

:pencil: Checklist

  • [ ] I reviewed the submitted code.
  • [ ] I added tests to verify the changes.
  • [ ] No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
  • [ ] I updated the docs if needed.
  • [ ] Review from the native team if needed.
  • [ ] No breaking change or entry added to the changelog.
  • [ ] No breaking change for hybrid SDKs or communicated to hybrid SDKs.

:crystal_ball: Next steps

needs a review from the Sentry team

designedbyz avatar May 07 '24 16:05 designedbyz

Question: how do I get a review for this?

designedbyz avatar May 21 '24 14:05 designedbyz

@designedbyz sorry for abandoning this, I've reopened #4143 as I was unable to push to this branch and update it for some reason. We'll merge it and ship in version 8.2.0. Thanks for your contribution!

romtsn avatar Feb 06 '25 23:02 romtsn