feat(core): Aggregate TurboModule call counts and latency per (module, method, kind)#6377
feat(core): Aggregate TurboModule call counts and latency per (module, method, kind)#6377alwx wants to merge 5 commits into
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. |
…, method, kind) Adds a small fixed-bucket histogram + counters per `(module, method, kind)` fed by the existing `wrapTurboModule` instrumentation. Aggregates flush on transaction finish (synthetic `turbo_modules.aggregate` child span + headline measurements on the root span) and on a lazy timer (info-level event for long-running sessions without transactions). Closes #6164. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Drop edge-case tests in favor of one happy-path check per surface. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
alwx
force-pushed
the
alwx/features/aggregated-stats
branch
from
933fdb9 to
5702b46
Compare
|
![sentry-warden[bot]](https://avatars.githubusercontent.com/in/2755838?s=60&v=4){kind=small}
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
The periodic flush's own `captureEvent` travels through the wrapped `RNSentry.captureEnvelope`, which records back into the aggregator and, because the map was just drained, re-armed the flush timer forever in otherwise-idle sessions. Suppress the empty→non-empty callback around the flush's `captureEvent`, deferring the release to the next macrotask so the async record fired from the transport's `.then()` also lands inside the suppression window.
`enableAggregateStats: false` skipped flush setup and processEvent but `wrapTurboModule` still recorded every call into the process-wide map, which had no drain path in that mode — so counters grew for the app's lifetime. Aggregator now exposes a master `setAggregateRecordingEnabled` switch that both no-ops future records and evicts existing entries. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
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.
Reviewed by Cursor Bugbot for commit 335716a. Configure here.
| }); | ||
| } finally { | ||
| setTimeout(endSuppressFirstTurboModuleRecordCallback, 0); | ||
| } |
There was a problem hiding this comment.
Suppressed flush kills timer re-arm
Medium Severity
When flush self-noise from captureEvent is recorded under suppression, those entries stay in the aggregator. The next real TurboModule call then sees a non-empty map, so onFirstRecordAfterEmpty never runs and the lazy periodic timer is never re-armed. After the first suppressed flush, periodic aggregates stop until a transaction drain clears the leftover.
Additional Locations (1)
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit 335716a. Configure here.
|
@alwx using some agents locally, it spotted the following, would you be able to verify on your side? Trace this in order:
Step 4 — the payoff: what happens to the next real call Now the map isn't empty anymore — it has that one leftover RNSentry.captureEnvelope entry sitting in it, untouched, forever (or until something else drains it). The next time a real TurboModule call happens (e.g. the user's own native module call), go back to Step 1, line 131: const wasEmpty = aggregates.size === 0; // FALSE — the leftover entry is still there wasEmpty is false, so the whole if (wasEmpty && onFirstRecordAfterEmpty && ...) block at line 164 is skipped — not because of suppression this time, but because the map genuinely isn't empty. The callback that arms the next 30-second timer never fires again. End state: periodic flush fired exactly once, quietly planted one leftover entry as its own exhaust, and that leftover permanently blocks the "empty → non-empty" trigger from ever detecting fresh work again — unless a transaction happens to fire processEvent and drain the map back to zero (which restarts the cycle, but only for one more round). The fix in one line: at aggregator line 164, also gate the write on suppression, not just the callback — i.e. bail out at the top of recordTurboModuleCall when suppressFirstRecordCallbackDepth > 0, so the periodic flush's own self-call never gets recorded as if it were real user traffic. |
2 participants
📢 Type of change
📜 Description
Adds per-
(module, method, kind)aggregation for TurboModule invocations on top of the existingwrapTurboModuleinstrumentation. Aggregates flush at two points:turbo_modules.aggregatechild span on the event carries the per-method breakdown in span attributes; headline measurements (turbo_modules.call_count,.error_count,.total_ms,.top_module_ms) land on the root span for the standard Measurements panel.info-level event taggedevent.kind=turbo_modules.aggregateso idle sessions without transactions still produce a signal. Armed only on the first record after a drain, so silent sessions don't churn timers.New
turboModuleContextIntegrationoptions:enableAggregateStats(defaulttrue),aggregateFlushIntervalMs(default30000,0disables),ignoreTurboModules(excludes modules from counters; still wrapped for crash attribution).O(1) per call: counters + 6-bucket histogram (
<1ms, <5ms, <20ms, <100ms, <500ms, >=500ms). Failures inside the aggregator never break the wrapped TurboModule call.💡 Motivation and Context
Closes #6164. Per-call spans for every TurboModule invocation would explode span counts on hot async paths; aggregated counters are the right tradeoff.
💚 How did you test it?
📝 Checklist
sendDefaultPIIis enabled🔮 Next steps