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

FEAT: Add version of withSentryObservableEffect that has better inter… #3411

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

designedbyz
Copy link

📜 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.

💡 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.

💚 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

📝 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.

🔮 Next steps

needs a review from the Sentry team

@designedbyz
Copy link
Author

Question: how do I get a review for this?

Copy link
Member

@romtsn romtsn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hi @designedbyz sorry for the delays, the team was overloaded with other tasks.

I've taken a look at the PR and it makes a lot of sense to have this functionality in the SDK, so I'm just approving it. You'd also have to add a changelog entry to make CI green, you can take a look at the other entries to comply with the changelog format.

Ideally we'd also update Sentry docs in this section and mention how to handover the navigation listener between compose and the old view system, but we could take care of that ourselves 👍

Thanks for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants