Skip to content

feat(core): Extend TurboModule instrumentation to legacy NativeModules - #6504

Merged
alwx merged 4 commits into
mainfrom
alwx/features/6166
Jul 30, 2026
Merged

feat(core): Extend TurboModule instrumentation to legacy NativeModules#6504
alwx merged 4 commits into
mainfrom
alwx/features/6166

Conversation

@alwx

@alwx alwx commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

📢 Type of change

  • New feature

📜 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 the arch field distinguishes new-arch ('new') vs bridge ('legacy') sources.

Approach

This ships the JS Proxy path. The native BridgeNativeModulePerfLogger path (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

TurboModulePerfLogger only 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

  • I added tests to verify changes.
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
  • I updated the docs if needed.
  • I updated the wizard if needed.
  • All tests passing.
  • Public API changes reviewed by another Mobile SDK team member or implemented according to the develop docs spec.
  • No breaking changes.

@github-actions

github-actions Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Semver Impact of This PR

None (no version bump detected)

📋 Changelog Preview

This is how your changes will appear in the changelog.
Entries from this PR are highlighted with a left border (blockquote style).


  • feat(core): Extend TurboModule instrumentation to legacy NativeModules by alwx in #6504
  • feat(ios): Introduce enableMetricKit option by alwx in #6540

🤖 This preview updates automatically when you update the PR.

@github-actions

github-actions Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor
Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 990db93

@alwx
alwx force-pushed the alwx/features/6166 branch from 6c180fc to e8e56a6 Compare July 23, 2026 08:54
@alwx

alwx commented Jul 23, 2026

Copy link
Copy Markdown
Contributor Author

@cursor review

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

@alwx
alwx force-pushed the alwx/feature/6165 branch from b3dc8ae to 49ba31f Compare July 23, 2026 14:05
Base automatically changed from alwx/feature/6165 to main July 27, 2026 13:05
@alwx
alwx force-pushed the alwx/features/6166 branch from e8e56a6 to ffb6280 Compare July 28, 2026 10:31
@alwx
alwx marked this pull request as ready for review July 28, 2026 10:49
@alwx
alwx requested review from a team, antonis and lucas-zimerman as code owners July 28, 2026 10:49
@antonis antonis added the ready-to-merge Triggers the full CI test suite label Jul 28, 2026
@sentry

sentry Bot commented Jul 28, 2026

Copy link
Copy Markdown

📲 Install Builds

Android

🔗 App Name App ID Version Configuration
Sentry RN io.sentry.reactnative.sample 8.21.0 (101) Release

⚙️ sentry-react-native Build Distribution Settings

@github-actions

github-actions Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

iOS (legacy) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 3831.88 ms 1208.91 ms -2622.96 ms
Size 4.98 MiB 6.56 MiB 1.58 MiB

Baseline results on branch: main

Startup times

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

@github-actions

github-actions Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Android (legacy) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 411.82 ms 444.15 ms 32.33 ms
Size 49.74 MiB 55.38 MiB 5.64 MiB

Baseline results on branch: main

Startup times

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

@github-actions

github-actions Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

iOS (new) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 3842.47 ms 1210.84 ms -2631.62 ms
Size 4.98 MiB 6.56 MiB 1.58 MiB

Baseline results on branch: main

Startup times

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

@github-actions

github-actions Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Android (new) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 464.04 ms 473.33 ms 9.28 ms
Size 49.74 MiB 55.38 MiB 5.64 MiB

Baseline results on branch: main

Startup times

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

Comment thread packages/core/test/turbomodule/wrapNativeModules.test.ts
Comment thread CHANGELOG.md Outdated
maxTopModulesPerSpan?: number;

/**
* On Old Architecture, auto-wrap registered `NativeModules.*`. Default: `true`.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

q: Is there any risk in defaulting to true? Would it make sense to start with false and gather feedback?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good call. Flipped it to opt-in (enableLegacyNativeModules: false by default)

@antonis antonis left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

@lucas-zimerman

Copy link
Copy Markdown
Collaborator

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?

@alwx
alwx requested review from antonis and removed request for antonis July 30, 2026 09:06
Comment thread packages/core/src/js/turbomodule/wrapNativeModules.ts
Comment thread CHANGELOG.md
- 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`.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's also move this to the unreleased section

@antonis antonis left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Left a few comments and a question along with the agent feedback but overall looks good 🚀

@alwx

alwx commented Jul 30, 2026

Copy link
Copy Markdown
Contributor Author

@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)

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

There are 2 total unresolved issues (including 1 from previous review).

Fix All in Cursor

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

Comment thread packages/core/src/js/turbomodule/wrapNativeModules.ts
@alwx
alwx requested a review from antonis July 30, 2026 11:54

@antonis antonis left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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)

alwx and others added 4 commits July 30, 2026 16:14
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.
@alwx
alwx force-pushed the alwx/features/6166 branch from 86befc9 to 990db93 Compare July 30, 2026 14:17
warnNotEnumerable();
return [];
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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
  • wrapAllNativeModules defaults skipMethodsPerModule to {} (a plain Object.prototype object) when the caller omits it.
  • In the module enumeration loop, skipMethodsPerModule[name] is read without an own-property check (e.g. Object.hasOwn or Object.create(null)).
  • If Object.keys(NativeModules) returns a name such as toString or constructor, the lookup resolves to the inherited Object.prototype function instead of undefined.
  • That function value is passed as options.skip into wrapTurboModule, where new Set(options.skip ?? []) throws because a function is not iterable.
  • The exception propagates uncaught out of wrapAllNativeModules, aborting instrumentation during Sentry.init.

Identified by Warden · find-bugs · YTQ-SKG

@alwx
alwx merged commit aa4c920 into main Jul 30, 2026
78 of 88 checks passed
@alwx
alwx deleted the alwx/features/6166 branch July 30, 2026 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-to-merge Triggers the full CI test suite

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Legacy NativeModules wrapper for the old architecture

3 participants