feat(cloudflare): Auto-register Flue instrumentation in bundled workers - #24476
Conversation
Flue is registered, not patched — `instrument()` writes into module-scope state — so instrumenting it needs a reference to that module's own binding, and no channel payload carries one. On Node the user supplies it by calling `instrument()` themselves, which stays the only route there. In a bundled worker there is no `node_modules` to resolve one from, so it is supplied at build time instead. Two halves, mirroring how Mastra reaches a worker: - `flueIntegration()` registers the instrumentation when the `@flue/runtime` namespace is on the orchestrion marker, and no-ops when it is not. A `registrationOnly` orchestrion entry is what installs it on a bundler-only SDK: evaluating `@flue/runtime` registers the factory on the marker. That also keeps the integration reachable under `sideEffects: false`, which would otherwise let the bundler drop the module and the registration with it. - `@sentry/cloudflare/vite` splices a static `@flue/runtime` import into Sentry's own Flue integration module and exposes the namespace on `providedModules`. Two things the Mastra provider does not have to handle. `@flue/runtime` is ESM-only, so `createRequire().resolve()` throws `ERR_PACKAGE_PATH_NOT_EXPORTED` on it and the existence check goes through the ESM resolver. And the namespace is exposed through a getter rather than assigned: the snippet is prepended to Sentry's module, which the bundler may evaluate before `@flue/runtime` is initialized, so assigning it stores `undefined` — the key lands on `providedModules` with nothing behind it. An app that also calls `instrument()` itself is unaffected: its own registration wins and the integration swallows the resulting `InstrumentationAlreadyInstalledError`. Node is unchanged. `moduleInjectedTransforms` is wired into the bundler paths only, and Sentry stays external in a Flue node build, so neither half applies there and `flueIntegration()` installs as a no-op. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
size-limit report 📦
|
…ation The build-time presence check used `import.meta.resolve(spec, parentURL)`. The `parentURL` argument is ignored without `--experimental-import-meta-resolve`, so the check resolved from Sentry's own install rather than the app's, and it compiles to `undefined(...)` in this package's CJS build, where it threw and fell through to a `createRequire` fallback that always fails for an ESM-only package. Injection was therefore skipped outright on the CJS path and wherever Sentry is not installed beneath the app. It now resolves with `createRequire` from the Vite root and counts `ERR_PACKAGE_PATH_NOT_EXPORTED` as a hit: `@flue/runtime` publishes no `require` condition on any subpath, so that error means the package is present, while a missing one reports `MODULE_NOT_FOUND`. Also narrows the registration catch to `InstrumentationAlreadyInstalledError` so a changed `instrument()` contract surfaces instead of becoming a debug log, bounds the supported range at `<3.0.0`, drops the unused `flueModuleNames` export, and removes `flueIntegration()` from the default integrations — it has no binding to read on Node, where registering stays a manual `instrument(Sentry.createFlueInstrumentation())` call. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
RulaKhaled
changed the title
|
bugbot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ 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 97b7ed7. Configure here.
…try.init()` Core calls `integration.setup()` unguarded, and Cloudflare runs `Sentry.init()` inside the request wrapper, so rethrowing an unexpected `instrument()` failure would take down the handler — and every later request, since the client is never cached. A duplicate registration stays a debug log; anything else now warns that Flue spans will not be recorded, which keeps the failure visible without making it fatal. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
isaacs
left a comment
isaacs
left a comment
There was a problem hiding this comment.
This is good :)
Breaking the import cycle with a getter is a good approach. Build-time presence detection avoids forcing @flue/runtime into every worker.
The only concern (which can be put off to a follow-up easily enough) is the overlap with Mastra, which is already drifting in a few spots (albeit pretty minor), so would probably be good to consolidate.
| @@ -0,0 +1,71 @@ | |||
| import { createRequire } from 'node:module'; | |||
There was a problem hiding this comment.
Main issue/comment I'd make for this PR: this file is nearly identical to the packages/cloudflare/src/vite/mastraObservability.ts file, except for the module specifier, the identifier, the target regex, the tolerated resolve error, and getter versus assignment.
Suggestion: extract one factory, eg createProvidedModulePlugin({ name, moduleName, identifier, targetId, lazy }), and let both call sites shrink to a few lines. That also gives one place to fix any other concerns for both packages.
Also, I notice that Mastra's plain catch { return; } works today only because @mastra/observability still publishes a require condition. If it goes ESM-only, that provider silently stops injecting, with the same symptom this branch just fixed for Flue. A shared check removes that potential future bug, and lets us improve both in one place.
There was a problem hiding this comment.
By the way, this can definitely be put off for a future PR, I just think we should probably get to it before there's a third one of these, and we start having a harder time deciding which drifting behavior is correct 😅
There was a problem hiding this comment.
will take it up on a follow up. you're right about the mastra half, it isn't hypothetical either, @mastra/observability@1.17.4 still ships a require condition, so mastraObservability.ts:48's bare catch { return; } works by luck, an esm only release turns injection off silently
fixed the flue side here and left mastra untouched so both land together with the rafactor
| try { | ||
| createRequire(resolve(config.root, 'noop.js')).resolve(FLUE_MODULE); | ||
| } catch (error) { | ||
| if ((error as NodeJS.ErrnoException | undefined)?.code !== 'ERR_PACKAGE_PATH_NOT_EXPORTED') { | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
Based on the comment above, it seems safer to detect module not found rather than "anything other than path not exported"?
| try { | |
| createRequire(resolve(config.root, 'noop.js')).resolve(FLUE_MODULE); | |
| } catch (error) { | |
| if ((error as NodeJS.ErrnoException | undefined)?.code !== 'ERR_PACKAGE_PATH_NOT_EXPORTED') { | |
| return; | |
| } | |
| } | |
| try { | |
| createRequire(resolve(config.root, 'noop.js')).resolve(FLUE_MODULE); | |
| } catch (error) { | |
| const code = (error as NodeJS.ErrnoException | undefined)?.code; | |
| if (code === 'MODULE_NOT_FOUND' || code === 'ERR_MODULE_NOT_FOUND') { | |
| return; | |
| } | |
| } |
There was a problem hiding this comment.
Oh, also, it'd be a bigger refactor, but I think if we have a rollup context, we can do await this.resolve(FLUE_MODULE, resolve(root, 'noop.js')) on it to get a more definitive answer, regardless of export type. That would drop createRequire, node:path and the error-code special case entirely.
There was a problem hiding this comment.
this.resolve() version is def nicer, I'll do it once with the refactor
| try { | ||
| instrument(createFlueInstrumentation(options)); | ||
| } catch (error) { | ||
| // Never rethrow: `setup()` runs inside `Sentry.init()`, which core calls unguarded and | ||
| // Cloudflare calls per request, so throwing here would take down the request handler. | ||
| if ((error as Error | undefined)?.name === 'InstrumentationAlreadyInstalledError') { | ||
| DEBUG_BUILD && debug.log('[Flue] already instrumented by the app; skipping auto-registration'); | ||
| } else { | ||
| debug.warn('[Flue] auto-registration failed; Flue spans will not be recorded:', error); | ||
| } | ||
| } |
There was a problem hiding this comment.
In dev, a repeat instrument() under the same key doesn't throw, and instead disposes the previous registration. Sentry's dispose() (in packages/server-utils/src/ai/flue/index.ts) ends every tracked turn and tool span and clears all three maps.
Cloudflare calls Sentry.init() per request. With the default cacheClient: true the cached client short-circuits before setup() reruns, so this doesn't fire.
With cacheClient: false, or any path that bypasses the cache, setup() runs per request.
Under vite dev that means every request ends the in-flight turn and tool spans of every concurrent request, and the fresh registration starts with empty maps so those spans are then orphaned, and nothing throws or is logged.
Suggesgtion: guard the call with a module-scope flag, for example let registered = false; set after a successful instrument(). One registration per isolate is all the design wants, and the flag also avoids allocating two 1000-entry LRUMaps per request just to throw them away on the production path.
There was a problem hiding this comment.
Very nice. I guarded this on the binding rather than a bare boolean, so a fresh isolate still registers
| transform(code: string, id: string): { code: string; map: ReturnType<MagicString['generateMap']> } | undefined { | ||
| if (!providerSnippet || !isFlueIntegrationModuleId(id)) return undefined; | ||
|
|
||
| const ms = new MagicString(code); | ||
| ms.prepend(providerSnippet); | ||
| return { code: ms.toString(), map: ms.generateMap({ hires: true }) }; | ||
| }, |
There was a problem hiding this comment.
This is not idempotent, because the ms.prepend unconditionally adds the snippet.
If transform ever sees the same module twice in one environment, the output carries two import * as __SENTRY_FLUE_RUNTIME__ statements, which is a duplicate binding and a syntax error. Vite's per-environment module graphs make it unlikely, but it's a potential future hazard.
(Note: same thing in the Mastra plugin, probably another reason to consider consolidating them.)
| transform(code: string, id: string): { code: string; map: ReturnType<MagicString['generateMap']> } | undefined { | |
| if (!providerSnippet || !isFlueIntegrationModuleId(id)) return undefined; | |
| const ms = new MagicString(code); | |
| ms.prepend(providerSnippet); | |
| return { code: ms.toString(), map: ms.generateMap({ hires: true }) }; | |
| }, | |
| transform(code: string, id: string): { code: string; map: ReturnType<MagicString['generateMap']> } | undefined { | |
| if (!providerSnippet || !isFlueIntegrationModuleId(id) || code.includes(PROVIDER_IDENTIFIER)) { | |
| return undefined; | |
| } | |
| const ms = new MagicString(code); | |
| ms.prepend(providerSnippet); | |
| return { code: ms.toString(), map: ms.generateMap({ hires: true }) }; | |
| }, |
There was a problem hiding this comment.
I couldn't find a config where vite actually re-transforms its own output, but added it anyway
left the mastra copy as is so it lands with the factory
Cloudflare calls `Sentry.init()` per request. With `cacheClient: false` the cached client no longer short-circuits, so `setup()` reran and called `instrument()` again on every request. Flue only throws on a duplicate key when `!isDevMode()`, and `@flue/vite` generates the entry with `devMode: import.meta.env.DEV`. Under `vite dev` the repeat disposes the previous registration instead, and our `dispose()` ends every tracked turn and tool span, so each request killed the in-flight spans of every concurrent one with nothing thrown or logged. Keyed on the provided binding rather than a bare boolean, so a new isolate or a freshly provided namespace still registers. Set on the `InstrumentationAlreadyInstalledError` branch too: the app owns the key and we will never win it, so retrying only rebuilds two 1000-entry `LRUMap`s per request. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review fixes, all in the Vite provider plugin. Treat only a module-not-found as absent. The old check skipped injection on any code other than `ERR_PACKAGE_PATH_NOT_EXPORTED`, so an installed package that failed to resolve some other way lost instrumentation silently. A malformed `package.json` resolves with `code === undefined` and hit exactly that. Injecting instead hands the failure to Vite, which reports it. Guard `transform` on the injected identifier. `ms.prepend` ran unconditionally, so a second pass over its own output would emit a duplicate `import * as` binding, which is a syntax error. Make the provider property enumerable, matching the Mastra plugin's plain assignment, so it survives a spread or `Object.keys`. Also re-export the `FlueOptions` type, which pairs with the `createFlueInstrumentation` this package already exports. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`flueRuntime.ts` and `mastraObservability.ts` were near-identical, differing
only in the module specifier, the injected identifier, the target regex, how
they handled a failed resolve, and getter versus assignment. Both call sites are
now a few lines over a shared `createProvidedModulePlugin`.
Replace the `createRequire().resolve()` probe with the Rollup context's
`this.resolve()`, which answers with the same resolver and conditions the
injected import will use. That drops `createRequire`, `node:path` and the
error-code special case, and it fixes a latent bug on the Mastra side: its bare
`catch { return; }` worked only because `@mastra/observability` still publishes
a `require` condition, so an ESM-only release would have turned injection off
with no error and no log. The probe needs a plugin context, so it moves from
`configResolved` to `buildStart`. `configResolved` stays to capture the app
root.
Resolution runs per environment against a shared plugin instance, so the probe
stops once it finds the package and retries in the next environment otherwise.
Only the worker environment ever reaches `transform`, and it may not run first.
Mastra also picks up the two guards flue gained in #24476: `transform` is now
idempotent, and a resolver error injects rather than silently skipping.
Mastra keeps plain assignment. Switching it to flue's lazy getter is probably
right for the same reason flue needs one, but it is a behavior change and worth
deciding on its own.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`flueRuntime.ts` and `mastraObservability.ts` were near-identical, differing
only in the module specifier, the injected identifier, the target regex, how
they handled a failed resolve, and getter versus assignment. Both call sites are
now a few lines over a shared `createProvidedModulePlugin`.
Replace the `createRequire().resolve()` probe with the Rollup context's
`this.resolve()`, which answers with the same resolver and conditions the
injected import will use. That drops `createRequire`, `node:path` and the
error-code special case, and it fixes a latent bug on the Mastra side: its bare
`catch { return; }` worked only because `@mastra/observability` still publishes
a `require` condition, so an ESM-only release would have turned injection off
with no error and no log. The probe needs a plugin context, so it moves from
`configResolved` to `buildStart`. `configResolved` stays to capture the app
root.
Resolution runs per environment against a shared plugin instance, so the probe
stops once it finds the package and retries in the next environment otherwise.
Only the worker environment ever reaches `transform`, and it may not run first.
Mastra also picks up the guards Flue gained in #24476: `transform` is
idempotent, a resolver error injects rather than silently skipping, and the
namespace now goes behind the same lazy getter. Assigning reads the binding at
injection time, so it stores `undefined` whenever the bundler evaluates Sentry's
module first. That hazard is not specific to Flue, so both providers use one
shape and the `lazy` option is gone.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Stacked on #24476 — review that first. A `flue init` Cloudflare app with no `instrument()` call anywhere: registration comes from the build, so any `gen_ai` span here is itself proof the auto-wiring worked. Covers AI spans, tool-error capture, a manual span nesting under its tool, and an orchestrion `dataloader` span in the agent's trace. Secrets go through `.dev.vars` rather than `--var` because Flue resolves the provider key inside `pi-ai` at runtime, leaving nothing for Vite to inline, and `vite preview` is what serves the worker with Flue's generated Durable Object config. Two Flue constraints the app works around, both commented in place: `agents` is imported by Flue's generated worker entry without being declared, so it only resolves under npm's hoisting and needs declaring for pnpm; and the `'use agent'` scan parses every source file as plain JavaScript, so generics and return types fail the build. _Worth a docs note_: the Sentry wrapper has to be re-exported as `cloudflare` from the agent module. Defining it elsewhere leaves the Durable Object unwrapped — the agent runs, turns settle, and nothing is traced. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
2 participants
Flue is registered, not patched —
instrument()writes into module-scope state — so instrumenting it needs a reference to that module's own binding, and no channel payload carries one. A bundled worker has nonode_modulesto resolve one from, so this supplies it at build time.@sentry/cloudflare/vitesplices a static@flue/runtimeimport into Sentry's own Flue integration module and exposes the namespace onprovidedModules;flueIntegration()reads it there and registers. AregistrationOnlyorchestrion entry installs the integration on a bundler-only SDK and keeps it reachable undersideEffects: false.@flue/runtimeis ESM-only, so the presence check resolves withcreateRequirefrom the Vite root and countsERR_PACKAGE_PATH_NOT_EXPORTEDas a hit — the package publishes norequirecondition on any subpath, while a genuinely missing one reportsMODULE_NOT_FOUND. The namespace is exposed through a getter rather than assigned, because the bundler may evaluate Sentry's module before@flue/runtimeis initialized.An app that also calls
instrument()itself is unaffected: its own registration wins, and only the resultingInstrumentationAlreadyInstalledErroris swallowed. On Node registering stays a manualinstrument(Sentry.createFlueInstrumentation())call —flueIntegration()is not among the default integrations there.Verified end to end in #24477.