perf(backend): replace @sentry/node-core + OpenTelemetry with @sentry/core - #241
Open
gmaclennan wants to merge 13 commits into
Open
perf(backend): replace @sentry/node-core + OpenTelemetry with @sentry/core#241gmaclennan wants to merge 13 commits into
gmaclennan wants to merge 13 commits into
Conversation
…ackend Measured against real builds: the lazy sentry-init chunk shrinks from 442 KB to 84 KB minified (total backend JS -10%), import time drops ~85% and post-init heap ~5 MB, with a validated prototype (all 97 backend unit tests pass) included as an appendix. Co-Authored-By: Claude Fable 5 <[email protected]> Claude-Session: https://claude.ai/code/session_012vfe8e6oPWSUW2GZDYKdxz
…alysis Co-Authored-By: Claude Fable 5 <[email protected]> Claude-Session: https://claude.ai/code/session_012vfe8e6oPWSUW2GZDYKdxz
Co-Authored-By: Claude Fable 5 <[email protected]> Claude-Session: https://claude.ai/code/session_012vfe8e6oPWSUW2GZDYKdxz
The embedded Node backend depended on `@sentry/node-core`, which drags in
`@sentry/opentelemetry` and five `@opentelemetry/*` packages. That stack
was a 441.7 KB minified chunk parsed and evaluated on every production FGS
boot — and we used essentially none of it: no auto-instrumentations were
ever registered, the HTTP transport is replaced by the control-socket
forwarding transport, ESM loader hooks were already disabled, and fatals
go through index.js's own handleFatal handlers.
`@sentry/core` ships its own complete, OTel-free tracing implementation
(the one @sentry/browser, @sentry/deno and @sentry/vercel-edge use), and
every API the backend calls exists there. So `lib/sentry-init.js` now
assembles a minimal client out of core primitives — initAndBind on
ServerRuntimeClient, createStackParser(nodeStackLineParser()), an explicit
default-integration list — and `lib/als-async-context.js` supplies the
AsyncLocalStorage context strategy that the OTel context manager used to
provide (adapted from @sentry/vercel-edge's async.ts, MIT).
Measured on real builds of this repo:
chunks/sentry-init-*.mjs (Android) 441,707 -> 84,672 bytes (-81%)
OTel side-chunks (esm/getMachineId/execAsync/src) ~22 KB -> 0
Total Android JS 3,728,019 -> 3,350,293 (-10.1%)
Total iOS JS 3,696,613 -> 3,318,647 (-10.2%)
The external contract is unchanged. `initSentry(argv, storageDir)` and the
injected-SDK seam absorb the whole swap, so sentry.js, metrics.js,
before-send.js, sentry-frame.js, loader.mjs and index.js keep their runtime
logic. Two parity items from the plan's behaviour-deltas table are included
because they are nearly free: applySdkMetadata keeps event.sdk.name on
"sentry.javascript.node-core" so existing dashboards keep matching (the
package list names @sentry/core), and a small appContextIntegration restores
contexts.app app_start_time/app_memory, the one piece of node-core's
nodeContextIntegration nothing else supplied.
The `typeof import("@sentry/node-core")` JSDoc annotations in sentry.js and
metrics.js are replaced by a `SentrySdk` typedef exported from sentry-init.js
(a type-only reference, so no runtime edge from the always-on modules into
the lazy chunk), keeping tsc at its pre-existing baseline of 78 errors.
Accepted losses, all documented in docs/sentry-core-migration-plan.md:
outbound-HTTP breadcrumbs (httpIntegration/nativeNodeFetch), contextLines,
localVariables, and forwarding of any OpenTelemetry spans @comapeo/core might
emit in future. No spans are lost — every span we ship is created by hand.
Backend unit tests: 100/100 (the existing 97 plus three covering concurrent
scope isolation across awaits in the new async-context strategy).
Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_012vfe8e6oPWSUW2GZDYKdxz
Targeted edits to the statements the core migration makes false, rather than a rewrite of these docs. - sentry-integration.md §5.1: pinned versions are now `@sentry/core@^10` alone; describe the SDK as assembled from core primitives and link the migration plan for the accounting. §5.6 (OpenTelemetry forwarding of @comapeo/core PR #1051 spans) is marked not available, with the cheap route back if it ever becomes worth it. Goal 3 struck for the same reason. §4.2 init-order bullet no longer claims import-in-the-middle patches modules, and the lazy-chunk bullet names the real dynamic import. Architecture diagram, the `debug` toggle table and the privacy-tier table no longer list OTel spans. - ARCHITECTURE.md §7.2: `layer:node` is the core-only SDK assembled in backend/lib/sentry-init.js, not `@sentry/node`. - sentry-core-migration-plan.md: status moves from proposal to implemented, with the reproduced measurements and the two validation steps still outstanding (tripwire run, on-device boot benchmark). The two behaviour-delta rows for the optional parity items now record that both were taken, and the appendix is framed as the historical prototype rather than the current source. Pre-existing drift in §5.1's bundle-layout tree (importHook.js and lib/register.js, removed before this change) is left alone. Co-Authored-By: Claude Fable 5 <[email protected]> Claude-Session: https://claude.ai/code/session_012vfe8e6oPWSUW2GZDYKdxz
Review follow-ups on the @sentry/core migration. Two were real behaviour regressions that the bundle-size work introduced silently. Client reports. `@sentry/node-core` defaulted `sendClientReports` on and ran `startClientReportTracking()`; core gates `recordDroppedEvent` on that option and — unlike the browser and node clients — never calls `_flushOutcomes` itself. The result was that the node layer's discarded-event stats (events dropped by `scrubEvent`, by sampling, by rate limits) vanished with no signal at all. Restored: `sendClientReports` defaults to true again, and a five-line `ServerRuntimeClient` subclass hangs `_flushOutcomes` off the client's public `flush` hook. Subclassing is how the upstream SDKs reach that protected method. The existing shutdown `sentry.flush()` therefore drains outcomes, and `close()` does too since it flushes first. We deliberately skip node-core's other trigger, a 60s `setInterval`: a perpetual timer in a foreground service tuned for low-memory devices is a poor trade for a report only read in aggregate. Verified end to end — two sample-rate-dropped events produce a `client_report` envelope with `discarded_events` through the existing forwarding transport. Stack-frame `module`. node-core built its stack parser as `nodeStackLineParser(createGetModuleFromFilename())`; we passed no getModule, so frames carried no `module` attribute. Sentry's default grouping fingerprints on module+function, so this would have re-fingerprinted existing backend issues as new ones. `@sentry/core` does not export that helper, so it is reimplemented locally — only node-core's live branch, since its `/node_modules/` branch is dead here (the bundle ships no runtime JS under node_modules, only json/sql/smp data) and so is its Windows handling. Tests: the three load-bearing tracing paths now have shape assertions rather than presence-only ones, since the tracing backend changed and shape is what such a swap gets wrong quietly — trace continuation and parenting in `rpcHook`, explicit `parentSpan` parenting in `withBootTrace`, and the sync-session sampling exception at base rate 0. Plus a regression test for the client-report path. 97 -> 104 tests. Also, smaller: drop the now-inert `registerEsmLoaderHooks` option (nothing reads it); drop the redundant `initialScope` update in `init` (initAndBind already does it); prune `getClient`/`captureMessage`/`close` from the injected namespace since nothing calls them; correct the `stackParser` JSDoc, the `als-async-context` header (vercel-edge no longer ships the file it was adapted from) and the `withBootTrace` parenting comment; and document the deliberate by-reference current scope in the two isolation-scope strategy methods. Co-Authored-By: Claude Fable 5 <[email protected]> Claude-Session: https://claude.ai/code/session_012vfe8e6oPWSUW2GZDYKdxz
The dependency removal was first applied with the npm 10 in this environment, which silently dropped six `"libc"` fields from optional platform binaries — the repo's devEngines pins npm ^11.16.0, so that was unrelated churn riding along in the diff. Regenerated by restoring the pre-migration lockfile and re-running `npm@11 install --package-lock-only`. The diff is now a pure removal (268 deleted lines, zero additions) of @sentry/node-core, @sentry/opentelemetry, the six @opentelemetry packages and their transitive deps. `npm ci` still resolves cleanly under npm 10. Co-Authored-By: Claude Fable 5 <[email protected]> Claude-Session: https://claude.ai/code/session_012vfe8e6oPWSUW2GZDYKdxz
…ded build Review follow-ups; all four were statements that had become false. sentry-integration.md §5.1's bundle-layout tree still described `importHook.js`, `lib/register.js` and a path-rewrite plugin — none of which exist any more — and collapsed the two chunks matching `chunks/sentry-*.mjs` into one row, which is actively misleading because they have opposite load semantics: the ~10 KB adapter chunk (sentry.js, metrics.js, before-send.js, node-resources.js) is statically imported by both entries and is always loaded, and contains no @sentry/* code at all; the ~85 KB `sentry-init` chunk is @sentry/core and loads only behind the DSN check. They are now separate rows, and the bundle-size figures below the tree are the real ones rather than the pre-split estimate. sentry-core-migration-plan.md carried two different sets of numbers for the same artifacts — the landed figures in the status block against the prototype's in the measured-savings table. The table is now the landed build throughout, expanded so every artifact is listed and the column sums are checkable against the totals. Footnote 1 claimed index.mjs grew because rolldown re-hoisted @sentry/core bits into it; that is false — index.mjs contains no @sentry/* code before or after (verified by grep on the built output). What actually happened is that a shared `src-*.mjs` chunk lost one of its two consumers and got inlined into the other, which is why the growth matches its size almost exactly. Co-Authored-By: Claude Fable 5 <[email protected]> Claude-Session: https://claude.ai/code/session_012vfe8e6oPWSUW2GZDYKdxz
… shape
The op assertion expected "comapeo.boot" but SentryFgsBridge starts the
transaction as TransactionContext("comapeo.boot", "boot") — all 894
production boot transactions in the last 30 days carry op "boot". The
device.family assertion expected "Android" but sentry-java reports the
device model (883/894 production events); keep only the "Google"
processor-not-running detector and a missing-context check.
Also drop apps/integration's tsconfig paths alias into src/ — same
double-load hazard apps/e2e already documents, and Metro cannot resolve
src's ESM-style ".js" imports when release-bundling, which broke
assembleRelease.
Tripwire green on both pre- and post-migration release builds on a Pixel_7a_API_34 emulator; boot.loader-import-sentry-node p50 217 ms -> 23 ms (-89%), consistent with the fleet baseline (p50 101 / avg 195 / p95 567 ms on production node-core) and the desktop prediction.
…re-bundle-size-nzic8m * origin/main: chore(deps): bump @comapeo/map-server Release v1.0.0-pre.12 chore(deps): bump @comapeo/map-server test(e2e): surface stalled spec and capture BrowserStack diagnostics perf(backend): use Node's built-in undici instead of bundling 6.x fix(backend): flush control frames before closing IPC sockets refactor: address review on the node 24 environment changes test(ios): fail the build if the SIMD llhttp payload reaches the iOS bundle fix(ios): keep aliasing undici's SIMD llhttp away from polywasm fix: move better-sqlite3 to 13.x so it stops aborting on device chore(deps-dev): bump rolldown perf: fetch the lite nodejs-mobile flavour feat: run the backend on nodejs-mobile 24 (Node 24.19.0) # Conflicts: # backend/package-lock.json # backend/rolldown.config.ts
The branch now sits on top of nodejs-mobile 24, the V8 compile cache and Node's built-in undici, so both the bundle figures and the boot benchmark were taken again with baseline and migrated release APKs built from that same toolchain. Bundle bytes are read out of the two APKs: the sentry-init chunk is 441,707 -> 84,800 and total Android JS 3,332,488 -> 2,954,854 (-11.3%). On device, across 38 warm boots per variant, the SDK load+init span drops 1118 -> 200 ms, FGS peak RSS 245.2 -> 237.8 MB and PSS 107.0 -> 103.0 MB; with the compile cache wiped, PSS drops 107.4 -> 101.5 MB. Only about 0.26 s of the 0.9 s saved in the SDK import reaches total boot time, because the index import that follows runs longer on the core-only build; that observation is recorded rather than explained away.
The ALS strategy passed the caller's current scope through by reference while forking the isolation scope, so a current-scope mutation inside a withIsolationScope callback would have escaped to the caller. Nothing in backend/ does that today, but SDK v11's own AsyncLocalStorage strategy (@sentry/server-utils) clones it, as did the OpenTelemetry context manager node-core installed, so match them rather than keep a divergence that only a comment defends. Costs 19 bytes in the built chunk and one Scope.clone() on a path the backend does not currently take.
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
2 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.
Replaces the backend's Sentry SDK stack (@sentry/node-core + @sentry/opentelemetry + five @opentelemetry/* packages) with @sentry/core only. The node-core stack was a 442 KB minified chunk parsed on every production FGS boot, almost all of it dead under our configuration: we register no OTel instrumentations, use a custom IPC forwarding transport, and handle fatals ourselves. @sentry/core ships its own OTel-free tracing implementation (the same one @sentry/deno and @sentry/vercel-edge use), so the swap is a ~170-line adapter behind the existing injected-SDK seam: a new AsyncLocalStorage context strategy plus a rewritten sentry-init.js. No changes to sentry.js runtime logic, the IPC contract, or CLI flags.
Results, re-measured after merging main (nodejs-mobile 24, V8 compile cache, undici from Node) with both variants built from that same toolchain: the lazy sentry-init chunk drops 441,707 → 84,819 bytes (−81%), the OTel side-chunks disappear, and total Android JS shrinks 3,332,488 → 2,954,873 bytes (−11.3%). Restored parity items: client reports (via a small ServerRuntimeClient subclass on core's flush hook), stack-frame module attribution, contexts.app, and sdk.name stays sentry.javascript.node-core so dashboards keep matching.
Validation: 110/110 backend unit tests (7 new: async-context isolation, trace shape, client reports), and the tripwire passes on the post-merge build. On device (Pixel_7a_API_34 emulator, release builds of apps/integration, A/B interleaved over five blocks, 38 warm boots per variant), the SDK load+init span drops 1118 ms → 200 ms median, FGS peak RSS 245.2 → 237.8 MB and PSS 107.0 → 103.0 MB; with the V8 compile cache wiped, PSS drops 107.4 → 101.5 MB. Total boot-to-ready improves ~4% (CPU time 6.12 → 5.86 s), less than the SDK-import saving alone, because the index import that follows runs ~0.6–0.9 s longer on the core-only build — measured twice by independent means, not a compile-cache miss, and recorded as an open question in the doc. Full measurements, byte attribution, behavior deltas, and methodology are in docs/sentry-core-migration-plan.md.
Also included: two stale tripwire assertions corrected (op and device.family contradicted all 894 production boot transactions from the last 30 days; the pre-migration build passes the corrected assertions), and apps/integration's tsconfig paths alias removed (double-load hazard apps/e2e documents, and it broke release bundling on current Metro).