feat(core): Wire TurboModulePerfLogger on iOS and Android#6307
Merged
Conversation
Contributor
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. |
Contributor
If a user enables AGP
Our docs instruct React Native users to set
|
![sentry-warden[bot]](https://avatars.githubusercontent.com/in/2755838?s=60&v=4){kind=small}
Contributor
Author
|
@cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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 3b618c5. Configure here.
alwx
added a commit
that referenced
this pull request
Jun 18, 2026
Address Warden's medium-severity finding on PR #6307: the new `SentryTurboModulePerfController` and `RNSentryTurboModulePerfTracker` shipped without unit coverage. Add focused tests that exercise the state machines independently of React Native's runtime. - **iOS** (`RNSentryCocoaTester/.../RNSentryTurboModulePerfControllerTests.mm`): default `isEnabled() == false`, `setEnabled` toggle, the C-linkage `Sentry_SetTurboModuleTrackingEnabled` entry point matches the typed setter, `setSink`/`sink` round-trips including `nullptr` detach, and `Sentry_InstallTurboModulePerfLogger` idempotency under repeated calls. End-to-end forwarding through `facebook::react::TurboModulePerfLogger` is intentionally not covered here \u2014 it requires `+load` ordering and process-wide singletons that the follow-up sink PRs will integration-test. - **Android** (`RNSentryAndroidTester/.../RNSentryTurboModulePerfTrackerTest.kt`): the JVM-side latch around the JNI symbol. In the test JVM the underlying `.so` is not loaded, so the first `setEnabled` call must catch `UnsatisfiedLinkError` and flip `nativeUnavailable`; subsequent calls must short-circuit. Uses Robolectric so the `android.util.Log.i` call inside the catch branch resolves instead of throwing the not-mocked stub. A small `@TestOnly` window on the tracker exposes the latch state to assertions. Also fix the changelog entry to reference the PR (#6307) rather than the issue (#6162) so danger stops nagging.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
antonis
reviewed
Jun 18, 2026
antonis
left a comment
antonis
left a comment
Contributor
There was a problem hiding this comment.
Thank you for you work on this @alwx 🙇
Did a 1st pass and didn't notice anything off other what has been caught by the agents and the lint check. Let's also add the ready-to-merge since there are many changes on the native side that need to be validated.
alwx
added a commit
that referenced
this pull request
Jun 22, 2026
…g flips on Address two related medium findings on #6307: - Warden: `enableLogging` runs from `+load` / `JNI_OnLoad` regardless of the runtime flag, unconditionally evicting any pre-existing `NativeModulePerfLogger` (Metro, other SDKs, host-app instrumentation). - Cursor: when `enableTurboModuleTracking: true`, callbacks between load time and `initNativeSdk` are dropped by the `enabled_=false` fast-path anyway, so the eager install was not actually delivering on its 'never miss early events' promise \u2014 just on its side effects. The fix is a single one-way ratchet: `setEnabled(true)` lazily calls `install()` on the first transition, and the typed setter doubles as the public lifecycle hook. The `+load` installer class on iOS and the `JNI_OnLoad` install on Android are gone; the C `Sentry_InstallTurboModulePerfLogger` entry stays for hosts that want to claim the perf-logger slot eagerly via their own native code, but it is no longer wired into our load hooks. Header / JSDoc updated to describe the new contract. Also fix two adjacent issues flagged on the same PR: - Sentry HIGH (build.gradle): two sibling `buildFeatures { ... }` blocks under the same Android scope replace rather than merge, so `prefab = true` was clobbering `buildConfig = true` on AGP 8+. Merge into a single conditional block. - Lint: ran `yarn java:format fix`, `yarn fix:clang`, and switched `RNSentryTurboModulePerfTracker.nativeUnavailable` from `volatile` to `AtomicBoolean` to satisfy the project-wide PMD `AvoidUsingVolatile` rule. Removed a Kotlin `no-consecutive-comments` violation from the Robolectric note above the tracker test. Test updates: - iOS: add `testSetEnabledFalseDoesNotInstall` and `testSetEnabledTrueIsLazyInstallAndSticky` to lock down the lazy install ratchet. Existing `testInstallIsIdempotent` still covers explicit-install callers. - Android: tracker tests unchanged in behaviour; only the test-only `isNativeUnavailableForTests` / `resetNativeUnavailableForTests` helpers were updated to go through the new `AtomicBoolean`.
alwx
added a commit
that referenced
this pull request
Jun 22, 2026
Address Cursor's low-severity finding on #6307: `setEnabled(true)` was storing `enabled_` *after* calling `install()`, so any callback React Native fired synchronously from inside `enableLogging()` would hit the `isEnabled() == false` fast-path and be dropped \u2014 a tiny window of lost events for the very first opted-in invocation. Swap the order: publish `enabled_ = true` (release ordering) before the install, so by the time `enableLogging()` could re-enter us via a synchronous callback, the flag is already visible to other threads. On disable the order does not matter since we never uninstall.
Contributor
Android (legacy) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| eb93136+dirty | 416.18 ms | 467.32 ms | 51.14 ms |
| 5257d80+dirty | 423.37 ms | 467.54 ms | 44.17 ms |
| ca9d079+dirty | 411.29 ms | 455.12 ms | 43.83 ms |
| 7887847+dirty | 416.61 ms | 462.04 ms | 45.43 ms |
| 1122a96+dirty | 422.22 ms | 464.33 ms | 42.10 ms |
| 88735e9+dirty | 429.04 ms | 484.17 ms | 55.13 ms |
| 5fe1c6c+dirty | 401.62 ms | 445.28 ms | 43.66 ms |
| 4953e94+dirty | 442.02 ms | 456.52 ms | 14.50 ms |
| ecf47a2+dirty | 420.40 ms | 458.02 ms | 37.62 ms |
| 5a010b7+dirty | 425.62 ms | 469.38 ms | 43.76 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| eb93136+dirty | 48.30 MiB | 53.58 MiB | 5.28 MiB |
| 5257d80+dirty | 48.30 MiB | 53.58 MiB | 5.28 MiB |
| ca9d079+dirty | 48.30 MiB | 53.58 MiB | 5.28 MiB |
| 7887847+dirty | 49.74 MiB | 54.81 MiB | 5.07 MiB |
| 1122a96+dirty | 48.30 MiB | 53.54 MiB | 5.24 MiB |
| 88735e9+dirty | 49.74 MiB | 54.82 MiB | 5.07 MiB |
| 5fe1c6c+dirty | 43.75 MiB | 48.14 MiB | 4.39 MiB |
| 4953e94+dirty | 43.75 MiB | 48.08 MiB | 4.33 MiB |
| ecf47a2+dirty | 49.74 MiB | 54.82 MiB | 5.07 MiB |
| 5a010b7+dirty | 48.30 MiB | 53.58 MiB | 5.28 MiB |
📲 Install BuildsAndroid
|
Contributor
iOS (legacy) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 9474ead+dirty | 3864.29 ms | 1223.55 ms | -2640.74 ms |
| 5fe1c6c+dirty | 1220.79 ms | 1217.63 ms | -3.16 ms |
| ae37560+dirty | 3832.10 ms | 1219.09 ms | -2613.02 ms |
| f170ec3+dirty | 3822.26 ms | 1218.33 ms | -2603.93 ms |
| df5d108+dirty | 1225.90 ms | 1220.14 ms | -5.76 ms |
| 882f8ae+dirty | 3840.30 ms | 1224.41 ms | -2615.88 ms |
| 23598c3+dirty | 1207.00 ms | 1209.90 ms | 2.90 ms |
| 1122a96+dirty | 3823.10 ms | 1218.64 ms | -2604.46 ms |
| 580fb5c+dirty | 3836.13 ms | 1218.72 ms | -2617.41 ms |
| 5a23c47+dirty | 3855.46 ms | 1221.95 ms | -2633.50 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 9474ead+dirty | 5.15 MiB | 6.71 MiB | 1.55 MiB |
| 5fe1c6c+dirty | 3.38 MiB | 4.77 MiB | 1.39 MiB |
| ae37560+dirty | 5.15 MiB | 6.70 MiB | 1.54 MiB |
| f170ec3+dirty | 5.15 MiB | 6.69 MiB | 1.53 MiB |
| df5d108+dirty | 3.38 MiB | 4.73 MiB | 1.35 MiB |
| 882f8ae+dirty | 5.15 MiB | 6.70 MiB | 1.54 MiB |
| 23598c3+dirty | 3.38 MiB | 4.80 MiB | 1.42 MiB |
| 1122a96+dirty | 5.15 MiB | 6.68 MiB | 1.53 MiB |
| 580fb5c+dirty | 4.98 MiB | 6.46 MiB | 1.48 MiB |
| 5a23c47+dirty | 4.98 MiB | 6.46 MiB | 1.49 MiB |
alwx
added a commit
that referenced
this pull request
Jun 22, 2026
…debug info Address Warden's medium-severity finding on #6307: passing `-Wl,--strip-all` at CMake link time strips DWARF (and `.symtab`) from `libsentry-tm-perf-logger.so` *before* AGP's `StripDebugSymbolsTask` gets a chance to copy the unstripped artefact for symbolication upload. Any crash inside the library in production would be unsymbolicated even with the Sentry Gradle plugin installed. Drop the manual link option entirely. AGP already strips the .so for the packaged APK while preserving the unstripped copy under `intermediates/merged_native_libs/.../obj`, which is the one Sentry Gradle plugin uploads. Verified locally with `llvm-readelf -S` on the release intermediate: `.debug_*` and `.symtab` sections are now present.
Contributor
iOS (new) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 038a6d7+dirty | 3838.96 ms | 1218.86 ms | -2620.10 ms |
| 5748023+dirty | 3844.74 ms | 1225.49 ms | -2619.26 ms |
| b0d3373+dirty | 3842.49 ms | 1218.49 ms | -2624.00 ms |
| 7887847+dirty | 3844.89 ms | 1221.67 ms | -2623.22 ms |
| 6177334+dirty | 3851.52 ms | 1226.23 ms | -2625.29 ms |
| 21a1e70+dirty | 3826.22 ms | 1217.74 ms | -2608.47 ms |
| 774257e+dirty | 3821.35 ms | 1211.96 ms | -2609.39 ms |
| 5789645+dirty | 3841.36 ms | 1214.81 ms | -2626.55 ms |
| 15d4514+dirty | 3843.73 ms | 1228.09 ms | -2615.64 ms |
| 94af3bd+dirty | 3847.84 ms | 1230.33 ms | -2617.51 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 038a6d7+dirty | 5.15 MiB | 6.70 MiB | 1.55 MiB |
| 5748023+dirty | 5.15 MiB | 6.68 MiB | 1.53 MiB |
| b0d3373+dirty | 5.15 MiB | 6.68 MiB | 1.53 MiB |
| 7887847+dirty | 4.98 MiB | 6.46 MiB | 1.48 MiB |
| 6177334+dirty | 5.15 MiB | 6.68 MiB | 1.53 MiB |
| 21a1e70+dirty | 4.98 MiB | 6.46 MiB | 1.49 MiB |
| 774257e+dirty | 5.15 MiB | 6.70 MiB | 1.54 MiB |
| 5789645+dirty | 4.98 MiB | 6.50 MiB | 1.52 MiB |
| 15d4514+dirty | 5.15 MiB | 6.70 MiB | 1.55 MiB |
| 94af3bd+dirty | 5.15 MiB | 6.69 MiB | 1.53 MiB |
Contributor
Android (new) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| b9bebee+dirty | 500.50 ms | 536.42 ms | 35.92 ms |
| ecf47a2+dirty | 457.21 ms | 498.10 ms | 40.89 ms |
| 5fe1c6c+dirty | 365.84 ms | 408.62 ms | 42.78 ms |
| 4953e94+dirty | 398.80 ms | 431.81 ms | 33.01 ms |
| 1a2e7e0+dirty | 451.98 ms | 501.50 ms | 49.52 ms |
| 5125c43+dirty | 409.52 ms | 451.00 ms | 41.48 ms |
| 0b1b5e3+dirty | 425.58 ms | 476.02 ms | 50.44 ms |
| 1122a96+dirty | 510.16 ms | 542.00 ms | 31.84 ms |
| 7ff4d0f+dirty | 403.38 ms | 427.06 ms | 23.68 ms |
| 5a21b51+dirty | 505.16 ms | 539.20 ms | 34.04 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| b9bebee+dirty | 48.30 MiB | 53.58 MiB | 5.28 MiB |
| ecf47a2+dirty | 49.74 MiB | 54.82 MiB | 5.07 MiB |
| 5fe1c6c+dirty | 43.94 MiB | 49.00 MiB | 5.06 MiB |
| 4953e94+dirty | 43.94 MiB | 48.94 MiB | 5.00 MiB |
| 1a2e7e0+dirty | 49.74 MiB | 54.82 MiB | 5.07 MiB |
| 5125c43+dirty | 48.30 MiB | 53.54 MiB | 5.24 MiB |
| 0b1b5e3+dirty | 48.30 MiB | 53.60 MiB | 5.29 MiB |
| 1122a96+dirty | 48.30 MiB | 53.54 MiB | 5.24 MiB |
| 7ff4d0f+dirty | 48.30 MiB | 53.60 MiB | 5.30 MiB |
| 5a21b51+dirty | 48.30 MiB | 53.49 MiB | 5.19 MiB |
Mirrors the iOS behavior: SentryAndroid.init can throw at runtime (e.g.\non invalid DSN or option configuration), but the Android initNativeSdk\npreviously caught nothing and unconditionally resolved the promise as\ntrue. The JS caller would then proceed to make native API calls against\nan uninitialized SDK and observe confusing downstream failures.\n\nWrap RNSentryStart.startWithOptions in a try/catch and reject the\npromise with the underlying error, matching the iOS reject(@"SentryReactNative",\n...) contract. The TurboModule perf-logger toggle is also skipped on\nfailure so we don't claim React Native's perf-logger slot when no\nSentry SDK is around to consume the data.\n\nAlso move the 8.15.0 changelog entry for enableTurboModuleTracking into\nthe Unreleased section, since this PR is now targeting the next\nrelease, and shorten it per review feedback.
Two related fixes from review:
1. Re-init bug: the native TurboModule perf controller is process-wide
and not reset by closeNativeSdk. The previous code only forwarded the
JS option when it was present and a boolean, so a sequence of
init({enableTurboModuleTracking: true}) -> init({}) would leave
tracking latched on indefinitely after a prior opt-in. Both iOS and
Android now resolve the option to a concrete boolean (defaulting to
false when the key is absent or non-boolean) and unconditionally
call setEnabled, mirroring the JS-side default of false on every
init. setEnabled(false) is cheap and never triggers the lazy
install, so hosts that never opt in still pay zero native cost.
2. Stale JSDoc: the enableTurboModuleTracking docstring still claimed
the native perf logger is installed at SDK load time. That changed
with the lazy-load refactor — install() is now reached only on the
first setEnabled(true). Updated the JSDoc to describe the actual
behavior so consumers know the RN perf-logger slot stays untouched
while the option is off.
Turns on the experimental TurboModule perf logger in the React Native\nsample app so reviewers can capture sample events end-to-end. The\nexisting "Native Crash" button on the Errors tab already routes through\nthe wrapped RNSentry.nativeCrash TurboModule call, so pressing it now\nproduces a crash report tagged with contexts.turbo_module +\nturbo_module.name=RNSentry / turbo_module.method=nativeCrash.\n\nNo-op on the Old Architecture and on hosts that strip Sentry's native\nlibraries; setEnabled(true) lazily loads libsentry-tm-perf-logger.so on\nfirst opt-in.
The TurboModule call tracker pushed context+tags onto getCurrentScope(),\nbut enableSyncToNative is wired only on the global and isolation scopes.\nUnder @sentry/core's async-context strategy getCurrentScope() can return\na forked scope that is neither \u2014 in which case the scope.setContext(\n'turbo_module', ...) call updates JS state only and never propagates to\nsentry-cocoa / sentry-java. As a result, a native crash captured during\na TurboModule invocation lost the contexts.turbo_module attribution and\nthe turbo_module.name / turbo_module.method tags it was supposed to\ncarry, defeating the whole point of turboModuleContextIntegration.\n\nDefault pushTurboModuleCall to the isolation scope instead. Isolation\nis always synced to native via enableSyncToNative, so the write reaches\nsentry-cocoa / sentry-java in time for the crash report; JS captures\nstill see the data because @sentry/core merges global + isolation +\ncurrent at event time. Tests can still pass an explicit scope.\n\nVerified end-to-end in the RN sample: triggering 'Native Crash' on the\nErrors tab now produces an issue with contexts.turbo_module = { name:\n'RNSentry', method: 'crash', kind: 'sync', \u2026 } and matching tags.\n\nAlso correct the App.tsx comment to reference the underlying\nRNSentry.crash TurboModule method (not RNSentry.nativeCrash, which is\nthe JS-side wrapper name).
The setEnabled() short-circuit `!enabled && !libraryLoadAttempted.get()`\nran *outside* the monitor that ensureNativeLibraryLoaded() acquires for\nthe lazy System.loadLibrary call. A second thread issuing setEnabled(\nfalse) while a first thread was still inside loadLibrary observed\nlibraryLoadAttempted == false (the first thread sets it only after the\nload completes), returned early, and left tracking latched on after the\nloader finished \u2014 the opposite of what the disabling caller asked for.\n\nMake setEnabled itself synchronized so the short-circuit and the lazy\nload share a single monitor. Concurrent enable/disable now resolves as\nlast-call-wins instead of dropping the disable.\n\nThe lock is contended only on the first few calls before the library\nfinishes loading; setEnabled is invoked once per initNativeSdk so the\noverhead is negligible.
CI was failing 2 tests in `wrapTurboModule.test.ts` ("tracker push\nthrows" and "tracker pop throws"): the spy on `scope.setContext` was\nnever fired, so `pushTurboModuleCall` never threw, so the diagnostic\n`warn` call the tests asserted on never happened.\n\nRoot cause: commit `fix(turbomodule): Default TM tracker to isolation\nscope for native sync` (`8a16f7de`) switched `pushTurboModuleCall`'s\ndefault scope from `getCurrentScope()` to `getIsolationScope()` and\nupdated `turboModuleTracker.test.ts` to mock the new entry point, but\nmissed the sibling `wrapTurboModule.test.ts` which still mocked only\n`getCurrentScope`. The wrapper therefore wrote context/tags onto the\nreal isolation scope, the test's mock\u2019d `scope.setContext` never\nfired, and the assertions about the diagnostic warn went un-met.\n\nMock both `getIsolationScope` and `getCurrentScope` so the test\nremains deterministic regardless of which scope the tracker walks.\nAll 1618 JS tests pass after the change.
…td::terminate Cursor HIGH on #6307: `SentryTurboModulePerfController::install` is\ndeclared `noexcept`, but it calls `std::make_unique<ForwardingLogger>()`\n(can throw `std::bad_alloc`) and forwards to\n`facebook::react::TurboModulePerfLogger::enableLogging` (no exception\nguarantees). If either throws \u2014 most plausibly under memory pressure\nwhen a user opts in to TurboModule tracking \u2014 the runtime calls\n`std::terminate` and the host app dies, instead of gracefully degrading\nto tracking-off.\n\nWrap the allocation+install in a `try { } catch (...) { }` and roll\nback the `installed_` latch on failure so a later `setEnabled(true)`\ncan retry once memory pressure clears. The SDK keeps running; the\nworst-case observable effect is that tracking remained off when the\nuser opted in. That is strictly better than terminating the process.\n\n`setEnabled` is unchanged \u2014 it only writes atomics and calls `install`,\nwhich is now truly `noexcept` after this fix.\n\nThe other comment in this review pass (Sentry bot MEDIUM about a\nconcurrent enable/disable race in `RNSentryTurboModulePerfTracker.setEnabled`)\nwas already addressed in commit 482ac45, which made `setEnabled`\n`synchronized` so the short-circuit and the lazy `System.loadLibrary`\nshare a single monitor.
Two small follow-ups on #6307: - Warden flagged the 23-character placeholder UUIDs I hand-rolled (`A1B2C3D4E5F600000000001` / `A1B2C3D4E5F600000000002`) in the RNSentryCocoaTester pbxproj when adding the new `RNSentryTurboModulePerfControllerTests.mm` source. Xcode expects exactly 24 uppercase hex characters; antonis confirmed. Replaced with real 24-character UUIDs (`2639D71D3BD04F17B0BAC987` / `E795057A6D534A80A9D06356`) across all four references (`PBXBuildFile`, `PBXFileReference`, `PBXSourcesBuildPhase`, and the group children). - clang-format violation on the wrapped `enableLogging(...)` call in the noexcept-install fix \u2014 `yarn fix:clang` realigns it. CI lint job (run 28083145588) now matches local output. Verified with `pod install` + `xcodebuild test` on the cocoa-tester target: all 7 `RNSentryTurboModulePerfControllerTests` pass.
…ll race Four fixes from PR #6307 review: 1. **antonis blocker** (RN 0.71.19 legacy Android matrix): the build failed with `CMake Error at CMakeLists.txt:21 (add_library): Target 'sentry-tm-perf-logger' links to target 'ReactAndroid::reactnative' but the target was not found.` even though my gradle conditional only declared `externalNativeBuild` under NewArch. The cause is AGP/CMake auto-detection of `CMakeLists.txt` at the module root. Move the file to `src/main/jni/CMakeLists.txt` (next to OnLoad.cpp) so it lives outside any auto-detected path; gradle now references it explicitly via `cmake { path "src/main/jni/CMakeLists.txt" }` and the Old Arch build no longer touches it. Verified locally by running `./gradlew :sentry_react-native:assembleRelease` from `performance-tests/TestAppSentry/android` (newArchEnabled=false): no `configureCMake` task runs, no .so is produced, build succeeds. 2. **Warden** (`RNSentryModuleImpl.java:229`): if `RNSentryTurboModulePerfTracker.setEnabled` threw anything other than the already-caught `UnsatisfiedLinkError` (e.g. `SecurityException` from `System.loadLibrary`, any `RuntimeException` from the JNI symbol), execution skipped past `promise.resolve(true)` and the JS-side `initNativeSdk` promise hung forever. Wrap the `setEnabled` call in its own `try/catch` that logs and continues \u2014 the SDK has already started by this point, so a tracking-toggle failure is non-fatal to init. 3. **Cursor** (cross-platform parsing inconsistency): iOS accepted any `NSNumber` as the option value (so JS numeric `1` enabled tracking), Android required `ReadableType.Boolean` (so JS `1` was ignored). Tighten iOS to match: only honour an NSNumber whose `objCType` is `@encode(BOOL)`, which is what RN's bridge produces for a real JS boolean. JS numbers and strings now consistently fail the check on both platforms. 4. **Sentry bot** (race in `SentryTurboModulePerfController::install`): the previous fix rolled back `installed_` on `enableLogging` failure so a later caller could retry. That introduced a race: a concurrent thread observing the brief `installed_ == true` window would skip its own install attempt, then the originating thread's rollback would put us in a state where every caller thought someone else installed but nobody actually did. Switch to sticky "install attempted" semantics \u2014 the latch never rolls back. A failed install during the user opt-in path leaves tracking off for the rest of the process, which is strictly better than a silent half-installed state. All builds + tests pass locally (`yarn lint`, samples on both arches, `RNSentryAndroidTester`, `RNSentryCocoaTester/RNSentryTurboModulePerfControllerTests`).
… for option parsing Warden caught a real bug in my previous `enableTurboModuleTracking`\nparsing fix: on 64-bit iOS `BOOL` is `typedef bool BOOL`, so\n`@encode(BOOL)` expands to `"B"` \u2014 but `[NSNumber numberWithBool:YES]`\n(including every JS boolean crossing the RN bridge) always reports\n`objCType == "c"` for historical compatibility. The\n`strcmp([(NSNumber *)value objCType], @encode(BOOL))` check therefore\nnever matched on any modern iOS device, and\n`enableTurboModuleTracking: true` was a silent no-op on iOS.\n\nReplace with the canonical, toll-free-bridged check\n`CFGetTypeID == CFBooleanGetTypeID()`. Verified with a small repro\nthat `@YES` returns `true` and `@1` returns `false` from\n`CFBooleanGetTypeID()`.\n\nAlso extract the parsing into a testable class method\n(`+ [RNSentry turboModuleTrackingEnabledFromOptions:]`) and add 7 unit\ntests in `RNSentryTurboModulePerfControllerTests.mm` covering:\n\n - JS `true` \u2192 enabled (the bug)\n - JS `false` \u2192 disabled\n - JS `1` \u2192 disabled (cross-platform parity with Android's\n `ReadableType.Boolean`)\n - JS `0` \u2192 disabled\n - String \u2192 disabled\n - Missing key \u2192 disabled\n - `NSNull` \u2192 disabled\n\nDictionary literals are hoisted into locals inside each test because\n`XCTAssertTrue`/`Fals`'s macro expansion uses `catch(T)` in ObjC++ and\nthe parser otherwise chokes on the comma inside `@{ k : v, }`.
…ode fallback error
Two follow-ups from latest review pass:
- **Cursor MEDIUM** (`SentryTurboModulePerfLogger.cpp`): the previous
sticky-install fix accidentally let `isEnabled()` lie. The flow was
`setEnabled(true)` \u2192 `enabled_ = true` \u2192 `install()` \u2192
`enableLogging` throws \u2192 `installed_` stays sticky-true (no retry)
\u2192 `isEnabled()` returns `true` even though RN never received the
perf logger. Worse, a later `setEnabled(true)` would short-circuit
on the sticky latch and the same lie persisted for the rest of the
process.
Split the latch into `installAttempted_` (sticky after the first
try) and `installed_` (only `true` if `enableLogging` succeeded),
and have `isEnabled()` AND user intent with actual install state.
Now: a successful install \u2192 `isEnabled()` mirrors `enabled_`. A
failed install \u2192 `isEnabled()` is permanently `false` for the
process, even if the user keeps calling `setEnabled(true)`. This is
strictly honest about what tracking can deliver and means tests and
consumers stop seeing false-positive 'tracking is on' readings.
- **Sentry bot MEDIUM** (`build.gradle`): `resolveReactNativeDir()`
calls `providers.exec("node", ...).get()` at gradle configure time,
which throws hard if `node` is not on PATH. Wrap with a clearer
`GradleException` pointing at the `REACT_NATIVE_NODE_MODULES_DIR`
ext property the host project can use to skip the node lookup.
- **Warden** (RNSentry.mm BOOL parsing): the new comment was a snapshot
of the bug that 416006d already fixed via `CFBooleanGetTypeID`.
No code change needed beyond a reply pointing at that commit.
…lability
Address the RN 0.71.19 legacy CI failure. Root cause:
`dev-packages/e2e-tests/cli.mjs` has a long-standing bug: the check
`if (env.RCT_NEW_ARCH_ENABLED)` is truthy for the string "0", so
the legacy matrix entries (which set the env var to "0") actually
flip `newArchEnabled=true` in the host app's `gradle.properties`.
That has been silently masking new-arch coverage as legacy on every
PR for ages; my CMake config is just the first thing that requires
the `ReactAndroid::reactnative` prefab, which only exists in RN
>= 0.75. So configure fails on RN 0.71.19 with:
CMake Error at CMakeLists.txt:27 (add_library):
Target "sentry-tm-perf-logger" links to target
"ReactAndroid::reactnative" but the target was not found.
Rather than touch the CI script (out of this PR's scope), gate our
CMake config on the host's actual React Native version. The new
`isReactNativePrefabAvailable()` helper:
1. requires `newArchEnabled=true` (same as before)
2. resolves the host's `react-native/package.json` (via the existing\n `resolveReactNativeDir()` helper, which honours\n `REACT_NATIVE_NODE_MODULES_DIR`)
3. returns true only when the major.minor is >= 0.75
Both `buildFeatures { prefab true }` and `externalNativeBuild { cmake\n... }` blocks are now keyed off this gate. On RN < 0.75 we skip the
native build entirely; `RNSentryTurboModulePerfTracker.setEnabled`
catches the missing `.so` via its existing `UnsatisfiedLinkError`
latch, exactly as it already does on Old Architecture.
Verified locally:
- RN 0.86.0 + NewArch \u2192 prefab-available = true, 4 .so files in AAR
- RN 0.71.11 \u2192 prefab-available = false, no .so files
- Android unit tests \u2192 still pass
Co-authored-by: Antonis Lilis <antonis.lilis@sentry.io>
alwx
force-pushed
the
alwx/feature/turbo-module-perf-logger
branch
from
5da4254 to
955852e
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 955852e. Configure here.
3 tasks
11 tasks
antonis
added a commit
that referenced
this pull request
Jul 3, 2026
…so (#6396) * fix(android): Force 16 KB ELF alignment for libsentry-tm-perf-logger.so The native library added in 8.17.0 (#6307) for Turbo Module performance tracking is compiled from source with the app's NDK. On NDK r27 and earlier the linker defaults to 4 KB segment alignment, so this library was the lone misaligned `.so` in the APK (all prebuilt libs are already 16 KB aligned). This tripped Android 15+'s 16 KB page size compatibility check and Google Play's 16 KB requirement for New Architecture apps — even when `enableTurboModuleTracking` was left disabled, since the library is packaged whenever the New Architecture is enabled. Pass `-Wl,-z,max-page-size=16384` to the CMake target. Verified with NDK 27 against the RN 0.86 source tree: LOAD segment alignment goes from 0x1000 (4 KB) to 0x4000 (16 KB), with no file-size change. Backward compatible with 4 KB-page devices (16 KB is a multiple of 4 KB) and a no-op on NDK r28+ where 16 KB is already the default. Fixes #6394 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Update changelog * test(android): Add CI check for 16 KB native library alignment Guards against regressing #6394. Adds scripts/check-android-16kb-alignment.sh which unzips a built APK/AAB and asserts every bundled .so has ELF LOAD segments aligned to at least 16 KB (p_align >= 0x4000), failing otherwise. Wired into the sample-application workflow right after the New Architecture Android app build, where a real APK is produced. The script is portable across the macOS (bash 3.2) and Ubuntu CI (mawk) shells and resolves readelf from $READELF, then llvm-readelf, then readelf. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs: Fix grammar in changelog entry Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(android): Scope 16 KB alignment check to Sentry-owned libraries The initial CI run failed because the check scanned every .so in the APK, including React Native and third-party libraries (libreactnative, libhermesvm, libreanimated, librnscreens, ...) which are 4 KB aligned in the x86 build CI produces. RN aligns arm64 to 16 KB but not x86, and those libraries are outside this repo's control. Add an optional name-filter argument to the check script and pass 'libsentry' in CI so the guard verifies only the libraries this repo ships (libsentry-tm-perf-logger.so et al.). A regression that drops the max-page-size flag from our CMake target is still caught — verified locally: with the filter, our lib at 4 KB still fails the check. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
This was referenced Jul 6, 2026
antonis
added a commit
that referenced
this pull request
Jul 6, 2026
…logger.so + CI ABI/link coverage (#6406) * fix(android): Prevent New Arch build break linking libsentry-tm-perf-logger.so The perf-logger native library added in 8.17.0 (#6307) references facebook::react::TurboModulePerfLogger::enableLogging, which lives in React Native's `reactnative` prefab. Some New Architecture build setups (e.g. Expo builds on armeabi-v7a) do not satisfy that reference at link time, and because the NDK/AGP toolchain links with -Wl,-z,defs (--no-undefined) the unresolved symbol becomes a fatal "undefined symbol" error that breaks the whole Android build — even for hosts that never opt in to enableTurboModuleTracking, since the library is compiled whenever the New Architecture is enabled. Append -Wl,-z,undefs to the target's link options so undefined symbols are non-fatal, leaving them to resolve at load time against the loaded libreactnative.so. This is safe: the library is loaded lazily and only when tracking is opted in, and an unresolvable symbol surfaces as an UnsatisfiedLinkError from System.loadLibrary that the Java side already swallows (RNSentryTurboModulePerfTracker), degrading the experimental feature to a no-op rather than crashing. Verified with NDK 27 against the RN 0.86 prefab: - armeabi-v7a with --no-undefined and the prefab unresolved: FAILED before, links after the fix. - normal builds (prefab linked) keep libreactnative.so in DT_NEEDED and resolve the symbol at load time -- the flag is a no-op there. - x86 / arm64 still build and stay 16 KB aligned. Fixes #6398 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs: Reference PR instead of issue in changelog entry Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(android): Add ABI coverage and link regression guard for perf-logger Two CI additions to catch the class of failures that shipped with the app-compiled libsentry-tm-perf-logger.so (#6394, #6398), both of which escaped because CI only built x86: - Build all four ABIs (armeabi-v7a, arm64-v8a, x86, x86_64) in the New Architecture sample Android build instead of x86 only, so per-ABI native breakage is exercised and the 16 KB alignment check runs across every ABI. - Add scripts/check-tm-perf-logger-link.sh, a lightweight guard that reproduces the #6398 link condition (armeabi-v7a, --no-undefined, reactnative prefab reference unresolved) using the real sources and the real link flags declared in CMakeLists.txt. It fails if -Wl,-z,undefs is ever removed. Wired into buildandtest.yml so it runs on every PR using the runner's preinstalled NDK. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(android): Build all ABIs in the Expo sample too #6398 (armeabi-v7a link failure) was reported on an Expo build, but the Expo sample only built x86 in CI. Build all four ABIs to keep coverage consistent with the bare React Native sample. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(android): Honor reactNativeArchitectures for the perf-logger native build Root-cause fix for #6398. The module hardcoded `abiFilters` to all four ABIs, ignoring the host app's `reactNativeArchitectures`. When the app builds a subset (e.g. `-PreactNativeArchitectures=arm64-v8a`, common in Expo/dev/CI builds), React Native only provides its `reactnative` prefab for that subset, so building the extra ABIs cannot resolve `TurboModulePerfLogger::enableLogging` at link time and breaks the build. Honor `reactNativeArchitectures` (falling back to all four when unset), matching react-native-screens, reanimated and the RN app template. Now the module builds only the ABIs the app requested. Verified locally against a subset build: with the old hardcoded filters, `:sentry_react-native:externalNativeBuildDebug -PreactNativeArchitectures=arm64-v8a` built arm64-v8a + armeabi-v7a + x86 + x86_64; with the fix it builds only arm64-v8a. The `-Wl,-z,undefs` link flag stays as defense-in-depth for any remaining setup where a built ABI's prefab reference is unresolved. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📢 Type of change
📜 Description
Install a Sentry-owned
facebook::react::NativeModulePerfLoggeron both platforms so the SDK observes every TurboModule lifecycle event:moduleDataCreate{Start,End},moduleCreate{Start,CacheHit,Construct*,SetUp*,End,Fail}moduleJSRequireBeginning*,moduleJSRequireEnding*syncMethodCall{Start,ArgConversion*,Execution*,ReturnConversion*,End,Fail}asyncMethodCall{Start,ArgConversion*,Dispatch,End,Fail}asyncMethodCallBatchPreprocess{Start,End}asyncMethodCallExecution{Start,ArgConversion*,End,Fail}This is the foundation that the next three issues in the Turbo Modules instrumentation project build on: JS↔Native crash attribution, per-Turbo-Module spans, and aggregated per-module stats. Each will ship its own
ISentryTurboModulePerfSinkimplementation and plug into the hook this PR exposes.New
enableTurboModuleTrackingoption onSentry.init, defaultfalsefor this first release so the foundation lands without behavioural change. The native logger is always installed (we never want to miss early lifecycle events); the flag only decides whether forwarded callbacks reach the sink. The option is plumbed throughinitNativeSdkon both platforms.💡 Motivation and Context
Closes #6162.
💚 How did you test it?
📝 Checklist
sendDefaultPIIis enabled🔮 Next steps
This is the foundation for the Turbo Modules project. Follow-up issues plug in
ISentryTurboModulePerfSinkimplementations:turbo_module.name/turbo_module.methodof the call that was in flight.duration,status,module.methoddata.