feat(cloudflare): Auto-register Flue instrumentation in bundled workers - #24476
RulaKhaled wants to merge 3 commits into
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 😅
| 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.
| 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.
| 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 }) }; | |
| }, |
| '(globalThis.__SENTRY_ORCHESTRION__ = globalThis.__SENTRY_ORCHESTRION__ || {});\n' + | ||
| '(globalThis.__SENTRY_ORCHESTRION__.providedModules = globalThis.__SENTRY_ORCHESTRION__.providedModules || {});\n' + | ||
| `Object.defineProperty(globalThis.__SENTRY_ORCHESTRION__.providedModules, '${FLUE_MODULE}', ` + | ||
| `{ configurable: true, get() { return ${PROVIDER_IDENTIFIER}; } });\n`; |
There was a problem hiding this comment.
| `{ configurable: true, get() { return ${PROVIDER_IDENTIFIER}; } });\n`; | |
| `{ configurable: true, enumerable: true, get() { return ${PROVIDER_IDENTIFIER}; } });\n`; |
| export { langChainIntegration } from './integrations/langchain'; | ||
| export { langGraphIntegration } from './integrations/langgraph'; | ||
| export { createFlueInstrumentation } from './ai/flue'; | ||
| export { flueIntegration } from './integrations/flue'; |
There was a problem hiding this comment.
This is exported here, should it be exported from cloudflare as well?
Same with the FlueOptions type.
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.