diff --git a/dev-packages/e2e-tests/test-applications/hono-4-legacy/package.json b/dev-packages/e2e-tests/test-applications/hono-4-legacy/package.json index 99f96170110f..465d1fbb2654 100644 --- a/dev-packages/e2e-tests/test-applications/hono-4-legacy/package.json +++ b/dev-packages/e2e-tests/test-applications/hono-4-legacy/package.json @@ -16,6 +16,7 @@ "dependencies": { "@sentry/bun": "latest || *", "@sentry/cloudflare": "latest || *", + "@sentry/core": "latest || *", "@sentry/deno": "latest || *", "@sentry/hono": "latest || *", "@sentry/node": "latest || *", diff --git a/dev-packages/e2e-tests/test-applications/hono-4-legacy/src/route-groups/test-route-patterns.ts b/dev-packages/e2e-tests/test-applications/hono-4-legacy/src/route-groups/test-route-patterns.ts index ac6fea1320cf..473df4b9e9ad 100644 --- a/dev-packages/e2e-tests/test-applications/hono-4-legacy/src/route-groups/test-route-patterns.ts +++ b/dev-packages/e2e-tests/test-applications/hono-4-legacy/src/route-groups/test-route-patterns.ts @@ -1,3 +1,4 @@ +import { captureMessage } from '@sentry/core'; import { Hono } from 'hono'; const routePatterns = new Hono(); @@ -30,4 +31,24 @@ METHODS.forEach(method => { routePatterns.on(method, '/on', c => c.text(`${method} on response`)); }); +// The catch-all below also matches. An event from inside this handler must still get this route. +routePatterns.get('/capture/:id', c => { + captureMessage('Message from a route handler'); + return c.text('captured'); +}); +routePatterns.get('/capture/*', c => c.text('capture catch-all')); + +// Passes `draft` on to the catch-all below, which then names the request. +routePatterns.get('/posts/:id', async (c, next) => { + if (c.req.param('id') === 'draft') { + await next(); + return; + } + return c.text('post'); +}); +routePatterns.get('/posts/*', c => { + captureMessage('Message from a route handler after next()'); + return c.text('fallback'); +}); + export { routePatterns }; diff --git a/dev-packages/e2e-tests/test-applications/hono-4-legacy/tests/route-patterns.test.ts b/dev-packages/e2e-tests/test-applications/hono-4-legacy/tests/route-patterns.test.ts index bfebb9f5b6ab..d1251fad340e 100644 --- a/dev-packages/e2e-tests/test-applications/hono-4-legacy/tests/route-patterns.test.ts +++ b/dev-packages/e2e-tests/test-applications/hono-4-legacy/tests/route-patterns.test.ts @@ -1,5 +1,10 @@ import { expect, test } from '@playwright/test'; -import { waitForStreamedSpan, getSpanOp, collectStreamedSpansUntilSegment } from '@sentry-internal/test-utils'; +import { + waitForError, + waitForStreamedSpan, + getSpanOp, + collectStreamedSpansUntilSegment, +} from '@sentry-internal/test-utils'; import { APP_NAME } from './constants'; const PREFIX = '/test-routes'; @@ -176,3 +181,23 @@ test('async handler sends span', async ({ baseURL }) => { const middlewareSpans = spans.filter(s => getSpanOp(s) === 'middleware'); expect(middlewareSpans).toEqual([]); }); + +test('names an event captured in a route handler after that route', async ({ baseURL }) => { + const eventPromise = waitForError(APP_NAME, event => event.message === 'Message from a route handler'); + + const response = await fetch(`${baseURL}${PREFIX}/capture/42`); + expect(response.status).toBe(200); + + const event = await eventPromise; + expect(event.transaction).toBe(`GET ${PREFIX}/capture/:id`); +}); + +test('names an event from the route handler that runs after next() after that route', async ({ baseURL }) => { + const eventPromise = waitForError(APP_NAME, event => event.message === 'Message from a route handler after next()'); + + const response = await fetch(`${baseURL}${PREFIX}/posts/draft`); + expect(response.status).toBe(200); + + const event = await eventPromise; + expect(event.transaction).toBe(`GET ${PREFIX}/posts/*`); +}); diff --git a/dev-packages/e2e-tests/test-applications/hono-4/package.json b/dev-packages/e2e-tests/test-applications/hono-4/package.json index 82f47a8b7368..180ed53a5188 100644 --- a/dev-packages/e2e-tests/test-applications/hono-4/package.json +++ b/dev-packages/e2e-tests/test-applications/hono-4/package.json @@ -20,6 +20,7 @@ "dependencies": { "@sentry/bun": "latest || *", "@sentry/cloudflare": "latest || *", + "@sentry/core": "latest || *", "@sentry/deno": "latest || *", "@sentry/node": "latest || *", "@hono/node-server": "^2.0.5", diff --git a/dev-packages/e2e-tests/test-applications/hono-4/src/route-groups/test-route-patterns.ts b/dev-packages/e2e-tests/test-applications/hono-4/src/route-groups/test-route-patterns.ts index ac6fea1320cf..473df4b9e9ad 100644 --- a/dev-packages/e2e-tests/test-applications/hono-4/src/route-groups/test-route-patterns.ts +++ b/dev-packages/e2e-tests/test-applications/hono-4/src/route-groups/test-route-patterns.ts @@ -1,3 +1,4 @@ +import { captureMessage } from '@sentry/core'; import { Hono } from 'hono'; const routePatterns = new Hono(); @@ -30,4 +31,24 @@ METHODS.forEach(method => { routePatterns.on(method, '/on', c => c.text(`${method} on response`)); }); +// The catch-all below also matches. An event from inside this handler must still get this route. +routePatterns.get('/capture/:id', c => { + captureMessage('Message from a route handler'); + return c.text('captured'); +}); +routePatterns.get('/capture/*', c => c.text('capture catch-all')); + +// Passes `draft` on to the catch-all below, which then names the request. +routePatterns.get('/posts/:id', async (c, next) => { + if (c.req.param('id') === 'draft') { + await next(); + return; + } + return c.text('post'); +}); +routePatterns.get('/posts/*', c => { + captureMessage('Message from a route handler after next()'); + return c.text('fallback'); +}); + export { routePatterns }; diff --git a/dev-packages/e2e-tests/test-applications/hono-4/tests/route-patterns.test.ts b/dev-packages/e2e-tests/test-applications/hono-4/tests/route-patterns.test.ts index bfebb9f5b6ab..d1251fad340e 100644 --- a/dev-packages/e2e-tests/test-applications/hono-4/tests/route-patterns.test.ts +++ b/dev-packages/e2e-tests/test-applications/hono-4/tests/route-patterns.test.ts @@ -1,5 +1,10 @@ import { expect, test } from '@playwright/test'; -import { waitForStreamedSpan, getSpanOp, collectStreamedSpansUntilSegment } from '@sentry-internal/test-utils'; +import { + waitForError, + waitForStreamedSpan, + getSpanOp, + collectStreamedSpansUntilSegment, +} from '@sentry-internal/test-utils'; import { APP_NAME } from './constants'; const PREFIX = '/test-routes'; @@ -176,3 +181,23 @@ test('async handler sends span', async ({ baseURL }) => { const middlewareSpans = spans.filter(s => getSpanOp(s) === 'middleware'); expect(middlewareSpans).toEqual([]); }); + +test('names an event captured in a route handler after that route', async ({ baseURL }) => { + const eventPromise = waitForError(APP_NAME, event => event.message === 'Message from a route handler'); + + const response = await fetch(`${baseURL}${PREFIX}/capture/42`); + expect(response.status).toBe(200); + + const event = await eventPromise; + expect(event.transaction).toBe(`GET ${PREFIX}/capture/:id`); +}); + +test('names an event from the route handler that runs after next() after that route', async ({ baseURL }) => { + const eventPromise = waitForError(APP_NAME, event => event.message === 'Message from a route handler after next()'); + + const response = await fetch(`${baseURL}${PREFIX}/posts/draft`); + expect(response.status).toBe(200); + + const event = await eventPromise; + expect(event.transaction).toBe(`GET ${PREFIX}/posts/*`); +}); diff --git a/dev-packages/node-integration-tests/suites/hono-sdk/scenario.mjs b/dev-packages/node-integration-tests/suites/hono-sdk/scenario.mjs index d27dc20bbc30..c8ec88a9975b 100644 --- a/dev-packages/node-integration-tests/suites/hono-sdk/scenario.mjs +++ b/dev-packages/node-integration-tests/suites/hono-sdk/scenario.mjs @@ -1,5 +1,5 @@ import { serve } from '@hono/node-server'; -import { sentry } from '@sentry/hono/node'; +import { captureMessage, sentry } from '@sentry/hono/node'; import { sendPortToRunner } from '@sentry-internal/node-integration-tests'; import { Hono } from 'hono'; @@ -20,6 +20,61 @@ app.get('/error/:param', () => { throw new Error('Test error from Hono app'); }); +// Both routes match `/users/me`. The name must come from the handler that responded. +app.get('/users/me', c => c.text('me')); +app.get('/users/:id', c => c.text(`user ${c.req.param('id')}`)); + +// The catch-all below also matches. An event from inside this handler must still get this route. +app.get('/users/:id/capture', c => { + captureMessage('Message from a route handler'); + return c.text('captured'); +}); + +// A middleware that returns early. The request gets the route that would have run. +app.use('/early/*', async (c, _next) => c.text('blocked by middleware')); +app.get('/early/:id', c => c.text('never reached')); + +// Only a middleware matches this path. +app.use('/mw-only/*', async (_c, _next) => new Response('handled by middleware')); + +const subApp = new Hono(); +subApp.get('/hello', c => c.text('sub hello')); +app.route('/sub', subApp); + +app.basePath('/base').get('/hello', c => c.text('base hello')); + +app.all('/any-method', c => c.text(`any method: ${c.req.method}`)); + +// Hono answers `HEAD` with the `GET` route. The SDK records no server span for `HEAD`, so the test +// checks the name on the error event. +app.get('/head-error/:id', () => { + throw new Error('Test error from a HEAD request'); +}); + +const innerApp = new Hono(); +innerApp.get('/item/:itemId', c => c.json({ itemId: c.req.param('itemId') })); +app.get('/outer/:itemId', async c => { + const res = await innerApp.request(`/item/${c.req.param('itemId')}`); + return c.json({ outer: c.req.param('itemId'), inner: await res.json() }); +}); + +// Passes `draft` on to the catch-all below, which then names the request. +app.get('/posts/:id', async (c, next) => { + if (c.req.param('id') === 'draft') { + await next(); + return; + } + return c.text('post'); +}); +app.get('/posts/*', c => { + captureMessage('Message from a route handler after next()'); + return c.text('fallback'); +}); + +// A catch-all registered last. It also matches `/users/me` and `/users/42`, but must not name them. +app.get('/users/*', c => c.text('users catch-all')); +app.get('/early/*', c => c.text('never reached')); + serve({ fetch: app.fetch, port: 0 }, info => { sendPortToRunner(info.port); }); diff --git a/dev-packages/node-integration-tests/suites/hono-sdk/test.ts b/dev-packages/node-integration-tests/suites/hono-sdk/test.ts index 1366be964a95..ca34ff88baf8 100644 --- a/dev-packages/node-integration-tests/suites/hono-sdk/test.ts +++ b/dev-packages/node-integration-tests/suites/hono-sdk/test.ts @@ -1,12 +1,97 @@ import { afterAll, describe, expect } from 'vitest'; import { cleanupChildProcesses, createEsmAndCjsTests } from '../../utils/runner'; +const ROUTE_NAME_CASES: Array<{ method: 'get' | 'post'; path: string; name: string; route: string }> = [ + { method: 'get', path: '/users/me', name: 'GET /users/me', route: '/users/me' }, + { method: 'get', path: '/users/42', name: 'GET /users/:id', route: '/users/:id' }, + { method: 'get', path: '/users/42/posts', name: 'GET /users/*', route: '/users/*' }, + { method: 'get', path: '/early/1', name: 'GET /early/:id', route: '/early/:id' }, + { method: 'get', path: '/posts/1', name: 'GET /posts/:id', route: '/posts/:id' }, + { method: 'get', path: '/posts/draft', name: 'GET /posts/*', route: '/posts/*' }, + { method: 'get', path: '/mw-only/anything', name: 'GET /mw-only/*', route: '/mw-only/*' }, + { method: 'get', path: '/sub/hello', name: 'GET /sub/hello', route: '/sub/hello' }, + { method: 'get', path: '/base/hello', name: 'GET /base/hello', route: '/base/hello' }, + { method: 'post', path: '/any-method', name: 'POST /any-method', route: '/any-method' }, + { method: 'get', path: '/outer/self-watering-plant', name: 'GET /outer/:itemId', route: '/outer/:itemId' }, +]; + describe('hono-sdk (Node)', () => { afterAll(() => { cleanupChildProcesses(); }); createEsmAndCjsTests(__dirname, 'scenario.mjs', 'instrument.mjs', (createRunner, test) => { + // All cases in one test, because each `test()` starts its own Node process. + test('names each request after the route of the handler that responded', async () => { + const runner = createRunner().unordered(); + for (const { name, route } of ROUTE_NAME_CASES) { + runner.expect({ + span: container => { + const segment = container.items.find(item => item.is_segment && item.name === name); + if (!segment) { + throw new Error(`segment for \`${name}\` not in this container`); + } + expect(segment.attributes?.['http.route']).toEqual({ type: 'string', value: route }); + expect(segment.attributes?.['sentry.segment.name.source']).toEqual({ type: 'string', value: 'route' }); + }, + }); + } + + const started = runner.start(); + for (const { method, path } of ROUTE_NAME_CASES) { + await started.makeRequest(method, path); + } + await started.completed(); + }); + + test('names an event captured in a route handler after that route', async () => { + const runner = createRunner() + .ignore('span') + .expect({ event: { message: 'Message from a route handler', transaction: 'GET /users/:id/capture' } }) + .start(); + runner.makeRequest('get', '/users/42/capture'); + await runner.completed(); + }); + + test('keeps the runtime name for a request without a route', async () => { + const runner = createRunner() + .expect({ + span: container => { + const segment = container.items.find(item => item.is_segment); + expect(segment?.name).toBe('GET'); + expect(segment?.attributes?.['sentry.segment.name.source']).toEqual({ type: 'string', value: 'url' }); + expect(segment?.attributes?.['url.path']).toEqual({ type: 'string', value: '/does-not-exist' }); + expect(segment?.attributes?.['http.route']).toBeUndefined(); + }, + }) + .start(); + runner.makeRequest('get', '/does-not-exist', { expectError: true }); + await runner.completed(); + }); + + test('names an event from the route handler that runs after next() after that route', async () => { + const runner = createRunner() + .ignore('span') + .expect({ event: { message: 'Message from a route handler after next()', transaction: 'GET /posts/*' } }) + .start(); + runner.makeRequest('get', '/posts/draft'); + await runner.completed(); + }); + + test('names a HEAD request after the GET route', async () => { + const runner = createRunner() + .ignore('span') + .expect({ + event: { + exception: { values: [{ value: 'Test error from a HEAD request' }] }, + transaction: 'HEAD /head-error/:id', + }, + }) + .start(); + runner.makeRequest('head', '/head-error/1', { expectError: true }); + await runner.completed(); + }); + test('creates a segment span for a basic GET request', async () => { const runner = createRunner() .expect({ diff --git a/dev-packages/node-integration-tests/suites/hono/scenario-honoMiddleware.mjs b/dev-packages/node-integration-tests/suites/hono/scenario-honoMiddleware.mjs new file mode 100644 index 000000000000..3b2e380f4b73 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/hono/scenario-honoMiddleware.mjs @@ -0,0 +1,38 @@ +import { serve } from '@hono/node-server'; +import * as Sentry from '@sentry/node'; +import { sendPortToRunner } from '@sentry-internal/node-integration-tests'; +import { Hono } from 'hono'; + +// The auto-instrumentation and the manual `honoMiddleware(app)` are both active. +const app = new Hono(); +app.use(Sentry.honoMiddleware(app)); + +// Counts the Sentry wrappers around the handler that runs. Both setups wrap, and the router reuses the +// handler list for each request. +function wrapperCount(c) { + const [matchResultKey] = Object.getOwnPropertySymbols(Object.getPrototypeOf(c.req)); + let handler = c.req[matchResultKey][0][c.req.routeIndex][0][0]; + let count = 0; + while (handler.__sentry_original__) { + handler = handler.__sentry_original__; + count++; + } + return count; +} + +// No route handler declares `next`, so the SDK does not wrap. +app.get('/users/me', c => c.json({ wrappers: wrapperCount(c) })); + +// Declares `next`, so the SDK wraps the route handlers. +app.get('/posts/:id', async (c, next) => { + if (c.req.param('id') === 'draft') { + await next(); + return; + } + return c.json({ wrappers: wrapperCount(c) }); +}); +app.get('/posts/*', c => c.json({ wrappers: wrapperCount(c) })); + +serve({ fetch: app.fetch, port: 0 }, info => { + sendPortToRunner(info.port); +}); diff --git a/dev-packages/node-integration-tests/suites/hono/scenario.mjs b/dev-packages/node-integration-tests/suites/hono/scenario.mjs index ed0f816c03cf..85c9cd8709fb 100644 --- a/dev-packages/node-integration-tests/suites/hono/scenario.mjs +++ b/dev-packages/node-integration-tests/suites/hono/scenario.mjs @@ -1,5 +1,6 @@ import { serve } from '@hono/node-server'; import { sendPortToRunner } from '@sentry-internal/node-integration-tests'; +import { captureMessage, startSpan } from '@sentry/node'; import { Hono } from 'hono'; // No `@sentry/hono` and no `sentry()` middleware: the app is instrumented automatically by the @@ -97,6 +98,50 @@ app }) .get('/hello', c => c.json({ greeting: 'world' })); +// Both routes match `/users/me`. The name must come from the handler that responded. +app.get('/users/me', c => c.text('me')); +app.get('/users/:id', c => c.text(`user ${c.req.param('id')}`)); + +// The catch-all below also matches. An event from inside this handler must still get this route. +app.get('/users/:id/capture', c => { + captureMessage('Message from a route handler'); + return c.text('captured'); +}); + +// A middleware that returns early. The request gets the route that would have run. +app.use('/early/*', async (c, _next) => c.text('blocked by middleware')); +app.get('/early/:id', c => c.text('never reached')); + +app.all('/any-method', c => c.text(`any method: ${c.req.method}`)); + +// Hono answers `HEAD` with the `GET` route. The SDK records no server span for `HEAD`, so the test +// checks the name on the error event. +app.get('/head-error/:id', () => { + throw new Error('Test error from a HEAD request'); +}); + +// A user span around the route handler. The SDK must rename the segment, not this span. The handler +// declares `next`, so the SDK renames inside this span. +app.use('/traced/*', (_c, next) => startSpan({ name: 'user span' }, () => next())); +app.get('/traced/:id', (c, _next) => c.text('traced')); + +// Passes `draft` on to the catch-all below, which then names the request. +app.get('/posts/:id', async (c, next) => { + if (c.req.param('id') === 'draft') { + await next(); + return; + } + return c.text('post'); +}); +app.get('/posts/*', c => { + captureMessage('Message from a route handler after next()'); + return c.text('fallback'); +}); + +// A catch-all registered last. It also matches `/users/me` and `/users/42`, but must not name them. +app.get('/users/*', c => c.text('users catch-all')); +app.get('/early/*', c => c.text('never reached')); + serve({ fetch: app.fetch, port: 0 }, info => { sendPortToRunner(info.port); }); diff --git a/dev-packages/node-integration-tests/suites/hono/test.ts b/dev-packages/node-integration-tests/suites/hono/test.ts index 20a16f7e1eba..511ea3a83cf7 100644 --- a/dev-packages/node-integration-tests/suites/hono/test.ts +++ b/dev-packages/node-integration-tests/suites/hono/test.ts @@ -15,12 +15,130 @@ type StreamedSpan = { name?: string; status?: string; is_segment?: boolean; attr const attr = (span: StreamedSpan, key: string): unknown => span.attributes?.[key]?.value; const op = (span: StreamedSpan): unknown => attr(span, 'sentry.op'); +const ROUTE_NAME_CASES: Array<{ method: 'get' | 'post'; path: string; name: string; route: string }> = [ + { method: 'get', path: '/users/me', name: 'GET /users/me', route: '/users/me' }, + { method: 'get', path: '/users/42', name: 'GET /users/:id', route: '/users/:id' }, + { method: 'get', path: '/users/42/posts', name: 'GET /users/*', route: '/users/*' }, + { method: 'get', path: '/overlap/123', name: 'GET /overlap/:id', route: '/overlap/:id' }, + { method: 'get', path: '/early/1', name: 'GET /early/:id', route: '/early/:id' }, + { method: 'get', path: '/posts/1', name: 'GET /posts/:id', route: '/posts/:id' }, + { method: 'get', path: '/posts/draft', name: 'GET /posts/*', route: '/posts/*' }, + { method: 'get', path: '/mw-only/anything', name: 'GET /mw-only/*', route: '/mw-only/*' }, + { method: 'get', path: '/sub/hello', name: 'GET /sub/hello', route: '/sub/hello' }, + { + method: 'get', + path: '/test-basepath-mw/hello', + name: 'GET /test-basepath-mw/hello', + route: '/test-basepath-mw/hello', + }, + { method: 'post', path: '/any-method', name: 'POST /any-method', route: '/any-method' }, + { method: 'get', path: '/outer/self-watering-plant', name: 'GET /outer/:itemId', route: '/outer/:itemId' }, +]; + +function segmentNamed(name: string, route: string): { span: (container: { items: StreamedSpan[] }) => void } { + return { + span: container => { + const segment = container.items.find(item => item.is_segment && item.name === name); + if (!segment) { + throw new Error(`segment for \`${name}\` not in this container`); + } + expect(op(segment)).toBe('http.server'); + expect(attr(segment, 'http.route')).toBe(route); + expect(attr(segment, 'sentry.segment.name.source')).toBe('route'); + }, + }; +} + describe('hono auto-instrumentation', () => { afterAll(() => { cleanupChildProcesses(); }); createEsmAndCjsTests(__dirname, 'scenario.mjs', 'instrument.mjs', (createRunner, test) => { + // All cases in one test, because each `test()` starts its own Node process. + test('names each request after the route of the handler that responded', async () => { + const runner = createRunner().unordered(); + for (const { name, route } of ROUTE_NAME_CASES) { + runner.expect(segmentNamed(name, route)); + } + + const started = runner.start(); + for (const { method, path } of ROUTE_NAME_CASES) { + await started.makeRequest(method, path); + } + await started.completed(); + }); + + test('names an event captured in a route handler after that route', async () => { + const runner = createRunner() + .ignore('span') + .expect({ event: { message: 'Message from a route handler', transaction: 'GET /users/:id/capture' } }) + .start(); + runner.makeRequest('get', '/users/42/capture'); + await runner.completed(); + }); + + test('does not rename a span that a middleware starts around the route handler', async () => { + const runner = createRunner() + .unordered() + .expect({ + span: container => { + const segment = container.items.find(item => item.is_segment && item.name === 'GET /traced/:id'); + if (!segment) { + throw new Error('segment for `GET /traced/:id` not in this container'); + } + expect(container.items.filter(item => item.name === 'user span')).toHaveLength(1); + }, + }) + .start(); + runner.makeRequest('get', '/traced/1'); + await runner.completed(); + }); + + test('keeps the runtime name for a request without a route', async () => { + const runner = createRunner() + .unordered() + .expect({ + span: container => { + const segment = container.items.find( + item => item.is_segment && attr(item, 'url.path') === '/does-not-exist', + ); + if (!segment) { + throw new Error('segment for `/does-not-exist` not in this container'); + } + expect(segment.name).toBe('GET'); + expect(attr(segment, 'sentry.segment.name.source')).toBe('url'); + expect(attr(segment, 'http.route')).toBeUndefined(); + }, + }) + .start(); + runner.makeRequest('get', '/does-not-exist', { expectError: true }); + await runner.completed(); + }); + + test('names an event from the route handler that runs after next() after that route', async () => { + const runner = createRunner() + .ignore('span') + .expect({ event: { message: 'Message from a route handler after next()', transaction: 'GET /posts/*' } }) + .start(); + runner.makeRequest('get', '/posts/draft'); + await runner.completed(); + }); + + test('names a HEAD request after the GET route', async () => { + const runner = createRunner() + .unordered() + .expect({ + event: { + exception: { values: [{ value: 'Test error from a HEAD request' }] }, + transaction: 'HEAD /head-error/:id', + }, + }) + .start(); + runner.makeRequest('head', '/head-error/1', { expectError: true }); + await runner.completed(); + }); + test('creates a transaction for a basic GET request', async () => { const runner = createRunner() .unordered() @@ -227,42 +345,6 @@ describe('hono auto-instrumentation', () => { await runner.completed(); }); - test('resolves overlapping handlers to the responding route, not the catch-all', async () => { - const runner = createRunner() - .unordered() - .expect({ - span: container => { - const segment = container.items.find(item => item.is_segment && item.name === 'GET /overlap/:id'); - if (!segment) { - throw new Error('segment for `GET /overlap/:id` not in this container'); - } - // The catch-all `GET /overlap/*` handler also matched, but the parameterized handler - // responded — the route name must not collapse to `/overlap/*`. - expect(attr(segment, 'sentry.segment.name.source')).toBe('route'); - }, - }) - .start(); - runner.makeRequest('get', '/overlap/123'); - await runner.completed(); - }); - - test('falls back to the matched middleware path when only middleware matched', async () => { - const runner = createRunner() - .unordered() - .expect({ - span: container => { - const segment = container.items.find(item => item.is_segment && item.name === 'GET /mw-only/*'); - if (!segment) { - throw new Error('segment for `GET /mw-only/*` not in this container'); - } - expect(op(segment)).toBe('http.server'); - }, - }) - .start(); - runner.makeRequest('get', '/mw-only/anything'); - await runner.completed(); - }); - test('does not set the middleware span to error for a 3xx middleware error', async () => { const runner = createRunner() .unordered() @@ -321,3 +403,28 @@ describe('hono auto-instrumentation', () => { }); }); }); + +describe('hono auto-instrumentation with honoMiddleware(app)', () => { + afterAll(() => { + cleanupChildProcesses(); + }); + + createEsmAndCjsTests(__dirname, 'scenario-honoMiddleware.mjs', 'instrument.mjs', (createRunner, test) => { + test('wraps route handlers only when one declares next, and only once', async () => { + const runner = createRunner() + .unordered() + .expect(segmentNamed('GET /users/me', '/users/me')) + .expect(segmentNamed('GET /posts/:id', '/posts/:id')) + .expect(segmentNamed('GET /posts/:id', '/posts/:id')) + .expect(segmentNamed('GET /posts/*', '/posts/*')) + .start(); + + expect(await runner.makeRequest('get', '/users/me')).toEqual({ wrappers: 0 }); + // The second request gets the same cached handler list. + expect(await runner.makeRequest('get', '/posts/1')).toEqual({ wrappers: 1 }); + expect(await runner.makeRequest('get', '/posts/1')).toEqual({ wrappers: 1 }); + expect(await runner.makeRequest('get', '/posts/draft')).toEqual({ wrappers: 1 }); + await runner.completed(); + }); + }); +}); diff --git a/dev-packages/node-integration-tests/utils/runner/createRunner.ts b/dev-packages/node-integration-tests/utils/runner/createRunner.ts index b592869e9aca..a98b6a89e0fa 100644 --- a/dev-packages/node-integration-tests/utils/runner/createRunner.ts +++ b/dev-packages/node-integration-tests/utils/runner/createRunner.ts @@ -103,7 +103,7 @@ type StartResult = { getPort(): number | undefined; sendSignal(signal: NodeJS.Signals): void; makeRequest( - method: 'get' | 'post' | 'put' | 'delete' | 'patch', + method: 'get' | 'head' | 'post' | 'put' | 'delete' | 'patch', path: string, options?: { headers?: Record; data?: BodyInit; expectError?: boolean; timeout?: number }, ): Promise; @@ -781,7 +781,7 @@ export function createRunner(...paths: string[]) { child?.kill(signal); }, makeRequest: async function ( - method: 'get' | 'post' | 'put' | 'delete' | 'patch', + method: 'get' | 'head' | 'post' | 'put' | 'delete' | 'patch', path: string, options: { headers?: Record; diff --git a/packages/hono/test/shared/middlewareHandlers.test.ts b/packages/hono/test/shared/middlewareHandlers.test.ts index fb7d16699d07..2ab3f046c594 100644 --- a/packages/hono/test/shared/middlewareHandlers.test.ts +++ b/packages/hono/test/shared/middlewareHandlers.test.ts @@ -50,13 +50,17 @@ const getRootSpanMock = SentryCore.getRootSpan as ReturnType; // `event` throws on access so `hasFetchEvent` reports `false` (the Node/Bun/Deno path), which makes // the request handler read request data from `req.raw` rather than a Cloudflare fetch event. -function createMockContext(status: number, error?: Error): object { +function createMockContext( + status: number, + error?: Error, + matchedRoutes: unknown[] = [{ basePath: '/', path: '/test', method: 'GET', handler: (_c: unknown) => undefined }], +): object { return { req: { method: 'GET', routeIndex: 0, // `resolveRouteName` reads the matched routes straight off the request. - matchedRoutes: [{ basePath: '/', path: '/test', method: 'GET', handler: (_c: unknown) => undefined }], + matchedRoutes, raw: new Request('http://localhost/test'), }, get event(): never { @@ -204,6 +208,16 @@ describe('responseHandler (via createHonoRequestMiddleware)', () => { expect(mockRootSpan.setAttribute).toHaveBeenCalledWith(HTTP_ROUTE, '/test'); expect(mockRootSpan.setAttribute).toHaveBeenCalledWith(SENTRY_SEGMENT_NAME_SOURCE, 'route'); }); + + it('keeps the runtime name when no route matched', async () => { + getActiveSpanMock.mockReturnValue(mockRootSpan); + + await runMiddleware(createMockContext(404, undefined, [])); + + expect(mockRootSpan.updateName).not.toHaveBeenCalled(); + expect(mockRootSpan.setAttribute).not.toHaveBeenCalledWith(HTTP_ROUTE, expect.anything()); + expect(mockSetTransactionName).not.toHaveBeenCalled(); + }); }); }); diff --git a/packages/hono/test/shared/resolveRouteName.test.ts b/packages/hono/test/shared/resolveRouteName.test.ts new file mode 100644 index 000000000000..48881b277010 --- /dev/null +++ b/packages/hono/test/shared/resolveRouteName.test.ts @@ -0,0 +1,197 @@ +import * as SentryCore from '@sentry/core'; +import { applyHonoPatches, createHonoRequestMiddleware } from '@sentry/server-utils'; +import { Hono } from 'hono'; +import { HonoRequest } from 'hono/request'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +const mockSetTransactionName = vi.fn(); + +vi.mock('@sentry/core', async () => { + const actual = await vi.importActual('@sentry/core'); + return { + ...actual, + getActiveSpan: vi.fn(() => undefined), + getIsolationScope: vi.fn(() => ({ + setTransactionName: mockSetTransactionName, + setSDKProcessingMetadata: vi.fn(), + })), + }; +}); + +const getIsolationScopeMock = SentryCore.getIsolationScope as ReturnType; + +// One isolation scope per request, as in a server. A shared scope keeps the dedup marker of the +// Sentry middleware, which then skips the next request. +function freshIsolationScope(): void { + getIsolationScopeMock.mockReturnValue({ + setTransactionName: mockSetTransactionName, + setSDKProcessingMetadata: vi.fn(), + }); +} + +function createApp({ restoreRouteIndex }: { restoreRouteIndex: boolean }): Hono { + const app = new Hono(); + applyHonoPatches(app); + app.use(createHonoRequestMiddleware({ runtime: 'node' })); + + if (restoreRouteIndex) { + // Like Hono 5, which resets `routeIndex` after `next()` (honojs/hono#4424). + app.use(async (c, next) => { + const routeIndex = c.req.routeIndex; + await next(); + c.req.routeIndex = routeIndex; + }); + } + + app.use('/users/*', async (_c, next) => { + await next(); + }); + // A route handler from `.use()`, which also gets a middleware span wrapper. + app.use('/users/via-use', async c => c.text('via use')); + app.get('/users/me', c => c.text('me')); + app.get('/users/:id', c => c.text('user')); + app.use('/mw-only/*', async (_c, _next) => new Response('handled by middleware')); + + // A middleware that returns early. Of the matched route handlers, Hono would have run `/blocked/me`. + app.use('/blocked/*', async (c, _next) => c.text('blocked', 401)); + app.get('/blocked/me', c => c.text('me')); + app.get('/blocked/:id', c => c.text('user')); + app.get('/blocked/*', c => c.text('catch-all')); + + const subApp = new Hono(); + subApp.get('/items/:itemId', c => c.text('item')); + app.route('/sub', subApp); + + // Passes `draft` on to the catch-all below, which then names the request. + app.get('/posts/:id', async (c, next) => { + if (c.req.param('id') === 'draft') { + await next(); + return; + } + return c.text('post'); + }); + app.get('/posts/*', c => c.text('fallback')); + + // A catch-all registered last. It also matches `/users/me` and `/users/42`, but never responds. + app.get('/users/*', c => c.text('catch-all')); + return app; +} + +// Returns a function that restores the property. +function overrideRequestProperty(key: PropertyKey, descriptor: PropertyDescriptor): () => void { + const original = Object.getOwnPropertyDescriptor(HonoRequest.prototype, key); + Object.defineProperty(HonoRequest.prototype, key, { configurable: true, ...descriptor }); + return () => { + if (original) { + Object.defineProperty(HonoRequest.prototype, key, original); + } + }; +} + +const hideRequestProperty = (key: PropertyKey): (() => void) => overrideRequestProperty(key, { value: undefined }); + +// The symbol of the match result getter, which `hono/route` reads. +const [matchResultKey] = Object.getOwnPropertySymbols(HonoRequest.prototype); +if (!matchResultKey) { + throw new Error('The match result getter is missing on `HonoRequest.prototype`.'); +} + +const SOURCES: Array<{ source: string; hidden: PropertyKey[]; restoreRouteIndex: boolean }> = [ + { source: 'Hono 4', hidden: [], restoreRouteIndex: false }, + // Hono 5 has no `c.req.matchedRoutes`. + { source: 'Hono 5 (simulated)', hidden: ['matchedRoutes'], restoreRouteIndex: true }, +]; + +const CASES: Array<[path: string, expected: string]> = [ + ['/users/me', 'GET /users/me'], + ['/users/42', 'GET /users/:id'], + ['/users/via-use', 'GET /users/via-use'], + ['/users/all/the/things', 'GET /users/*'], + ['/sub/items/7', 'GET /sub/items/:itemId'], + ['/mw-only/anything', 'GET /mw-only/*'], + ['/blocked/me', 'GET /blocked/me'], + ['/posts/1', 'GET /posts/:id'], + ['/posts/draft', 'GET /posts/*'], +]; + +describe.each(SOURCES)('route name on $source', ({ hidden, restoreRouteIndex }) => { + let restores: Array<() => void> = []; + + beforeEach(() => { + vi.clearAllMocks(); + restores = hidden.map(hideRequestProperty); + }); + + afterEach(() => { + restores.forEach(restore => restore()); + }); + + it.each(CASES)('names %s as %s', async (path, expected) => { + freshIsolationScope(); + const app = createApp({ restoreRouteIndex }); + + await app.request(path); + + expect(mockSetTransactionName).toHaveBeenLastCalledWith(expected); + }); + + it('does not name a request without a route', async () => { + freshIsolationScope(); + const app = createApp({ restoreRouteIndex }); + + await app.request('/does-not-exist'); + + expect(mockSetTransactionName).not.toHaveBeenCalled(); + }); +}); + +describe('route name with another symbol-keyed getter on the request', () => { + let restores: Array<() => void> = []; + + beforeEach(() => { + vi.clearAllMocks(); + restores = [ + hideRequestProperty('matchedRoutes'), + overrideRequestProperty(Symbol('requestId'), { get: () => 'req-7f3a' }), + ]; + }); + + afterEach(() => { + restores.forEach(restore => restore()); + }); + + it('still finds the match result getter', async () => { + freshIsolationScope(); + const app = createApp({ restoreRouteIndex: true }); + + await app.request('/users/me'); + + expect(mockSetTransactionName).toHaveBeenLastCalledWith('GET /users/me'); + }); +}); + +describe('route name from a match result of an unknown shape', () => { + let restores: Array<() => void> = []; + + beforeEach(() => { + vi.clearAllMocks(); + restores = [ + hideRequestProperty('matchedRoutes'), + overrideRequestProperty(matchResultKey, { get: () => [[[null]], {}] }), + ]; + }); + + afterEach(() => { + restores.forEach(restore => restore()); + }); + + it('keeps the request working and does not name it', async () => { + freshIsolationScope(); + const app = createApp({ restoreRouteIndex: true }); + + const response = await app.request('/users/me'); + + expect(response.status).toBe(200); + expect(mockSetTransactionName).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/server-utils/src/integrations/hono/constants.ts b/packages/server-utils/src/integrations/hono/constants.ts index 297f3a469271..fff246e8bfce 100644 --- a/packages/server-utils/src/integrations/hono/constants.ts +++ b/packages/server-utils/src/integrations/hono/constants.ts @@ -2,3 +2,9 @@ // Shared by the `app.request` Proxy (`patchAppRequest`) and the orchestrion channel subscriber // (`instrumentInternalRequests` in `honoIntegration`), which both open this span. export const INTERNAL_REQUEST_ORIGIN = 'auto.http.hono.internal_request'; + +// Marks the Sentry request/response middleware so the span-wrapping patches never turn it into a +// middleware span — most importantly when an auto-instrumented sub-app carrying this middleware is +// mounted into a parent and `wrapSubAppMiddleware` wraps its handlers. Read by `wrapMiddlewareWithSpan` +// and `resolveRouteName`. +export const SENTRY_HONO_MIDDLEWARE = '__SENTRY_HONO_MIDDLEWARE__'; diff --git a/packages/server-utils/src/integrations/hono/createHonoMiddleware.ts b/packages/server-utils/src/integrations/hono/createHonoMiddleware.ts index 2540fc059871..1c5087cf567e 100644 --- a/packages/server-utils/src/integrations/hono/createHonoMiddleware.ts +++ b/packages/server-utils/src/integrations/hono/createHonoMiddleware.ts @@ -1,14 +1,10 @@ import { addNonEnumerableProperty, getDefaultIsolationScope, getIsolationScope } from '@sentry/core'; import type { HonoRuntime } from './connInfo'; +import { SENTRY_HONO_MIDDLEWARE } from './constants'; import type { Context, MiddlewareHandler } from './honoTypes'; import { captureContextError, requestHandler, responseHandler } from './middlewareHandlers'; import type { SentryHonoMiddlewareOptions } from './types'; -// Marks the Sentry request/response middleware so the span-wrapping patches never turn it into a -// middleware span — most importantly when an auto-instrumented sub-app carrying this middleware is -// mounted into a parent and `wrapSubAppMiddleware` wraps its handlers. Read by `wrapMiddlewareWithSpan`. -export const SENTRY_HONO_MIDDLEWARE = '__SENTRY_HONO_MIDDLEWARE__'; - // `Symbol.for` (global registry) so the markers are shared even if this module is evaluated from more // than one copy of `@sentry/server-utils`. const HONO_REQUEST_HANDLED = Symbol.for('sentry.hono.requestHandled'); diff --git a/packages/server-utils/src/integrations/hono/honoIntegration.ts b/packages/server-utils/src/integrations/hono/honoIntegration.ts index 575462d6d018..389b4fe951db 100644 --- a/packages/server-utils/src/integrations/hono/honoIntegration.ts +++ b/packages/server-utils/src/integrations/hono/honoIntegration.ts @@ -14,7 +14,7 @@ import { applyPatches } from './applyPatches'; import { INTERNAL_REQUEST_ORIGIN } from './constants'; import { createHonoRequestMiddleware } from './createHonoMiddleware'; import { getMiddlewareFlags } from './isMiddleware'; -import { isMatchedHandlerList } from './matchResult'; +import { isMatchedHandlerList, wrapRouteHandlers } from './matchResult'; import { extractPathname, isInternalRequestSpanActive } from './patchAppRequest'; import { wrapMiddlewareWithSpan } from './wrapMiddlewareSpan'; import type { SentryHonoMiddlewareOptions } from './types'; @@ -50,8 +50,7 @@ export function honoMiddleware(app: Hono, options: HonoIntegra }); } -function wrapMatchedMiddleware(handlers: MatchedHandler[]): void { - const middlewareFlags = getMiddlewareFlags(handlers.map(([[, route]]) => route)); +function wrapMatchedMiddleware(handlers: MatchedHandler[], middlewareFlags: boolean[]): void { for (const [i, [pair]] of handlers.entries()) { if (middlewareFlags[i]) { pair[0] = wrapMiddlewareWithSpan(pair[0]); @@ -63,7 +62,9 @@ function wrapMatchedMiddleware(handlers: MatchedHandler[]): void { * Per-request Context hook. `#dispatch` builds `new Context(req, { matchResult })` before its * single-handler fast-path check, so from the live `matchResult` we: * 1. wrap the matched middleware handlers for spans (route handlers are covered by the request span); - * 2. prepend the Sentry request/response middleware — it drives route naming, request data and error + * 2. wrap the matched route handlers if one can call `next()`, so the route name follows the handler + * that responds; + * 3. prepend the Sentry request/response middleware — it drives route naming, request data and error * capture from inside the chain, and forces the ≥2-handler `compose` path so the fast-path is * never taken. * @@ -82,7 +83,9 @@ function injectHonoInstrumentation( injectedHandlerLists.add(handlers); if (isMatchedHandlerList(handlers)) { - wrapMatchedMiddleware(handlers); + const middlewareFlags = getMiddlewareFlags(handlers.map(([[, route]]) => route)); + wrapMatchedMiddleware(handlers, middlewareFlags); + wrapRouteHandlers(handlers, middlewareFlags); } // Prepend the Sentry request/response middleware. Its arity marks it as a middleware, so its `/*` diff --git a/packages/server-utils/src/integrations/hono/matchResult.ts b/packages/server-utils/src/integrations/hono/matchResult.ts index e6bd298f1206..0c670269ffd8 100644 --- a/packages/server-utils/src/integrations/hono/matchResult.ts +++ b/packages/server-utils/src/integrations/hono/matchResult.ts @@ -1,5 +1,16 @@ -import { isPlainObject } from '@sentry/core'; -import type { MatchedHandler } from './honoTypes'; +import { addNonEnumerableProperty, isPlainObject } from '@sentry/core'; +import type { Context, HonoRoute, MatchedHandler } from './honoTypes'; +import { isMiddleware } from './isMiddleware'; + +// `routeIndex` of the route handler that started last. `Symbol.for` shares it between SDK copies. +const ROUTE_HANDLER_INDEX = Symbol.for('sentry.hono.routeHandlerIndex'); + +// Marks the index wrapper. The router reuses the match result for each request to a route, so without +// it the wrappers pile up. A span wrapper does not count: a route handler from `app.use()` has one and +// still needs this wrapper. +const RECORDS_ROUTE_HANDLER_INDEX = Symbol.for('sentry.hono.recordsRouteHandlerIndex'); + +const ON_ROUTE_HANDLER_START = Symbol.for('sentry.hono.onRouteHandlerStart'); function isMatchedHandler(entry: unknown): entry is MatchedHandler { const pair: unknown = Array.isArray(entry) ? entry[0] : undefined; @@ -17,10 +28,91 @@ function isMatchedHandler(entry: unknown): entry is MatchedHandler { ); } -/** - * Checks the entry shape that `compose` and `hono/route` read. The match result is a Hono internal, so - * the SDK leaves other shapes alone. - */ +/** The match result is internal to Hono. The SDK uses it only in the shape that `compose` reads. */ export function isMatchedHandlerList(handlers: unknown): handlers is MatchedHandler[] { return Array.isArray(handlers) && handlers.every(isMatchedHandler); } + +/** + * Reads the match result like `hono/route` does (Hono 4.8+). Hono does not export the getter's symbol, + * so this looks for a symbol getter that returns a match result. + * https://github.com/honojs/hono/blob/v5.0.0-rc.1/src/helper/route/index.ts#L32-L34 + */ +export function getMatchedHandlers(context: Context): MatchedHandler[] | undefined { + const requestProto: object | null = Object.getPrototypeOf(context.req); + if (!requestProto) { + return undefined; + } + + for (const key of Object.getOwnPropertySymbols(requestProto)) { + if (typeof Object.getOwnPropertyDescriptor(requestProto, key)?.get !== 'function') { + continue; + } + const matchResult: unknown = Reflect.get(context.req, key); + if (Array.isArray(matchResult) && isMatchedHandlerList(matchResult[0])) { + return matchResult[0]; + } + } + return undefined; +} + +/** Runs `callback` each time a route handler of this request starts. */ +export function onRouteHandlerStart(context: Context, callback: () => void): void { + addNonEnumerableProperty(context, ON_ROUTE_HANDLER_START, callback); +} + +/** + * Wraps the route handlers so that each stores `c.req.routeIndex` when it starts. Hono 5 resets the + * index after `next()` (https://github.com/honojs/hono/pull/4424). + * + * Wraps only if a route handler declares `next`. Otherwise the first route handler also responds. + * Returns `true` if it wrapped. + * + * Safe to call during the request: `compose` reads each handler only when it calls it. + */ +export function wrapRouteHandlers(handlers: MatchedHandler[], middlewareFlags: boolean[]): boolean { + // For a route handler, `isMiddleware()` tells if it declares `next`. + const canPassOn = handlers.some(([[handler]], i) => !middlewareFlags[i] && isMiddleware(handler)); + if (!canPassOn) { + return false; + } + + for (const [i, [pair]] of handlers.entries()) { + if (!middlewareFlags[i]) { + pair[0] = recordRouteHandlerIndex(pair[0]); + } + } + return true; +} + +function recordRouteHandlerIndex(handler: HonoRoute['handler']): HonoRoute['handler'] { + if (Reflect.get(handler, RECORDS_ROUTE_HANDLER_INDEX)) { + return handler; + } + + return new Proxy(handler, { + apply(target, thisArg, args: [Context, ...unknown[]]) { + const [context] = args; + addNonEnumerableProperty(context, ROUTE_HANDLER_INDEX, context.req.routeIndex); + const onStart: unknown = Reflect.get(context, ON_ROUTE_HANDLER_START); + if (typeof onStart === 'function') { + onStart(); + } + return Reflect.apply(target, thisArg, args); + }, + get(target, prop, receiver) { + if (prop === RECORDS_ROUTE_HANDLER_INDEX) { + return true; + } + if (prop === '__sentry_original__') { + return handler; + } + return Reflect.get(target, prop, receiver); + }, + }); +} + +export function getRecordedRouteHandlerIndex(context: Context): number | undefined { + const index: unknown = Reflect.get(context, ROUTE_HANDLER_INDEX); + return typeof index === 'number' ? index : undefined; +} diff --git a/packages/server-utils/src/integrations/hono/middlewareHandlers.ts b/packages/server-utils/src/integrations/hono/middlewareHandlers.ts index 6ca799b3e340..520e11ee9d38 100644 --- a/packages/server-utils/src/integrations/hono/middlewareHandlers.ts +++ b/packages/server-utils/src/integrations/hono/middlewareHandlers.ts @@ -8,6 +8,7 @@ import { INTERNAL_setSegmentNameSourceIfSegment, updateSpanName, type Scope, + type Span, type SpanAttributes, spanToJSON, winterCGHeadersToDict, @@ -18,6 +19,8 @@ import type { Context } from './honoTypes'; import { type HonoRuntime, readConnInfo } from './connInfo'; import { hasFetchEvent } from './hono-context'; import { defaultShouldHandleError } from './defaultShouldHandleError'; +import { getMiddlewareFlags } from './isMiddleware'; +import { getMatchedHandlers, onRouteHandlerStart, wrapRouteHandlers } from './matchResult'; import { resolveRouteName } from './resolveRouteName'; import { type SentryHonoMiddlewareOptions } from './types'; import { @@ -35,10 +38,24 @@ import { */ export function requestHandler(context: Context, runtime: HonoRuntime): void { const isolationScope = getCurrentIsolationScope(); - - // Set a provisional route name as early as possible so events captured during the request already carry the route. - // It is re-resolved in `responseHandler` once the middleware chain has run. - updateSpanRouteName(isolationScope, context); + // Read now: when a route handler starts, a user span can be active. + const span = getActiveSpan(); + + // Hono 5 removes `c.req.matchedRoutes`. Before Hono 4.8, it is the only source. + const matchedHandlers = getMatchedHandlers(context); + const routes = matchedHandlers?.map(([[, route]]) => route) ?? context.req.matchedRoutes ?? []; + const middlewareFlags = getMiddlewareFlags(routes); + const nameSpan = (): void => + updateSpanRouteName(isolationScope, span, context, resolveRouteName(context, routes, middlewareFlags)); + + // Name early, so that events from the handlers carry the route. + nameSpan(); + + // A route handler that calls `next()` passes the request on, so rename when each one starts. Without + // the getter, only the Context hook can wrap the route handlers. + if (!matchedHandlers || wrapRouteHandlers(matchedHandlers, middlewareFlags)) { + onRouteHandlerStart(context, nameSpan); + } isolationScope.setSDKProcessingMetadata({ normalizedRequest: winterCGRequestToRequestData(hasFetchEvent(context) ? context.event.request : context.req.raw), @@ -116,17 +133,13 @@ export function responseHandler( context: Context, shouldHandleError?: SentryHonoMiddlewareOptions['shouldHandleError'], ): void { - // Overwrite the route name now that the middleware chain has run: `routeIndex` is accurate here - updateSpanRouteName(getCurrentIsolationScope(), context); - captureContextError(context, shouldHandleError); } /** * Captures an unhandled context error, gated by the user's `shouldHandleError` (or the default). * - * Split out from {@link responseHandler} so a deduplicated middleware can still report its own - * context's error without re-resolving the route name or overwriting request data — e.g. the inner + * A deduplicated middleware calls it directly to report the error of its own context — e.g. the inner * context of an internal `.request()` whose route threw but whose failed response the outer handler * swallowed. */ @@ -143,10 +156,18 @@ export function captureContextError( } } -function updateSpanRouteName(isolationScope: Scope, context: Context): void { - const route = resolveRouteName(context); +function updateSpanRouteName( + isolationScope: Scope, + activeSpan: Span | undefined, + context: Context, + route: string | undefined, +): void { + // Without a route, keep the span name from the runtime. + if (route === undefined) { + return; + } + const routeName = `${context.req.method} ${route}`; - const activeSpan = getActiveSpan(); if (activeSpan) { activeSpan.updateName(routeName); diff --git a/packages/server-utils/src/integrations/hono/resolveRouteName.ts b/packages/server-utils/src/integrations/hono/resolveRouteName.ts index 97b1283722ed..2f96bf83e278 100644 --- a/packages/server-utils/src/integrations/hono/resolveRouteName.ts +++ b/packages/server-utils/src/integrations/hono/resolveRouteName.ts @@ -1,50 +1,40 @@ import type { Context, HonoRoute } from './honoTypes'; -import { isMiddleware } from './isMiddleware'; - -// Arity alone is enough here (unlike `wrapSubAppMiddleware` in patchRoute.ts, which also needs position) -// We only want the path, and inline middleware shares its handler's path. -function isRouteHandler(handler: unknown): boolean { - return typeof handler === 'function' && !isMiddleware(handler); -} - -// Read the request's own matched routes rather than importing the `hono/route` helpers, so the -// instrumentation needs no runtime import from `hono`. `c.req.matchedRoutes` and `c.req.routePath` -// are the (deprecated but public) getters those helpers wrap; `routePath(c, index)` is reimplemented -// here as `matchedRoutes(c).at(index)?.path` so an arbitrary index (e.g. -1) can be resolved. -function matchedRoutes(context: Context): HonoRoute[] { - return context.req.matchedRoutes ?? []; -} - -function routePath(context: Context, index?: number): string { - const routes = matchedRoutes(context); - return routes.at(index ?? context.req.routeIndex)?.path ?? ''; -} +import { SENTRY_HONO_MIDDLEWARE } from './constants'; +import { getRecordedRouteHandlerIndex } from './matchResult'; /** - * Resolves the route path of the matched handler for the transaction name. - * - * Picking the handler (not just `routePath`) avoids two failure modes: a catch-all middleware - * registered after the handlers (`routePath(c, -1)` would return just `/*`), and a middleware that - * short-circuits before the handler runs (`routePath(c)` would return the middleware's path). + * Returns the path of the route that handles the request, or `undefined` if the request has no route. + * Not the last matched route: a catch-all registered later also matches. */ -export function resolveRouteName(context: Context): string { - const routes = matchedRoutes(context); +export function resolveRouteName( + context: Context, + routes: HonoRoute[], + middlewareFlags: boolean[], +): string | undefined { + const recordedIndex = getRecordedRouteHandlerIndex(context); + const recorded = recordedIndex === undefined ? undefined : routes[recordedIndex]; + if (recorded) { + return recorded.path; + } - // Trust routeIndex when it lands on a handler - to disambiguate overlapping handlers. - const current = routes[context.req.routeIndex]; - if (current && isRouteHandler(current.handler)) { - return current.path; + // Hono runs the first matched route handler first. If a middleware returned early, it is the one that + // would have run. + const firstRouteHandler = routes.find((_route, i) => !middlewareFlags[i]); + if (firstRouteHandler) { + return firstRouteHandler.path; } - // A middleware short-circuited, so routeIndex is stuck on it: fall back to the last matched handler. + // Only middleware matched, for example `app.use('/static/*', serveStatic())`. The Sentry middleware + // and `/*` middleware say nothing about the route. for (let i = routes.length - 1; i >= 0; i--) { const route = routes[i]; - if (route && isRouteHandler(route.handler)) { + if (route && route.path !== '/*' && !isSentryMiddleware(route.handler)) { return route.path; } } + return undefined; +} - // No handler matched (middleware-only path) - // Final matched route: https://hono.dev/docs/helpers/route#using-with-index-parameter - return routePath(context, -1); +function isSentryMiddleware(handler: unknown): boolean { + return typeof handler === 'function' && Boolean(Reflect.get(handler, SENTRY_HONO_MIDDLEWARE)); } diff --git a/packages/server-utils/src/integrations/hono/wrapMiddlewareSpan.ts b/packages/server-utils/src/integrations/hono/wrapMiddlewareSpan.ts index 86f96e05039c..29f8d97286a4 100644 --- a/packages/server-utils/src/integrations/hono/wrapMiddlewareSpan.ts +++ b/packages/server-utils/src/integrations/hono/wrapMiddlewareSpan.ts @@ -9,7 +9,7 @@ import { type WrappedFunction, } from '@sentry/core'; import { type MiddlewareHandler } from './honoTypes'; -import { SENTRY_HONO_MIDDLEWARE } from './createHonoMiddleware'; +import { SENTRY_HONO_MIDDLEWARE } from './constants'; import { defaultShouldHandleError } from './defaultShouldHandleError'; const MIDDLEWARE_ORIGIN = 'auto.middleware.hono';