Conversation
size-limit report 📦
|
1e729ec to
74f5d86
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| if (!IS_DEV) { | ||
| expect(attrValue(serverSpan, 'sentry.segment.name.source')).toBe('route'); | ||
| expect(attrValue(serverSpan, 'http.route')).toBe('/manual-route'); | ||
| } |
There was a problem hiding this comment.
Conditional assertions in one test
Low Severity
This was flagged because the testing conventions ask to split conditionals in a single test into separate tests per path. Both Mastra e2e tests branch on IS_DEV inside one case, so the route-enriched prod assertions and the un-enriched dev assertions never run as independent tests.
Additional Locations (1)
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit 74f5d86. Configure here.
d3071cb to
bef613e
Compare
| // A Hono `matchResult[0]` entry: `[[handler, routeMeta], paramIndexMap]`. `compose` reads the handler | ||
| // at `entry[0][0]`; the `matchedRoutes` getter reads `routeMeta` at `entry[0][1]`. | ||
| // oxlint-disable-next-line typescript/no-explicit-any | ||
| type MatchedHandlerEntry = [[any, any], any]; |
There was a problem hiding this comment.
Unguarded any in Hono integration
Low Severity
New SDK source uses any (and oxlint suppressions) on MatchedHandlerEntry, channel arguments, and tracingChannel payloads without a comment explaining why a safer type is not possible. This was flagged because the review rules require that explanation on each new any in production code.
Additional Locations (2)
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit bef613e. Configure here.
There was a problem hiding this comment.
Valid, but low severity imo.
A shared type ChannelArgs = { arguments: unknown[] } would remove two of these, since instrumentInternalRequests already narrows data.arguments at lines 178-181.
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>
bef613e to
8941894
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 3 total unresolved issues (including 2 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 8941894. Configure here.
| expect(attrValue(serverSpan!, 'sentry.segment.name.source')).toBe('route'); | ||
| expect(attrValue(serverSpan!, 'http.route')).toMatch(/^\/api\/agents\/:[^/]+\/generate$/); | ||
| expect(serverSpan!.name).toMatch(/^POST \/api\/agents\/:[^/]+\/generate$/); | ||
| } |
There was a problem hiding this comment.
Conditional assertions in Mastra tests
Low Severity
These tests branch on IS_DEV inside a single case instead of splitting prod and dev. The dev path also never asserts that http.route and sentry.segment.name.source stay absent, so the un-instrumented Hono behavior is not actually locked in. This is flagged because the review rules require splitting conditional tests and asserting omitted payload fields.
Additional Locations (1)
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit 8941894. Configure here.
| /** | ||
| * 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!
|
|
||
| const effectiveShouldHandleError = | ||
| (scope[HONO_SHOULD_HANDLE_ERROR] as SentryHonoMiddlewareOptions['shouldHandleError']) ?? shouldHandleError; | ||
| responseHandler(context, effectiveShouldHandleError); |
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.
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>
The Hono auto-instrumentation added to the Node SDK defaults pushes the ESM bundle to 135.3 KB, just over the 135 KB limit. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| // outer context never sees the error. Route naming and request data stay owned by the request | ||
| // that ran first, so only the error is captured here. | ||
| const dedupShouldHandleError = | ||
| (scope[HONO_SHOULD_HANDLE_ERROR] as SentryHonoMiddlewareOptions['shouldHandleError']) ?? shouldHandleError; |
There was a problem hiding this comment.
Bug: When both automatic honoIntegration and manual honoMiddleware are used, an error can be captured twice, leading to duplicate reports if dedupeIntegration is disabled.
Severity: LOW
Suggested Fix
To prevent duplicate error capturing, the logic should be updated. One approach is to check if the error has already been captured before attempting to capture it again. For example, the deduplicated middleware branch could skip calling captureContextError, or the responseHandler in the auto-middleware could add a check to see if the error was already handled by a downstream manual middleware.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: packages/server-utils/src/integrations/hono/createHonoMiddleware.ts#L85-L88
Potential issue: When a user enables both the automatic `honoIntegration` and manually
adds the `honoMiddleware`, an error in a route handler can be captured twice. The
auto-instrumented middleware runs first, sets a `HONO_REQUEST_HANDLED` flag, and
proceeds. The manual middleware then executes, sees the flag, and captures the error in
its deduplication branch. After the handler chain completes, the original
auto-instrumented middleware's `responseHandler` also runs and captures the same error
again. This results in duplicate error reports for users who have disabled the default
`dedupeIntegration`.
Did we get this right? 👍 / 👎 to inform future reviews.
There was a problem hiding this comment.
This is valid, reproduced. (And a subtle catch, good bot!)
compose catches a thrown handler error at the throwing handler's dispatch(i) frame (node_modules/hono/dist/compose.js lines 20-31), sets context.error, and returns normally. Every middleware below that frame then sees its await next() resolve, with context.error set. captureContextError has no per-error guard (see: packages/server-utils/src/integrations/hono/middlewareHandlers.ts lines 104-115), so both Sentry middlewares report the same Error instance.
The path is the one the code documents as supported: auto honoIntegration plus a manual app.use(honoMiddleware(app)). The docs at packages/server-utils/src/integrations/hono/honoIntegration.ts lines 73-75 and lines 225-226 both say this combination is safe. The default dedupeIntegration hides it, so the visible failure is limited to users who set enableDedupe: false or drop the integration, but the promise in the doc comment is no longer true.
Note the @sentry/hono SDK is not affected: buildFilteredIntegrations strips the Hono integration from the defaults (packages/hono/src/node/sdk.ts line 24). Only the new honoMiddleware + auto combination is exposed.
Suggestion: record the context that ran requestHandler on the request scope next to HONO_REQUEST_HANDLED (line 92), then in the dedup branch capture only when scope[HONO_OWNER_CONTEXT] !== context. That keeps the case the branch exists for (an internal .request() runs in a new Hono Context but the same isolation scope) and removes
the same-context duplicate.
A unit test in packages/server-utils/test/integrations/hono/createHonoMiddleware.test.ts that asserts captureContextError runs once for two middlewares on one context, and once more for a second context, would catch this issue: https://gist.github.com/isaacs/e07dea6f58fea80626378430d6e36a7f
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.)
| /** These are integrations that cover error capture, in addition to tracing. */ | ||
| export function getErrorIntegrations(): Integration[] { | ||
| return [expressIntegration(), fastifyIntegration(), hapiIntegration(), koaIntegration()]; | ||
| return [expressIntegration(), fastifyIntegration(), hapiIntegration(), honoIntegration(), koaIntegration()]; |
There was a problem hiding this comment.
Minor thing, but the PR description says this gets registered in the getTracingIntegrations, but it's actually registered in getErrorIntegrations. I'm assuming that the code is correct, and the PR description is out of date?
| // 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.
| // outer context never sees the error. Route naming and request data stay owned by the request | ||
| // that ran first, so only the error is captured here. | ||
| const dedupShouldHandleError = | ||
| (scope[HONO_SHOULD_HANDLE_ERROR] as SentryHonoMiddlewareOptions['shouldHandleError']) ?? shouldHandleError; |
There was a problem hiding this comment.
This is valid, reproduced. (And a subtle catch, good bot!)
compose catches a thrown handler error at the throwing handler's dispatch(i) frame (node_modules/hono/dist/compose.js lines 20-31), sets context.error, and returns normally. Every middleware below that frame then sees its await next() resolve, with context.error set. captureContextError has no per-error guard (see: packages/server-utils/src/integrations/hono/middlewareHandlers.ts lines 104-115), so both Sentry middlewares report the same Error instance.
The path is the one the code documents as supported: auto honoIntegration plus a manual app.use(honoMiddleware(app)). The docs at packages/server-utils/src/integrations/hono/honoIntegration.ts lines 73-75 and lines 225-226 both say this combination is safe. The default dedupeIntegration hides it, so the visible failure is limited to users who set enableDedupe: false or drop the integration, but the promise in the doc comment is no longer true.
Note the @sentry/hono SDK is not affected: buildFilteredIntegrations strips the Hono integration from the defaults (packages/hono/src/node/sdk.ts line 24). Only the new honoMiddleware + auto combination is exposed.
Suggestion: record the context that ran requestHandler on the request scope next to HONO_REQUEST_HANDLED (line 92), then in the dedup branch capture only when scope[HONO_OWNER_CONTEXT] !== context. That keeps the case the branch exists for (an internal .request() runs in a new Hono Context but the same isolation scope) and removes
the same-context duplicate.
A unit test in packages/server-utils/test/integrations/hono/createHonoMiddleware.test.ts that asserts captureContextError runs once for two middlewares on one context, and once more for a second context, would catch this issue: https://gist.github.com/isaacs/e07dea6f58fea80626378430d6e36a7f
| * 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.
| 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.
| import { Hono } from 'hono'; | ||
|
|
||
| // No `@sentry/hono` and no `sentry()` middleware: the app is instrumented automatically by the | ||
| // `honoIntegration` default in `@sentry/node` (via orchestrion hooking the `Hono` constructor). |
There was a problem hiding this comment.
| // `honoIntegration` default in `@sentry/node` (via orchestrion hooking the `Hono` constructor). | |
| // `honoIntegration` default in `@sentry/node` (via orchestrion hooking the `Context` constructor). |
| @@ -0,0 +1,28 @@ | |||
| // Builds `src/entry.bun.ts` with the orchestrion `bun build` plugin, emitting `dist/entry.bun.js` | |||
| // for the server to run. The plugin injects the `orchestrion:hono:honoConstructor` diagnostics | |||
There was a problem hiding this comment.
| // for the server to run. The plugin injects the `orchestrion:hono:honoConstructor` diagnostics | |
| // for the server to run. The plugin injects the `orchestrion:hono:context` diagnostics |
| // 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?
| if (!IS_DEV) { | ||
| expect(attrValue(serverSpan, 'sentry.segment.name.source')).toBe('route'); | ||
| expect(attrValue(serverSpan, 'http.route')).toBe('/manual-route'); | ||
| } |
| // A Hono `matchResult[0]` entry: `[[handler, routeMeta], paramIndexMap]`. `compose` reads the handler | ||
| // at `entry[0][0]`; the `matchedRoutes` getter reads `routeMeta` at `entry[0][1]`. | ||
| // oxlint-disable-next-line typescript/no-explicit-any | ||
| type MatchedHandlerEntry = [[any, any], any]; |
There was a problem hiding this comment.
Valid, but low severity imo.
A shared type ChannelArgs = { arguments: unknown[] } would remove two of these, since instrumentInternalRequests already narrows data.arguments at lines 178-181.
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.
| // Cloudflare-SDK init | ||
| export default (env: { E2E_TEST_DSN: string }) => ({ | ||
| dsn: env.E2E_TEST_DSN, | ||
| environment: 'qa', | ||
| tracesSampleRate: 1.0, | ||
| tunnel: 'http://localhost:3031/', | ||
| }); |
There was a problem hiding this comment.
I have to investigate why this is not suggested by Clankers (assuming this was written by them)
| // Cloudflare-SDK init | |
| export default (env: { E2E_TEST_DSN: string }) => ({ | |
| dsn: env.E2E_TEST_DSN, | |
| environment: 'qa', | |
| tracesSampleRate: 1.0, | |
| tunnel: 'http://localhost:3031/', | |
| }); | |
| import { defineCloudflareOptions } from '@sentry/cloudflare'; | |
| export default defineCloudflareOptions<{ E2E_TEST_DSN: string }>((env) => ({ | |
| dsn: env.E2E_TEST_DSN, | |
| environment: 'qa', | |
| tracesSampleRate: 1.0, | |
| tunnel: 'http://localhost:3031/', | |
| })); |
| }); | ||
| }); | ||
|
|
||
| // 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
| @@ -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)
| // `vite build` runs the Sentry auto-instrument transform over the worker entry: it wraps the default | ||
| // export (the Hono app) with `withSentry` and injects the orchestrion channels that the | ||
| // `honoIntegration` default subscribes to. `wrangler dev` (via the plugin's `.wrangler/deploy` | ||
| // redirect) then serves the built output. |
There was a problem hiding this comment.
l: I guess we can just remove it
| // `vite build` runs the Sentry auto-instrument transform over the worker entry: it wraps the default | |
| // export (the Hono app) with `withSentry` and injects the orchestrion channels that the | |
| // `honoIntegration` default subscribes to. `wrangler dev` (via the plugin's `.wrangler/deploy` | |
| // redirect) then serves the built output. |
| '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! |
This branch has not been deployed
4 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 ingetTracingIntegrations()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.🤖 Generated with Claude Code