Conversation
size-limit report 📦
|
1e729ec to
74f5d86
Compare
d3071cb to
bef613e
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
bef613e to
8941894
Compare
| /** | ||
| * Per-request Context hook: the heart of the automatic instrumentation. | ||
| * | ||
| * `#dispatch` builds `new Context(req, { matchResult })` before its single-handler fast-path check, | ||
| * passing the live `matchResult` array. We: | ||
| * 1. wrap the already-matched MIDDLEWARE handlers (arity ≥ 2) for spans — route handlers (arity < 2) | ||
| * are covered by the request span and left as-is; | ||
| * 2. prepend the Sentry request/response middleware, so it runs first in the composed chain. That | ||
| * both drives route naming / request data / error capture (from inside the chain, with the | ||
| * Context) and forces the ≥2-handler `compose` path, so there is no fast-path gap. | ||
| * | ||
| * All of this runs per request, so it works on Cloudflare (no module-scope publish) and needs no | ||
| * app-instance patching or app-construction hook. | ||
| */ |
There was a problem hiding this comment.
This comment is not up-to-date with the "detection" code anymore.
There was a problem hiding this comment.
I updated comments generally!
There was a problem hiding this comment.
Should we maybe clear the flags (HONO_REQUEST_HANDLED and HONO_SHOULD_HANDLE_ERROR) here after the response?
There was a problem hiding this comment.
this should not be necessary, because:
- When there's a per-request isolation scope, getRequestScope stores them there. The SDK forks a fresh isolation scope per request, so the object (and its flags) is discarded when the request ends.
- When there isn't one (isolationScope === getDefaultIsolationScope()), it deliberately falls back to the Hono Context, not the default scope — and Hono builds a new Context per dispatch. Also per-request.
There was a problem hiding this comment.
That's true for most scenarios (so it's fine to keep it) but for cases where there's a user-forked isolation scope in for example Cloudflare scheduled handlers (also email and queue).
That's an edge case that can be handled in another PR.
There was a problem hiding this comment.
pushed a change to clean this up!
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
isaacs
left a comment
isaacs
left a comment
There was a problem hiding this comment.
I think this is the right approach overall.
The duplicate error issue is worth fixing before landing. Apart from that, three trade-offs are made (likely correctly!) which should be called out:
- Every Hono request will now takes the
composepath instead of Hono's single-handler fast path, even with tracing disabled, because the integration is ingetErrorIntegrations(). That's because we unshift the Sentry middleware (honoIntegration.ts line 160) so a one-handler route now has two handlers, so it always takes the compose path. (Not sure this is possible to avoid, tbh.) c.req.matchedRoutesandc.req.routeIndexchange shape for user code.- Cloudflare loses connection-info attributes relative to the manual
@sentry/honomiddleware. (I'm not sure this is necessary, there may be a way to fix this, mentioned in the comments recreateRequirebelow.)
| // oxlint-disable-next-line typescript/no-deprecated | ||
| setupFastifyErrorHandler, | ||
| firebaseIntegration, | ||
| honoIntegration, |
There was a problem hiding this comment.
Is there a reason why elysia, remix, solidstart, and sveltekit only get honoIntegration, and not honoMiddleware?
If so, we should call it out. If not, we should probably make it consistent.
| * Resolves the runtime's `getConnInfo` helper once, best-effort. | ||
| * | ||
| * Cloudflare Workers can't `require()` out of a bundle at runtime, so there conn-info is left to the | ||
| * platform's `requestDataIntegration`. On Node/Bun/Deno a missing optional peer dependency degrades |
There was a problem hiding this comment.
so there conn-info is left to the platform's
requestDataIntegration.
I don't think that's true? Maybe that's fine, because it's intended to be best effort. But it looks like cloudflare just gets dropped (returns undefined), and no conn-info is added, because the requestDataIntegration doesn't set client.address/client.port span attributes.
The Hono SDK does this with static imports, like import { getConnInfo } from 'hono/cloudflare-workers'; in packages/hono/src/cloudflare/middleware.ts.
Suggestion: add an optional getConnInfo to HonoIntegrationOptions (CreateHonoRequestMiddlewareOptions already has the field, see packages/server-utils/src/integrations/hono/createHonoMiddleware.ts line 23). Then a Cloudflare user, or packages/cloudflare itself, can pass getConnInfo from hono/cloudflare-workers with a static import, and the createRequire fallback can shrink to Node only or go away.
There was a problem hiding this comment.
Clanker says:
- Hono's Cloudflare getConnInfo is trivial — it does nothing but read one header:
(c) => ({ remote: { address: c.req.header('cf-connecting-ip') } })
No socket, no port, no transport. Same for cloudflare-pages. - We don't need require at all. The current avoidance is about createRequire in workerd — but since the CF helper is just a header read, we can inline it. That's fully static and workerd-safe, so it doesn't
touch the "lazy loading is a last resort" rule. - The IP is already captured on Cloudflare. The CF SDK runs requestDataIntegration() (baseSdk.ts:62), whose getClientIPAddress header list includes CF-Connecting-IP (vendor/getIpAddress.ts:12). It sets
user.ip_address on both the event and the span. So the existing code comment ("left to the platform's requestDataIntegration") is accurate.
So if I follow correctly, everything this would set (ip address) is already set, so this is ok to just be omitted?
| honoIntegration, | ||
| honoMiddleware, |
There was a problem hiding this comment.
This pulls in node:module, but wouldn't if we avoid using createRequire. I'm not sure how big a deal that is, since we require a pretty recent nodejs compat setting anyway, but I seem to recall that we avoided that in the past for some reason. Bundling maybe? I forget; @timfish might know.
There was a problem hiding this comment.
In this scenario it should be ok. Importing should be fine - on Cloudflare we bail out before we call it - so all cool. We didn't import back then because we only had nodejs_als - that's a rough guess but I can't think any other reason.
| // forwarded defaults and the two never stack. | ||
| const INTEGRATION_NAME = 'Hono' as const; | ||
|
|
||
| const INTERNAL_REQUEST_ORIGIN = 'auto.http.hono.internal_request'; |
There was a problem hiding this comment.
This is also set in packages/server-utils/src/integrations/hono/patchAppRequest.ts on line 15. Could it be exported from there, so it's only defined in one place?
JPeer264
left a comment
JPeer264
left a comment
There was a problem hiding this comment.
Looks great overall - Isaac had some nice points. Once they're addressed I'll approve. Mine are mostly small nits - non blocking
| variants.forEach(variant => { | ||
| // Allow skipping an individual variant (e.g. one blocked by an upstream bug) while keeping the | ||
| // others. `sentryTest.skip` above skips the whole app; this is the per-variant equivalent. | ||
| if (variant.skip) { |
There was a problem hiding this comment.
l: That is not used (anymore), right? Not sure if this is a left over or needed for another PR.
There was a problem hiding this comment.
Yeah, had this to skip something before, imho still valuable to have this capability so I left it in?
| }); | ||
| }); | ||
|
|
||
| // TODO: this test is currently skipped because we do not yet support middleware registered on new instances (e.g. here via .basePath(..).use(...)). |
There was a problem hiding this comment.
q: This was here before already, is this something that could be easily added with the new approach? (not suggesting to add this functionality in this PR). Just hinting if the "new way" could solve this (easier) than before
There was a problem hiding this comment.
actually, this test passes now, so can simply be unskipped! 🎉
| @@ -0,0 +1,168 @@ | |||
| import { expect, test } from '@playwright/test'; | |||
| import { waitForStreamedSpan, getSpanOp } from '@sentry-internal/test-utils'; | |||
| import { APP_NAME, RUNTIME, type Runtime } from './constants'; | |||
There was a problem hiding this comment.
note for myself: since I also tried to reuse existing tests in other runtimes for more coverage: #24598 (the way to use runtimes might need some consolidation once they're in)
| 'build-command': string; | ||
| 'assert-command'?: string; | ||
| label?: string; | ||
| skip?: boolean; |
There was a problem hiding this comment.
| honoIntegration, | ||
| honoMiddleware, |
There was a problem hiding this comment.
In this scenario it should be ok. Importing should be fine - on Cloudflare we bail out before we call it - so all cool. We didn't import back then because we only had nodejs_als - that's a rough guess but I can't think any other reason.
|
👋 @nicohrubec, @s1gr1d — Please review this PR when you get a chance! |
d7880fd to
f961a74
Compare
746de6f to
1a39cad
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
There are 3 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 1a39cad. Configure here.
1a39cad to
f2e09f7
Compare
Add `honoIntegration`, the auto-instrumentation that hooks Hono through the orchestrion module transform (`node:diagnostics_channel`) so requests are route-enriched without a manual `sentry()` middleware, plus its manual counterpart `honoMiddleware`. Register it in `getTracingIntegrations()` and re-export both from the server runtimes (node, cloudflare, bun, deno, and the serverless/meta-framework packages). Adds the orchestrion transform config for `hono`, node-integration-tests for the auto-instrumentation, and a new orchestrion-based `hono-4` e2e app (the middleware-based app now lives as `hono-4-legacy`). node-mastra now asserts route-enriched Hono spans in prod, where Hono is external and instrumented. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
An internal app.request() runs in a new Hono context but the same isolation scope, so the request-handling dedup short-circuited the inner Sentry middleware before it could capture context.error. When an outer handler swallowed a failed internal response (degrading to a 200), the inner route's error was never reported. The deduplicated middleware now still captures its own context's error, without re-naming the transaction or overwriting request data. Also streamline the orchestrion hono config comments, add a named-function middleware span test, and add node-integration coverage for the inner-error case. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The auto-instrumentation injection loop used a raw arity check to decide which matched handlers to wrap as middleware spans. Use the shared isMiddleware helper instead, which unwraps onError-composed sub-app handlers before checking arity — a case the inline check missed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Arity alone cannot tell a middleware from a route handler declared with an unused `next` param. The matched entries carry their registration routeMeta, so apply the same positional heuristic as wrapSubAppMiddleware: within a method+path group the last handler is the route handler and earlier ones are middleware; `.use()` (method 'ALL') falls back to arity. Adds node-integration coverage for the arity-2 route handler case. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Restores coverage lost when the shared Hono unit tests were removed during the move to @sentry/server-utils. The observable route-name and middleware span-status behaviors move into the node-integration `hono` suite; the route-hook's non-invasive prototype patching (invisible to spans/events) is restored as a focused unit test. - resolveRouteName: overlapping catch-all handler and middleware-only path fallback - wrapMiddlewareWithSpan: 3xx/4xx middleware errors do not set an error span status, 5xx does - installRouteHookOnPrototype: preserves name/length/symbols/prototype of the original route method Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The dedup branch of createHonoRequestMiddleware calls captureContextError, but the mock only stubbed requestHandler/responseHandler, so the deduplication tests threw "No captureContextError export is defined on the mock". Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Trim the comments in honoIntegration to the non-obvious reasoning (Hono's internal matchResult shape, the cached-array injection guard, the synchronous Context-constructor timing, the compose fast-path) and drop the narration of what the code already shows. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-authored-by: isaacs <i@izs.me> Co-authored-by: Jan Peer Stöcklmair <jan.oster94@gmail.com>
…cted snippet
The build-time snippet injected into each instrumented module passed the
integration factory by reference (`orchestrionModuleInjected('@flue/runtime',
flueIntegration)`), reading the binding the instant that module evaluated. For a
provided-module integration like Flue — which `@sentry/*/vite` imports back into
its own instrumented package — this closes an import cycle and crashes the worker
at startup with "Cannot access 'flueIntegration' before initialization". Wrapping
it in an arrow (`() => flueIntegration()`) defers the read until the stored thunk
runs (at `init()`), which is what breaks the cycle; the consumer still gets an
Integration from `factory()`.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
f2e09f7 to
c78651e
Compare
The new hono node-integration suite relies on orchestrion auto-instrumentation, which `bun run` cannot inject. Also bump the @sentry/node size limit, which develop pushed over 144 KB. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The dedup markers live on the isolation scope. When that scope outlives the request, e.g. forked once around the whole server on a runtime without per-request isolation, every later request was treated as a duplicate and lost its route name and request data, and could pick up a stale shouldHandleError. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
JPeer264
left a comment
JPeer264
left a comment
There was a problem hiding this comment.
Amazing!! That is an insane win!
…#24496) First of two stacked PRs splitting the Hono instrumentation rework (originally #24371). Relocates the runtime-agnostic Hono instrumentation out of `@sentry/hono` into `@sentry/server-utils` (`src/integrations/hono/`), with `@sentry/hono` re-exporting it. Since `@sentry/server-utils` must not depend on `hono`, the relocated code no longer imports it: the `Hono` class is passed in by the SDK and a vendored `honoTypes` module provides the shapes. The `sentry()` middleware now builds its request handler via the shared `createHonoRequestMiddleware`. Pure relocation — no behavior change. Also renames the `hono-4` e2e test app to `hono-4-legacy` and moves the corresponding unit tests alongside the relocated code. The orchestrion-based auto-instrumentation that builds on this lives in the stacked PR #24497. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
5 participants
Second of two stacked PRs splitting the Hono instrumentation rework (originally #24371). Stacked on #24496 — review/merge that first; the diff here is against the base PR's branch.
Adds
honoIntegration, the auto-instrumentation that hooks Hono through the orchestrion module transform (node:diagnostics_channel) so requests are route-enriched without a manualsentry()middleware, plus its manual counterparthonoMiddleware. Registers it ingetErrorIntegrations()and re-exports both from the server runtimes (node, cloudflare, bun, deno, and the serverless / meta-framework packages).Also adds the orchestrion transform config for
hono, node-integration-tests for the auto-instrumentation, and a new orchestrion-basedhono-4e2e app (the middleware-based app now lives ashono-4-legacy, added in the base PR).node-mastranow asserts route-enriched Hono spans in prod, where Hono is external and orchestrion-instrumented.Root cause / notable fix: pulling the Hono module into the Cloudflare barrel reshuffled the worker bundle's module-init order and surfaced a latent TDZ crash for provided-module integrations (Flue):
Cannot access 'flueIntegration' before initializationat worker startup. The orchestrion snippet injected into each instrumented module passed the integration factory by reference, reading the binding the instant that module evaluated — and since@sentry/*/viteimports a provided-module integration back into its own instrumented package, that closes an import cycle. The snippet now wraps the factory in an arrow (() => flueIntegration()) so the binding is only read when the stored thunk runs atinit(), breaking the cycle.Also folds in a few follow-ups:
honoMiddlewareis now exported from the remaining runtimes that only hadhonoIntegration(elysia, remix, solidstart, sveltekit), the sharedINTERNAL_REQUEST_ORIGINconstant moved intohono/constants.ts, and the previously-skipped.basePath()middleware e2e test is unskipped — the per-request orchestrion hook discovers middleware from the matched-handler list at dispatch, so it now works on the clone.basePath()returns.🤖 Generated with Claude Code