feat(core): Extend TurboModule instrumentation to legacy NativeModules - #6504
Conversation
Semver Impact of This PR⚪ None (no version bump detected) 📋 Changelog PreviewThis is how your changes will appear in the changelog.
🤖 This preview updates automatically when you update the PR. |
|
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit e8e56a6. Configure here.
📲 Install BuildsAndroid
|
iOS (legacy) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 2cac31b+dirty | 3886.27 ms | 1230.85 ms | -2655.42 ms |
| 244f6e8+dirty | 3833.36 ms | 1212.96 ms | -2620.41 ms |
| 61cc206+dirty | 3849.70 ms | 1230.33 ms | -2619.38 ms |
| 04207c4+dirty | 1191.27 ms | 1189.78 ms | -1.48 ms |
| acd838e+dirty | 3849.78 ms | 1230.00 ms | -2619.78 ms |
| fa21fca+dirty | 3840.43 ms | 1224.42 ms | -2616.02 ms |
| 0d9949d+dirty | 1211.38 ms | 1219.67 ms | 8.29 ms |
| bc8f61e+dirty | 3819.00 ms | 1220.22 ms | -2598.78 ms |
| ca9d079+dirty | 3835.63 ms | 1218.68 ms | -2616.95 ms |
| 5a316ea+dirty | 3820.11 ms | 1211.28 ms | -2608.83 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 2cac31b+dirty | 4.98 MiB | 6.55 MiB | 1.58 MiB |
| 244f6e8+dirty | 4.98 MiB | 6.56 MiB | 1.58 MiB |
| 61cc206+dirty | 4.98 MiB | 6.55 MiB | 1.57 MiB |
| 04207c4+dirty | 3.38 MiB | 4.76 MiB | 1.38 MiB |
| acd838e+dirty | 5.15 MiB | 6.70 MiB | 1.55 MiB |
| fa21fca+dirty | 4.98 MiB | 6.55 MiB | 1.57 MiB |
| 0d9949d+dirty | 3.38 MiB | 4.76 MiB | 1.38 MiB |
| bc8f61e+dirty | 4.98 MiB | 6.47 MiB | 1.49 MiB |
| ca9d079+dirty | 5.15 MiB | 6.69 MiB | 1.53 MiB |
| 5a316ea+dirty | 4.98 MiB | 6.51 MiB | 1.53 MiB |
Previous results on branch: alwx/features/6166
Startup times
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| ffcbcca+dirty | 3817.84 ms | 1206.46 ms | -2611.38 ms |
| 1d407a9+dirty | 3856.64 ms | 1225.98 ms | -2630.66 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| ffcbcca+dirty | 4.98 MiB | 6.55 MiB | 1.58 MiB |
| 1d407a9+dirty | 4.98 MiB | 6.56 MiB | 1.58 MiB |
Android (legacy) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 1d3572b+dirty | 435.35 ms | 487.32 ms | 51.96 ms |
| d2eadf8+dirty | 414.64 ms | 454.56 ms | 39.92 ms |
| 7d8c8bd+dirty | 417.45 ms | 462.10 ms | 44.65 ms |
| bf168a4+dirty | 418.21 ms | 489.74 ms | 71.53 ms |
| 890d145+dirty | 504.54 ms | 491.55 ms | -12.99 ms |
| ad66da3+dirty | 468.46 ms | 533.56 ms | 65.10 ms |
| 15d4514+dirty | 406.77 ms | 428.06 ms | 21.29 ms |
| a0a3177+dirty | 441.27 ms | 499.86 ms | 58.59 ms |
| 6acdf1d+dirty | 513.58 ms | 608.31 ms | 94.72 ms |
| 038a6d7+dirty | 524.82 ms | 531.92 ms | 7.10 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 1d3572b+dirty | 49.74 MiB | 55.38 MiB | 5.63 MiB |
| d2eadf8+dirty | 48.30 MiB | 53.48 MiB | 5.18 MiB |
| 7d8c8bd+dirty | 48.30 MiB | 53.54 MiB | 5.23 MiB |
| bf168a4+dirty | 49.74 MiB | 55.09 MiB | 5.35 MiB |
| 890d145+dirty | 43.75 MiB | 48.14 MiB | 4.39 MiB |
| ad66da3+dirty | 48.30 MiB | 53.49 MiB | 5.19 MiB |
| 15d4514+dirty | 48.30 MiB | 53.60 MiB | 5.30 MiB |
| a0a3177+dirty | 49.74 MiB | 55.37 MiB | 5.63 MiB |
| 6acdf1d+dirty | 49.74 MiB | 55.09 MiB | 5.34 MiB |
| 038a6d7+dirty | 48.30 MiB | 53.60 MiB | 5.30 MiB |
Previous results on branch: alwx/features/6166
Startup times
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 1d407a9+dirty | 544.66 ms | 573.32 ms | 28.66 ms |
| ffcbcca+dirty | 458.32 ms | 508.47 ms | 50.15 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 1d407a9+dirty | 49.74 MiB | 55.38 MiB | 5.64 MiB |
| ffcbcca+dirty | 49.74 MiB | 55.37 MiB | 5.63 MiB |
iOS (new) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 2cac31b+dirty | 3854.43 ms | 1212.35 ms | -2642.08 ms |
| 244f6e8+dirty | 3825.51 ms | 1217.76 ms | -2607.75 ms |
| 9474ead+dirty | 3823.33 ms | 1208.31 ms | -2615.03 ms |
| 44c8b3f+dirty | 3849.24 ms | 1209.94 ms | -2639.31 ms |
| 61cc206+dirty | 3822.60 ms | 1206.17 ms | -2616.43 ms |
| acd838e+dirty | 3835.94 ms | 1215.87 ms | -2620.07 ms |
| b0d3373+dirty | 3842.49 ms | 1218.49 ms | -2624.00 ms |
| fa21fca+dirty | 3845.12 ms | 1215.89 ms | -2629.24 ms |
| 0d9949d+dirty | 1203.94 ms | 1202.27 ms | -1.67 ms |
| a5d243c+dirty | 3827.92 ms | 1220.10 ms | -2607.81 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 2cac31b+dirty | 4.98 MiB | 6.55 MiB | 1.58 MiB |
| 244f6e8+dirty | 4.98 MiB | 6.56 MiB | 1.58 MiB |
| 9474ead+dirty | 5.15 MiB | 6.71 MiB | 1.55 MiB |
| 44c8b3f+dirty | 5.15 MiB | 6.66 MiB | 1.51 MiB |
| 61cc206+dirty | 4.98 MiB | 6.55 MiB | 1.57 MiB |
| acd838e+dirty | 5.15 MiB | 6.70 MiB | 1.55 MiB |
| b0d3373+dirty | 5.15 MiB | 6.68 MiB | 1.53 MiB |
| fa21fca+dirty | 4.98 MiB | 6.55 MiB | 1.57 MiB |
| 0d9949d+dirty | 3.38 MiB | 4.76 MiB | 1.38 MiB |
| a5d243c+dirty | 5.15 MiB | 6.68 MiB | 1.53 MiB |
Previous results on branch: alwx/features/6166
Startup times
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| ffcbcca+dirty | 3837.71 ms | 1209.26 ms | -2628.46 ms |
| 1d407a9+dirty | 3850.93 ms | 1218.24 ms | -2632.69 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| ffcbcca+dirty | 4.98 MiB | 6.55 MiB | 1.58 MiB |
| 1d407a9+dirty | 4.98 MiB | 6.56 MiB | 1.58 MiB |
Android (new) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 1d3572b+dirty | 444.48 ms | 478.42 ms | 33.94 ms |
| 6177334+dirty | 404.80 ms | 456.74 ms | 51.94 ms |
| 5a23c47+dirty | 406.83 ms | 451.47 ms | 44.64 ms |
| a3265b6+dirty | 410.96 ms | 444.76 ms | 33.80 ms |
| bf168a4+dirty | 430.60 ms | 459.31 ms | 28.71 ms |
| ca9d079+dirty | 460.67 ms | 512.54 ms | 51.87 ms |
| ef27341+dirty | 519.02 ms | 553.42 ms | 34.40 ms |
| 23598c3+dirty | 371.92 ms | 420.65 ms | 48.74 ms |
| 7d6fd3a+dirty | 435.06 ms | 458.78 ms | 23.72 ms |
| a50b33d+dirty | 353.21 ms | 398.48 ms | 45.27 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 1d3572b+dirty | 49.74 MiB | 55.38 MiB | 5.63 MiB |
| 6177334+dirty | 48.30 MiB | 53.54 MiB | 5.23 MiB |
| 5a23c47+dirty | 49.74 MiB | 54.82 MiB | 5.07 MiB |
| a3265b6+dirty | 48.30 MiB | 53.58 MiB | 5.28 MiB |
| bf168a4+dirty | 49.74 MiB | 55.09 MiB | 5.35 MiB |
| ca9d079+dirty | 48.30 MiB | 53.58 MiB | 5.28 MiB |
| ef27341+dirty | 48.30 MiB | 53.54 MiB | 5.24 MiB |
| 23598c3+dirty | 43.94 MiB | 49.02 MiB | 5.08 MiB |
| 7d6fd3a+dirty | 43.94 MiB | 49.00 MiB | 5.06 MiB |
| a50b33d+dirty | 43.94 MiB | 48.94 MiB | 5.00 MiB |
Previous results on branch: alwx/features/6166
Startup times
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 1d407a9+dirty | 422.60 ms | 435.78 ms | 13.18 ms |
| ffcbcca+dirty | 426.20 ms | 470.62 ms | 44.42 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 1d407a9+dirty | 49.74 MiB | 55.38 MiB | 5.64 MiB |
| ffcbcca+dirty | 49.74 MiB | 55.37 MiB | 5.63 MiB |
| maxTopModulesPerSpan?: number; | ||
|
|
||
| /** | ||
| * On Old Architecture, auto-wrap registered `NativeModules.*`. Default: `true`. |
There was a problem hiding this comment.
q: Is there any risk in defaulting to true? Would it make sense to start with false and gather feedback?
There was a problem hiding this comment.
Good call. Flipped it to opt-in (enableLegacyNativeModules: false by default)
antonis
left a comment
There was a problem hiding this comment.
Left a couple of comments alogn with the agent feedback but overall LGTM. Let's also add the changes of this PR in the sentry-conventions issue
If so, shouldn't we block this PR until the changes are merged in sentry-conventions? |
| - Extend TurboModule instrumentation to the Old Architecture `NativeModules` bridge ([#6504](https://github.com/getsentry/sentry-react-native/pull/6504)) | ||
|
|
||
| Apps still on the Old Architecture now flow legacy `NativeModules.*` calls through the same aggregator, span-attribution, and slow-call breadcrumb path as TurboModules on the New Architecture. Records carry `arch: 'new' | 'legacy'` (also surfaced on the flushed aggregate span and each attributed span) so analyses can split the signal by architecture. Lazily-exposed modules stay lazy — they're wrapped on first access instead of being initialised during `Sentry.init`. `RNSentry` and hot RN infrastructure (`Timing`, `UIManager`, animated modules) are skipped by default. Auto-wrap is on by default; opt out with `enableLegacyNativeModules: false`, or scope it via `legacyModulesSkip` / `legacyModulesSkipMethods` on `turboModuleContextIntegration`. | ||
|
|
There was a problem hiding this comment.
Let's also move this to the unreleased section
antonis
left a comment
There was a problem hiding this comment.
Left a few comments and a question along with the agent feedback but overall looks good 🚀
|
@lucas-zimerman the only new field is arch: 'new' | 'legacy' on records we already emit — no new attribute keys. In addition, we have a conventions issue in this project anyway (#6168) |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 2980131. Configure here.
antonis
left a comment
There was a problem hiding this comment.
The changes LGTM 🚀
Approving but let's also bump the diffMax since the legacy architecture check fails on iOS on this or a follow up PR.
BinarySizeTest > app size() FAILED
java.lang.AssertionError: 1782264 should be < 1689600
at BinarySizeTest.app size(BinarySizeTest.kt:37)
Wraps `NativeModules.*` on the Old Architecture so bridge calls flow through the same aggregator, span-attribution, and slow-call breadcrumb surface as TurboModules on the New Architecture. - Adds `arch: 'new' | 'legacy'` to TurboModuleRecord / TurboModuleAggregate / TurboModuleCallStart, threaded through `wrapTurboModule`, and folded into the aggregate key so new-arch and legacy calls stay separate on flush. - New `wrapAllNativeModules` enumerates `NativeModules` on the Old Arch and wraps each registered module (skipping RNSentry, which the integration wraps explicitly with a curated recursion-safe skip list). - `turboModuleContextIntegration` auto-invokes it on the Old Arch. Opt out with `enableLegacyNativeModules: false`, or scope with `legacyModulesSkip` / `legacyModulesSkipMethods`. - Aggregate flush emits `turbo_modules.<name>.<method>.<kind>.arch`; attributed spans get a top-level `turbo_module.arch` derived from `isTurboModuleEnabled()` at integration construction (an app doesn't switch archs at runtime). Closes #6166. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…wrapping Review follow-ups on the legacy NativeModules auto-wrap: - Preserve RN's lazy module initialisation. The previous implementation read every NativeModules entry at Sentry.init, which forces each lazily-exposed native module to initialise during startup (defeating RN's laziness and risking module side effects). Lazy accessors are now replaced with a getter that resolves and wraps on first access. - Skip hot RN infrastructure by default (Timing, UIManager and the animated modules). Timing backs every setTimeout, so wrapping it added tracker work to one of the hottest paths and dominated the aggregate. - Warn instead of silently doing nothing when NativeModules cannot be enumerated (the JSI host-object proxy has no enumerable keys), pointing at the explicit `modules` option. - Export TurboModuleArch, which is part of the public wrapTurboModule signature, and refresh the API report.
turboModuleContextIntegration is a default integration, so defaulting enableLegacyNativeModules to true meant every Old Architecture app wrapped every registered bridge module, third-party ones included, at Sentry.init. Ship it opt-in until we have feedback. Also restores the full NativeModules descriptor snapshot in wrapNativeModules tests: the previous afterEach only deleted newly added keys, so overwritten entries and armLazyModule's descriptor rewrites leaked into later tests.
| warnNotEnumerable(); | ||
| return []; | ||
| } | ||
|
|
There was a problem hiding this comment.
skipMethodsPerModule[name] can inherit Object.prototype methods and crash New Set
Accessing skipMethodsPerModule[name] without an own-property check can return inherited Object.prototype functions (e.g. toString, constructor) for NativeModules with colliding names, causing new Set(options.skip) to throw a TypeError and abort Sentry initialization.
Evidence
wrapAllNativeModulesdefaultsskipMethodsPerModuleto{}(a plainObject.prototypeobject) when the caller omits it.- In the module enumeration loop,
skipMethodsPerModule[name]is read without an own-property check (e.g.Object.hasOwnorObject.create(null)). - If
Object.keys(NativeModules)returns a name such astoStringorconstructor, the lookup resolves to the inheritedObject.prototypefunction instead ofundefined. - That function value is passed as
options.skipintowrapTurboModule, wherenew Set(options.skip ?? [])throws because a function is not iterable. - The exception propagates uncaught out of
wrapAllNativeModules, aborting instrumentation duringSentry.init.
Identified by Warden · find-bugs · YTQ-SKG

📢 Type of change
📜 Description
Extends the TurboModule aggregate + span attribution + slow-call breadcrumb surface (from #6377 and #6478) to Old Architecture apps by wrapping
NativeModules.*at setup. Consumer side stays identical — only thearchfield distinguishes new-arch ('new') vs bridge ('legacy') sources.Approach
This ships the JS Proxy path. The native
BridgeNativeModulePerfLoggerpath (option 1) is symmetric but requires new C++/JNI code and a version gate — left for a follow-up. The JS approach works on every RN version we support and has the same shape as the existing TurboModule wrap; hot-path cost is bounded (one property access + function call per bridge invocation).💡 Motivation and Context
TurboModulePerfLoggeronly fires for TurboModules, so apps on the Old Architecture were dark to the entire aggregator / span-attribution / breadcrumb surface. This PR closes that gap so the long tail of un-migrated apps gets the same signals.Closes #6166.
💚 How did you test it?
There are a few tests added!
📝 Checklist
sendDefaultPIIis enabled.