Skip to content

fix(init): get Application from Context to register integrations #4355

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

krystofwoldrich
Copy link
Member

@krystofwoldrich krystofwoldrich commented Apr 24, 2025

📜 Description

React Native Native Modules receive ReactContext which is not instance of Application class.

This fixes missing integrations in RN applications and other cases when supplied context is not directly Application class.

💚 How did you test it?

RN Sample app, added unit tests

📝 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

@krystofwoldrich krystofwoldrich changed the title fix(init): Try to get Application from Context to register App Lifecycle Integrations fix(init): get Application from Context to register integrations Apr 25, 2025
Copy link
Contributor

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 445.40 ms 458.86 ms 13.46 ms
Size 1.70 MiB 2.36 MiB 677.25 KiB

@krystofwoldrich krystofwoldrich force-pushed the kw-fix-not-registering-app-integrations-on-context-wrappers branch from a03fc8d to 9d55fdf Compare April 28, 2025 14:37
@krystofwoldrich krystofwoldrich changed the base branch from 7.x.x to main April 28, 2025 14:37
@krystofwoldrich krystofwoldrich marked this pull request as ready for review April 28, 2025 14:38
@@ -16,7 +16,8 @@
- Set `-Dio.opentelemetry.context.contextStorageProvider=io.sentry.opentelemetry.SentryContextStorageProvider` on your `java` command
- Sentry will then wrap the other `ContextStorageProvider` that has been configured by loading it through SPI
- If no other `ContextStorageProvider` is available or there are problems loading it, we fall back to using `SentryOtelThreadLocalStorage`

- Fallback to `context.applicationContext` if `Sentry.init(context)` is not instance of Application ([#4355](https://github.com/getsentry/sentry-java/pull/4355))
Copy link
Contributor

Choose a reason for hiding this comment

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

  • 🚫 The changelog entry seems to be part of an already released section ## 8.10.0.
    Consider moving the entry to the ## Unreleased section, please.

@stefanosiano
Copy link
Member

this needs some more discussion, as there are things that may not make sense for react.
We currently add spans and breadcrumbs for Activities, Fragments and User Interactions. And i don't think they are needed in react. For example, spans may be created by native android, but not passed to react, or they could be passed and get duplicated (like app start or TTID) or there could be some weird behaviour, like TTFD started in native but only stopped in react.
@krystofwoldrich What's the reason behind this change?
Also, i think this would affect flutter as well, we should verify it.

@markushi
Copy link
Member

Let's have a quick call (also with @buenaflor) before getting this merged, to clarify what kind of integrations are useful / or not. This probably also affects Unity/Unreal.

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.

4 participants