diff --git a/dev-packages/e2e-tests/test-applications/hono-4/src/route-groups/test-errors.ts b/dev-packages/e2e-tests/test-applications/hono-4/src/route-groups/test-errors.ts new file mode 100644 index 000000000000..b8f2fd96fe93 --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/hono-4/src/route-groups/test-errors.ts @@ -0,0 +1,45 @@ +import { Hono } from 'hono'; +import { HTTPException } from 'hono/http-exception'; + +const errorRoutes = new Hono(); + +// Middleware that throws a 5xx HTTPException (should be captured) +errorRoutes.use('/middleware-http-exception/*', async (_c, _next) => { + throw new HTTPException(503, { message: 'Service Unavailable from middleware' }); +}); + +errorRoutes.get('/middleware-http-exception', c => c.text('should not reach')); + +// Middleware that throws a 4xx HTTPException (should NOT be captured) +errorRoutes.use('/middleware-http-exception-4xx/*', async (_c, _next) => { + throw new HTTPException(401, { message: 'Unauthorized from middleware' }); +}); + +errorRoutes.get('/middleware-http-exception-4xx', c => c.text('should not reach')); + +// Sub-app with a custom onError handler that swallows errors +const subAppWithOnError = new Hono(); + +subAppWithOnError.onError((err, c) => { + return c.text(`Handled by onError: ${err.message}`, 500); +}); + +subAppWithOnError.get('/fail', () => { + throw new Error('Error caught by custom onError'); +}); + +errorRoutes.route('/custom-on-error', subAppWithOnError); + +// Nested sub-apps: parent mounts child, child route throws +const childApp = new Hono(); + +childApp.get('/error', () => { + throw new Error('Nested child app error'); +}); + +const parentApp = new Hono(); +parentApp.route('/child', childApp); + +errorRoutes.route('/nested', parentApp); + +export { errorRoutes }; 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 e32662fb3b18..598943cad868 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,5 +1,4 @@ import { Hono } from 'hono'; -import { HTTPException } from 'hono/http-exception'; const routePatterns = new Hono(); @@ -24,32 +23,4 @@ METHODS.forEach(method => { routePatterns.on(method, '/on', c => c.text(`${method} on response`)); }); -// Error routes for direct method registration -METHODS.forEach(method => { - routePatterns[method]('/500', () => { - throw new HTTPException(500, { message: 'response 500' }); - }); - routePatterns[method]('/401', () => { - throw new HTTPException(401, { message: 'response 401' }); - }); - routePatterns[method]('/402', () => { - throw new HTTPException(402, { message: 'response 402' }); - }); - routePatterns[method]('/403', () => { - throw new HTTPException(403, { message: 'response 403' }); - }); -}); - -// Error routes for .all() -routePatterns.all('/all/500', () => { - throw new HTTPException(500, { message: 'response 500' }); -}); - -// Error routes for .on() -METHODS.forEach(method => { - routePatterns.on(method, '/on/500', () => { - throw new HTTPException(500, { message: 'response 500' }); - }); -}); - export { routePatterns }; diff --git a/dev-packages/e2e-tests/test-applications/hono-4/src/routes.ts b/dev-packages/e2e-tests/test-applications/hono-4/src/routes.ts index f6efc6dde03c..cfb13146b6f7 100644 --- a/dev-packages/e2e-tests/test-applications/hono-4/src/routes.ts +++ b/dev-packages/e2e-tests/test-applications/hono-4/src/routes.ts @@ -1,6 +1,7 @@ import type { Hono } from 'hono'; import { HTTPException } from 'hono/http-exception'; import { failingMiddleware, middlewareA, middlewareB } from './middleware'; +import { errorRoutes } from './route-groups/test-errors'; import { middlewareRoutes, subAppWithInlineMiddleware, subAppWithMiddleware } from './route-groups/test-middleware'; import { routePatterns } from './route-groups/test-route-patterns'; @@ -43,4 +44,7 @@ export function addRoutes(app: Hono<{ Bindings?: { E2E_TEST_DSN: string } }>): v // Route patterns: HTTP methods, .all(), .on(), sync/async, errors app.route('/test-routes', routePatterns); + + // Error-specific routes: onError handler, nested sub-apps, middleware HTTPException + app.route('/test-errors', errorRoutes); } diff --git a/dev-packages/e2e-tests/test-applications/hono-4/tests/errors.test.ts b/dev-packages/e2e-tests/test-applications/hono-4/tests/errors.test.ts index 832204237946..98c81d30afeb 100644 --- a/dev-packages/e2e-tests/test-applications/hono-4/tests/errors.test.ts +++ b/dev-packages/e2e-tests/test-applications/hono-4/tests/errors.test.ts @@ -1,53 +1,250 @@ import { expect, test } from '@playwright/test'; -import { waitForError } from '@sentry-internal/test-utils'; -import { APP_NAME } from './constants'; +import { waitForError, waitForTransaction } from '@sentry-internal/test-utils'; +import { APP_NAME, RUNTIME } from './constants'; -test('captures error thrown in route handler', async ({ baseURL }) => { - const errorWaiter = waitForError(APP_NAME, event => { - return event.exception?.values?.[0]?.value === 'This is a test error for Sentry!'; - }); +test.describe('route handler errors', () => { + test('captures error with mechanism and trace correlation', async ({ baseURL }) => { + const errorPromise = waitForError(APP_NAME, event => { + return event.exception?.values?.[0]?.value === 'This is a test error for Sentry!'; + }); + + const transactionPromise = waitForTransaction(APP_NAME, event => { + return event.contexts?.trace?.op === 'http.server' && !!event.transaction?.includes('/error/'); + }); + + const response = await fetch(`${baseURL}/error/test-cause`); + expect(response.status).toBe(500); + + const errorEvent = await errorPromise; + const transactionEvent = await transactionPromise; + + expect(transactionEvent.transaction).toBe('GET /error/:cause'); + + expect(errorEvent.exception?.values).toHaveLength(1); + + const exception = errorEvent.exception?.values?.[0]; + expect(exception?.value).toBe('This is a test error for Sentry!'); + expect(exception?.mechanism).toEqual({ + handled: false, + type: 'auto.http.hono.context_error', + }); - const response = await fetch(`${baseURL}/error/test-cause`); - expect(response.status).toBe(500); + expect(errorEvent.transaction).toBe('GET /error/:cause'); + expect(errorEvent.request?.method).toBe('GET'); + expect(errorEvent.request?.url).toContain('/error/test-cause'); - const event = await errorWaiter; - expect(event.exception?.values?.[0]?.value).toBe('This is a test error for Sentry!'); + expect(errorEvent.contexts?.trace?.trace_id).toBe(transactionEvent.contexts?.trace?.trace_id); + }); }); -test('captures HTTPException with 502 status', async ({ baseURL }) => { - const errorWaiter = waitForError(APP_NAME, event => { - return event.exception?.values?.[0]?.value === 'HTTPException 502'; +test.describe('HTTPException errors', () => { + test('captures 5xx HTTPException', async ({ baseURL }) => { + const errorPromise = waitForError(APP_NAME, event => { + return event.exception?.values?.[0]?.value === 'HTTPException 500'; + }); + + const response = await fetch(`${baseURL}/http-exception/500`); + expect(response.status).toBe(500); + + const errorEvent = await errorPromise; + expect(errorEvent.exception?.values?.[0]?.value).toBe('HTTPException 500'); + expect(errorEvent.exception?.values?.[0]?.mechanism).toEqual({ + handled: false, + type: 'auto.http.hono.context_error', + }); }); - const response = await fetch(`${baseURL}/http-exception/502`); - expect(response.status).toBe(502); + // On Node/Bun, httpServerSpansIntegration drops transactions for 3xx/4xx responses (ignoreStatusCodes), so we just use a request guard. + // On Cloudflare the transaction is available, and we additionally verify its name. + [301, 302].forEach(code => { + test(`does not capture ${code} HTTPException`, async ({ baseURL }) => { + let errorEventOccurred = false; + + waitForError(APP_NAME, event => { + if (event.exception?.values?.[0]?.value === `HTTPException ${code}`) { + errorEventOccurred = true; + } + return false; + }); + + const transactionPromise = waitForTransaction(APP_NAME, event => { + return RUNTIME === 'cloudflare' + ? event.contexts?.trace?.op === 'http.server' && !!event.transaction?.includes('/http-exception/') + : event.contexts?.trace?.op === 'http.server' && event.transaction === 'GET /'; + }); + + const response = await fetch(`${baseURL}/http-exception/${code}`, { redirect: 'manual' }); + expect(response.status).toBe(code); + + if (RUNTIME !== 'cloudflare') { + // Simple request guard for non-Cloudflare runtimes since the other transaction is dropped for 4xx responses + await fetch(`${baseURL}/`); + } + + const transaction = await transactionPromise; + + if (RUNTIME === 'cloudflare') { + expect(transaction.transaction).toBe('GET /http-exception/:code'); + } + + expect(errorEventOccurred).toBe(false); + }); + }); + + [401, 403, 404].forEach(code => { + test(`does not capture ${code} HTTPException`, async ({ baseURL }) => { + let errorEventOccurred = false; + + waitForError(APP_NAME, event => { + if (event.exception?.values?.[0]?.value === `HTTPException ${code}`) { + errorEventOccurred = true; + } + return false; + }); + + const transactionPromise = waitForTransaction(APP_NAME, event => { + return RUNTIME === 'cloudflare' + ? event.contexts?.trace?.op === 'http.server' && !!event.transaction?.includes('/http-exception/') + : event.contexts?.trace?.op === 'http.server' && event.transaction === 'GET /'; + }); + + const response = await fetch(`${baseURL}/http-exception/${code}`); + expect(response.status).toBe(code); + + if (RUNTIME !== 'cloudflare') { + // Simple request guard for non-Cloudflare runtimes since the other transaction is dropped for 4xx responses + await fetch(`${baseURL}/`); + } + + const transaction = await transactionPromise; + + if (RUNTIME === 'cloudflare') { + expect(transaction.transaction).toBe('GET /http-exception/:code'); + } - const event = await errorWaiter; - expect(event.exception?.values?.[0]?.value).toBe('HTTPException 502'); + expect(errorEventOccurred).toBe(false); + }); + }); }); -// TODO: 401 and 404 HTTPExceptions should not be captured by Sentry by default, -// but currently they are. Fix the filtering and update these tests accordingly. -test('captures HTTPException with 401 status', async ({ baseURL }) => { - const errorWaiter = waitForError(APP_NAME, event => { - return event.exception?.values?.[0]?.value === 'HTTPException 401'; +test.describe('middleware errors', () => { + test('captures 5xx HTTPException thrown in middleware with error span status', async ({ baseURL }) => { + const errorPromise = waitForError(APP_NAME, event => { + return event.exception?.values?.[0]?.value === 'Service Unavailable from middleware'; + }); + + const transactionPromise = waitForTransaction(APP_NAME, event => { + return ( + event.contexts?.trace?.op === 'http.server' && + !!event.transaction?.includes('/test-errors/middleware-http-exception') + ); + }); + + const response = await fetch(`${baseURL}/test-errors/middleware-http-exception`); + expect(response.status).toBe(503); + + const errorEvent = await errorPromise; + expect(errorEvent.exception?.values?.[0]?.value).toBe('Service Unavailable from middleware'); + expect(errorEvent.exception?.values?.[0]?.mechanism?.type).toBe('auto.middleware.hono'); + expect(errorEvent.exception?.values?.[0]?.mechanism?.handled).toBe(false); + + const transaction = await transactionPromise; + const middlewareSpan = (transaction.spans || []).find(s => s.op === 'middleware.hono'); + expect(middlewareSpan?.status).toBe('internal_error'); }); - const response = await fetch(`${baseURL}/http-exception/401`); - expect(response.status).toBe(401); + test('does not capture 4xx HTTPException thrown in middleware', async ({ baseURL }) => { + let errorEventOccurred = false; + + waitForError(APP_NAME, event => { + if (event.exception?.values?.[0]?.value === 'Unauthorized from middleware') { + errorEventOccurred = true; + } + return false; + }); + + const transactionPromise = waitForTransaction(APP_NAME, event => { + if (RUNTIME === 'cloudflare') { + return ( + event.contexts?.trace?.op === 'http.server' && + !!event.transaction?.includes('/test-errors/middleware-http-exception-4xx') + ); + } + return event.contexts?.trace?.op === 'http.server' && event.transaction === 'GET /'; + }); - const event = await errorWaiter; - expect(event.exception?.values?.[0]?.value).toBe('HTTPException 401'); + const response = await fetch(`${baseURL}/test-errors/middleware-http-exception-4xx`); + expect(response.status).toBe(401); + + if (RUNTIME !== 'cloudflare') { + await fetch(`${baseURL}/`); + } + + const transaction = await transactionPromise; + + if (RUNTIME === 'cloudflare') { + expect(transaction.transaction).toBe('GET /test-errors/middleware-http-exception-4xx/*'); + + const middlewareSpan = (transaction.spans || []).find(s => s.op === 'middleware.hono'); + expect(middlewareSpan?.status).not.toBe('internal_error'); + } + + expect(errorEventOccurred).toBe(false); + }); }); -test('captures HTTPException with 404 status', async ({ baseURL }) => { - const errorWaiter = waitForError(APP_NAME, event => { - return event.exception?.values?.[0]?.value === 'HTTPException 404'; +test.describe('nested sub-app errors', () => { + test('captures error from nested child sub-app', async ({ baseURL }) => { + const errorPromise = waitForError(APP_NAME, event => { + return event.exception?.values?.[0]?.value === 'Nested child app error'; + }); + + const transactionPromise = waitForTransaction(APP_NAME, event => { + return event.contexts?.trace?.op === 'http.server' && !!event.transaction?.includes('/nested/child/error'); + }); + + const response = await fetch(`${baseURL}/test-errors/nested/child/error`); + expect(response.status).toBe(500); + + const errorEvent = await errorPromise; + const transaction = await transactionPromise; + + expect(transaction.transaction).toBe('GET /test-errors/nested/child/error'); + + expect(errorEvent.exception?.values?.[0]?.value).toBe('Nested child app error'); + expect(errorEvent.exception?.values?.[0]?.mechanism).toEqual({ + handled: false, + type: 'auto.http.hono.context_error', + }); + expect(errorEvent.request?.url).toContain('/test-errors/nested/child/error'); }); +}); - const response = await fetch(`${baseURL}/http-exception/404`); - expect(response.status).toBe(404); +test.describe('custom onError handler', () => { + test('captures error even when onError handles the response', async ({ baseURL }) => { + const errorPromise = waitForError(APP_NAME, event => { + return event.exception?.values?.[0]?.value === 'Error caught by custom onError'; + }); - const event = await errorWaiter; - expect(event.exception?.values?.[0]?.value).toBe('HTTPException 404'); + const transactionPromise = waitForTransaction(APP_NAME, event => { + return event.contexts?.trace?.op === 'http.server' && !!event.transaction?.includes('/custom-on-error/fail'); + }); + + const response = await fetch(`${baseURL}/test-errors/custom-on-error/fail`); + expect(response.status).toBe(500); + + const body = await response.text(); + expect(body).toContain('Handled by onError'); + + const errorEvent = await errorPromise; + const transaction = await transactionPromise; + + expect(transaction.transaction).toBe('GET /test-errors/custom-on-error/fail'); + + expect(errorEvent.exception?.values?.[0]?.value).toBe('Error caught by custom onError'); + expect(errorEvent.exception?.values?.[0]?.mechanism).toEqual({ + handled: false, + type: 'auto.http.hono.context_error', + }); + }); }); diff --git a/dev-packages/e2e-tests/test-applications/hono-4/tests/middleware.test.ts b/dev-packages/e2e-tests/test-applications/hono-4/tests/middleware.test.ts index e8431bed67ce..d984ac0d38a8 100644 --- a/dev-packages/e2e-tests/test-applications/hono-4/tests/middleware.test.ts +++ b/dev-packages/e2e-tests/test-applications/hono-4/tests/middleware.test.ts @@ -18,13 +18,15 @@ for (const { name, prefix } of SCENARIOS) { test.describe(name, () => { test('creates a span for named middleware', async ({ baseURL }) => { const transactionPromise = waitForTransaction(APP_NAME, event => { - return event.contexts?.trace?.op === 'http.server' && event.transaction === `GET ${prefix}/named`; + return event.contexts?.trace?.op === 'http.server' && !!event.transaction?.includes(`${prefix}/named`); }); const response = await fetch(`${baseURL}${prefix}/named`); expect(response.status).toBe(200); const transaction = await transactionPromise; + expect(transaction.transaction).toBe(`GET ${prefix}/named`); + const spans = transaction.spans || []; const middlewareSpan = spans.find( @@ -37,9 +39,9 @@ for (const { name, prefix } of SCENARIOS) { description: 'middlewareA', op: 'middleware.hono', origin: 'auto.middleware.hono', - status: 'ok', }), ); + expect(middlewareSpan?.status).not.toBe('internal_error'); // @ts-expect-error timestamp is defined const durationMs = (middlewareSpan?.timestamp - middlewareSpan?.start_timestamp) * 1000; @@ -48,34 +50,37 @@ for (const { name, prefix } of SCENARIOS) { test('creates a span for anonymous middleware', async ({ baseURL }) => { const transactionPromise = waitForTransaction(APP_NAME, event => { - return event.contexts?.trace?.op === 'http.server' && event.transaction === `GET ${prefix}/anonymous`; + return event.contexts?.trace?.op === 'http.server' && !!event.transaction?.includes(`${prefix}/anonymous`); }); const response = await fetch(`${baseURL}${prefix}/anonymous`); expect(response.status).toBe(200); const transaction = await transactionPromise; + expect(transaction.transaction).toBe(`GET ${prefix}/anonymous`); + const spans = transaction.spans || []; - expect(spans).toContainEqual( - expect.objectContaining({ - description: '', - op: 'middleware.hono', - origin: 'auto.middleware.hono', - status: 'ok', - }), + const anonymousSpan = spans.find( + (span: { description?: string; op?: string }) => + span.op === 'middleware.hono' && span.description === '', ); + expect(anonymousSpan).toBeDefined(); + expect(anonymousSpan?.origin).toBe('auto.middleware.hono'); + expect(anonymousSpan?.status).not.toBe('internal_error'); }); test('multiple middleware are sibling spans under the same parent', async ({ baseURL }) => { const transactionPromise = waitForTransaction(APP_NAME, event => { - return event.contexts?.trace?.op === 'http.server' && event.transaction === `GET ${prefix}/multi`; + return event.contexts?.trace?.op === 'http.server' && !!event.transaction?.includes(`${prefix}/multi`); }); const response = await fetch(`${baseURL}${prefix}/multi`); expect(response.status).toBe(200); const transaction = await transactionPromise; + expect(transaction.transaction).toBe(`GET ${prefix}/multi`); + const spans = transaction.spans || []; const middlewareSpans = spans.sort((a, b) => (a.start_timestamp ?? 0) - (b.start_timestamp ?? 0)); @@ -115,12 +120,14 @@ for (const { name, prefix } of SCENARIOS) { test('sets error status on middleware span when middleware throws', async ({ baseURL }) => { const transactionPromise = waitForTransaction(APP_NAME, event => { - return event.contexts?.trace?.op === 'http.server' && event.transaction === `GET ${prefix}/error/*`; + return event.contexts?.trace?.op === 'http.server' && !!event.transaction?.includes(`${prefix}/error`); }); await fetch(`${baseURL}${prefix}/error`); const transaction = await transactionPromise; + expect(transaction.transaction).toBe(`GET ${prefix}/error/*`); + const spans = transaction.spans || []; const failingSpan = spans.find( @@ -153,7 +160,8 @@ test.describe('.all() handler in sub-app', () => { test('does not create middleware span for .all() route handler', async ({ baseURL }) => { const transactionPromise = waitForTransaction(APP_NAME, event => { return ( - event.contexts?.trace?.op === 'http.server' && event.transaction === 'GET /test-subapp-middleware/all-handler' + event.contexts?.trace?.op === 'http.server' && + !!event.transaction?.includes('/test-subapp-middleware/all-handler') ); }); @@ -164,6 +172,8 @@ test.describe('.all() handler in sub-app', () => { expect(body).toEqual({ handler: 'all' }); const transaction = await transactionPromise; + expect(transaction.transaction).toBe('GET /test-subapp-middleware/all-handler'); + const spans = transaction.spans || []; // No middleware is called for this route, so there should be no spans. @@ -191,13 +201,14 @@ test.describe('inline middleware spans (sub-app)', () => { const fullPath = `${INLINE_PREFIX}${regPath}${mwPath}`; const transactionPromise = waitForTransaction(APP_NAME, event => { - return event.contexts?.trace?.op === 'http.server' && event.transaction === `GET ${fullPath}`; + return event.contexts?.trace?.op === 'http.server' && !!event.transaction?.includes(fullPath); }); const response = await fetch(`${baseURL}${fullPath}`); expect(response.status).toBe(200); const transaction = await transactionPromise; + expect(transaction.transaction).toBe(`GET ${fullPath}`); const EXPECTED_DESCRIPTIONS: Record> = { '/direct': { '': 'inlineMiddleware', '/separately': 'inlineSeparateMiddleware' }, @@ -206,14 +217,11 @@ test.describe('inline middleware spans (sub-app)', () => { }; const expectedDescription = EXPECTED_DESCRIPTIONS[regPath]![mwPath]!; - expect(transaction.spans).toContainEqual( - expect.objectContaining({ - description: expectedDescription, - op: 'middleware.hono', - origin: 'auto.middleware.hono', - status: 'ok', - }), - ); + const inlineSpan = (transaction.spans || []).find(s => s.description === expectedDescription); + expect(inlineSpan).toBeDefined(); + expect(inlineSpan?.op).toBe('middleware.hono'); + expect(inlineSpan?.origin).toBe('auto.middleware.hono'); + expect(inlineSpan?.status).not.toBe('internal_error'); }); } } 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 fd6579fe3b17..decd1049b6c9 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,5 @@ import { expect, test } from '@playwright/test'; -import { waitForError, waitForTransaction } from '@sentry-internal/test-utils'; +import { waitForTransaction } from '@sentry-internal/test-utils'; import { APP_NAME } from './constants'; const PREFIX = '/test-routes'; @@ -11,45 +11,45 @@ const REGISTRATION_STYLES = [ ] as const; test.describe('HTTP methods', () => { - for (const method of ['POST', 'PUT', 'DELETE', 'PATCH']) { + ['POST', 'PUT', 'DELETE', 'PATCH'].forEach(method => { test(`sends transaction for ${method}`, async ({ baseURL }) => { const transactionPromise = waitForTransaction(APP_NAME, event => { - return event.contexts?.trace?.op === 'http.server' && event.transaction === `${method} ${PREFIX}`; + return event.contexts?.trace?.op === 'http.server' && !!event.transaction?.includes(PREFIX); }); const response = await fetch(`${baseURL}${PREFIX}`, { method }); expect(response.status).toBe(200); const transaction = await transactionPromise; - expect(transaction.contexts?.trace?.op).toBe('http.server'); expect(transaction.transaction).toBe(`${method} ${PREFIX}`); + expect(transaction.contexts?.trace?.op).toBe('http.server'); }); - } + }); }); test.describe('route registration styles', () => { - for (const { name, path } of REGISTRATION_STYLES) { + REGISTRATION_STYLES.forEach(({ name, path }) => { test(`${name} sends transaction`, async ({ baseURL }) => { const transactionPromise = waitForTransaction(APP_NAME, event => { - return event.contexts?.trace?.op === 'http.server' && event.transaction === `GET ${PREFIX}${path}`; + return event.contexts?.trace?.op === 'http.server' && !!event.transaction?.includes(`${PREFIX}${path}`); }); const response = await fetch(`${baseURL}${PREFIX}${path}`); expect(response.status).toBe(200); const transaction = await transactionPromise; - expect(transaction.contexts?.trace?.op).toBe('http.server'); expect(transaction.transaction).toBe(`GET ${PREFIX}${path}`); + expect(transaction.contexts?.trace?.op).toBe('http.server'); }); - } + }); - for (const { name, path } of [ + [ { name: '.all()', path: '/all' }, { name: '.on()', path: '/on' }, - ]) { + ].forEach(({ name, path }) => { test(`${name} responds to POST`, async ({ baseURL }) => { const transactionPromise = waitForTransaction(APP_NAME, event => { - return event.contexts?.trace?.op === 'http.server' && event.transaction === `POST ${PREFIX}${path}`; + return event.contexts?.trace?.op === 'http.server' && !!event.transaction?.includes(`${PREFIX}${path}`); }); const response = await fetch(`${baseURL}${PREFIX}${path}`, { method: 'POST' }); @@ -58,87 +58,18 @@ test.describe('route registration styles', () => { const transaction = await transactionPromise; expect(transaction.transaction).toBe(`POST ${PREFIX}${path}`); }); - } + }); }); test('async handler sends transaction', async ({ baseURL }) => { const transactionPromise = waitForTransaction(APP_NAME, event => { - return event.contexts?.trace?.op === 'http.server' && event.transaction === `GET ${PREFIX}/async`; + return event.contexts?.trace?.op === 'http.server' && !!event.transaction?.includes(`${PREFIX}/async`); }); const response = await fetch(`${baseURL}${PREFIX}/async`); expect(response.status).toBe(200); const transaction = await transactionPromise; + expect(transaction.transaction).toBe(`GET ${PREFIX}/async`); expect(transaction.contexts?.trace?.op).toBe('http.server'); }); - -test.describe('500 HTTPException capture', () => { - for (const { name, path } of REGISTRATION_STYLES) { - test(`captures 500 from ${name} route with correct mechanism`, async ({ baseURL }) => { - const fullPath = `${PREFIX}${path}/500`; - - const errorPromise = waitForError(APP_NAME, event => { - return event.exception?.values?.[0]?.value === 'response 500' && !!event.request?.url?.includes(fullPath); - }); - - const response = await fetch(`${baseURL}${fullPath}`); - expect(response.status).toBe(500); - - const errorEvent = await errorPromise; - expect(errorEvent.exception?.values?.[0]?.value).toBe('response 500'); - expect(errorEvent.exception?.values?.[0]?.mechanism).toEqual( - expect.objectContaining({ - handled: false, - type: 'auto.http.hono.context_error', - }), - ); - }); - } - - test('captures 500 error with POST method', async ({ baseURL }) => { - const errorPromise = waitForError(APP_NAME, event => { - return ( - event.exception?.values?.[0]?.value === 'response 500' && - !!event.request?.url?.includes(`${PREFIX}/500`) && - event.request?.method === 'POST' - ); - }); - - const response = await fetch(`${baseURL}${PREFIX}/500`, { method: 'POST' }); - expect(response.status).toBe(500); - - const errorEvent = await errorPromise; - expect(errorEvent.exception?.values?.[0]?.value).toBe('response 500'); - expect(errorEvent.exception?.values?.[0]?.mechanism).toEqual( - expect.objectContaining({ - handled: false, - type: 'auto.http.hono.context_error', - }), - ); - }); -}); - -test.describe('4xx HTTPException capture', () => { - for (const code of [401, 402, 403]) { - test(`captures ${code} HTTPException`, async ({ baseURL }) => { - const fullPath = `${PREFIX}/${code}`; - - const errorPromise = waitForError(APP_NAME, event => { - return event.exception?.values?.[0]?.value === `response ${code}` && !!event.request?.url?.includes(fullPath); - }); - - const response = await fetch(`${baseURL}${fullPath}`); - expect(response.status).toBe(code); - - const errorEvent = await errorPromise; - expect(errorEvent.exception?.values?.[0]?.value).toBe(`response ${code}`); - expect(errorEvent.exception?.values?.[0]?.mechanism).toEqual( - expect.objectContaining({ - handled: false, - type: 'auto.http.hono.context_error', - }), - ); - }); - } -}); diff --git a/dev-packages/e2e-tests/test-applications/hono-4/tests/tracing.test.ts b/dev-packages/e2e-tests/test-applications/hono-4/tests/tracing.test.ts index 1c33943f38f8..4d9644312913 100644 --- a/dev-packages/e2e-tests/test-applications/hono-4/tests/tracing.test.ts +++ b/dev-packages/e2e-tests/test-applications/hono-4/tests/tracing.test.ts @@ -3,38 +3,40 @@ import { waitForTransaction } from '@sentry-internal/test-utils'; import { APP_NAME } from './constants'; test('sends a transaction for the index route', async ({ baseURL }) => { - const transactionWaiter = waitForTransaction(APP_NAME, event => { - return event.transaction === 'GET /'; + const transactionPromise = waitForTransaction(APP_NAME, event => { + return event.contexts?.trace?.op === 'http.server' && event.transaction === 'GET /'; }); const response = await fetch(`${baseURL}/`); expect(response.status).toBe(200); - const transaction = await transactionWaiter; + const transaction = await transactionPromise; + expect(transaction.transaction).toBe('GET /'); expect(transaction.contexts?.trace?.op).toBe('http.server'); }); test('sends a transaction for a parameterized route', async ({ baseURL }) => { - const transactionWaiter = waitForTransaction(APP_NAME, event => { - return event.transaction === 'GET /test-param/:paramId'; + const transactionPromise = waitForTransaction(APP_NAME, event => { + return event.contexts?.trace?.op === 'http.server' && !!event.transaction?.includes('/test-param/'); }); const response = await fetch(`${baseURL}/test-param/123`); expect(response.status).toBe(200); - const transaction = await transactionWaiter; - expect(transaction.contexts?.trace?.op).toBe('http.server'); + const transaction = await transactionPromise; expect(transaction.transaction).toBe('GET /test-param/:paramId'); + expect(transaction.contexts?.trace?.op).toBe('http.server'); }); test('sends a transaction for a route that throws', async ({ baseURL }) => { - const transactionWaiter = waitForTransaction(APP_NAME, event => { - return event.transaction === 'GET /error/:cause'; + const transactionPromise = waitForTransaction(APP_NAME, event => { + return event.contexts?.trace?.op === 'http.server' && !!event.transaction?.includes('/error/'); }); await fetch(`${baseURL}/error/test-cause`); - const transaction = await transactionWaiter; + const transaction = await transactionPromise; + expect(transaction.transaction).toBe('GET /error/:cause'); expect(transaction.contexts?.trace?.op).toBe('http.server'); expect(transaction.contexts?.trace?.status).toBe('internal_error'); }); diff --git a/packages/hono/src/shared/isExpectedError.ts b/packages/hono/src/shared/isExpectedError.ts new file mode 100644 index 000000000000..f3014be0fb5e --- /dev/null +++ b/packages/hono/src/shared/isExpectedError.ts @@ -0,0 +1,17 @@ +/** + * 3xx and 4xx errors are expected (redirects, auth failures, not found, bad + * request) and should not be captured as Sentry error events. + * + * Checks any error-like value that carries a numeric `status` property — this + * covers Hono's `HTTPException`, third-party middleware errors, and custom + * error subclasses. + */ +export function isExpectedError(error: unknown): boolean { + if (typeof error !== 'object' || error === null) { + return false; + } + + const status = (error as { status?: unknown }).status; + + return typeof status === 'number' && status >= 300 && status < 500; +} diff --git a/packages/hono/src/shared/middlewareHandlers.ts b/packages/hono/src/shared/middlewareHandlers.ts index a470733b47a8..41902d90f84f 100644 --- a/packages/hono/src/shared/middlewareHandlers.ts +++ b/packages/hono/src/shared/middlewareHandlers.ts @@ -11,6 +11,7 @@ import { import type { Context } from 'hono'; import { routePath } from 'hono/route'; import { hasFetchEvent } from '../utils/hono-context'; +import { isExpectedError } from './isExpectedError'; /** * Request handler for Hono framework @@ -42,7 +43,7 @@ export function responseHandler(context: Context): void { getIsolationScope().setTransactionName(`${context.req.method} ${routePath(context)}`); - if (context.error) { + if (context.error && !isExpectedError(context.error)) { getClient()?.captureException(context.error, { mechanism: { handled: false, type: 'auto.http.hono.context_error' }, }); diff --git a/packages/hono/src/shared/wrapMiddlewareSpan.ts b/packages/hono/src/shared/wrapMiddlewareSpan.ts index b93e5de0bded..13668e129c66 100644 --- a/packages/hono/src/shared/wrapMiddlewareSpan.ts +++ b/packages/hono/src/shared/wrapMiddlewareSpan.ts @@ -1,17 +1,17 @@ import { captureException, getActiveSpan, - getRootSpan, getOriginalFunction, + getRootSpan, markFunctionWrapped, SEMANTIC_ATTRIBUTE_SENTRY_OP, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, SPAN_STATUS_ERROR, - SPAN_STATUS_OK, startInactiveSpan, type WrappedFunction, } from '@sentry/core'; import { type MiddlewareHandler } from 'hono'; +import { isExpectedError } from './isExpectedError'; const MIDDLEWARE_ORIGIN = 'auto.middleware.hono'; @@ -41,14 +41,15 @@ export function wrapMiddlewareWithSpan(handler: MiddlewareHandler): MiddlewareHa }); try { - const result = await handler(context, next); - span.setStatus({ code: SPAN_STATUS_OK }); - return result; + return await handler(context, next); } catch (error) { - span.setStatus({ code: SPAN_STATUS_ERROR, message: 'internal_error' }); - captureException(error, { - mechanism: { handled: false, type: MIDDLEWARE_ORIGIN }, - }); + if (!isExpectedError(error)) { + span.setStatus({ code: SPAN_STATUS_ERROR, message: 'internal_error' }); + captureException(error, { + mechanism: { handled: false, type: MIDDLEWARE_ORIGIN }, + }); + } + throw error; } finally { span.end(); diff --git a/packages/hono/test/shared/isExpectedError.test.ts b/packages/hono/test/shared/isExpectedError.test.ts new file mode 100644 index 000000000000..235db79e1357 --- /dev/null +++ b/packages/hono/test/shared/isExpectedError.test.ts @@ -0,0 +1,78 @@ +import { HTTPException } from 'hono/http-exception'; +import { describe, expect, it } from 'vitest'; +import { isExpectedError } from '../../src/shared/isExpectedError'; + +describe('isExpectedError', () => { + describe('HTTPException', () => { + it('returns true for 4xx HTTPException', () => { + expect(isExpectedError(new HTTPException(400, { message: 'Bad Request' }))).toBe(true); + expect(isExpectedError(new HTTPException(401, { message: 'Unauthorized' }))).toBe(true); + expect(isExpectedError(new HTTPException(403, { message: 'Forbidden' }))).toBe(true); + expect(isExpectedError(new HTTPException(404, { message: 'Not Found' }))).toBe(true); + expect(isExpectedError(new HTTPException(422, { message: 'Unprocessable Entity' }))).toBe(true); + expect(isExpectedError(new HTTPException(499))).toBe(true); + }); + + it('returns false for 5xx HTTPException', () => { + expect(isExpectedError(new HTTPException(500, { message: 'Internal Server Error' }))).toBe(false); + expect(isExpectedError(new HTTPException(502, { message: 'Bad Gateway' }))).toBe(false); + expect(isExpectedError(new HTTPException(503, { message: 'Service Unavailable' }))).toBe(false); + }); + }); + + describe('custom error classes with status property', () => { + it('returns true for custom Error subclass with 4xx status', () => { + class AuthError extends Error { + status = 401; + } + expect(isExpectedError(new AuthError('unauthorized'))).toBe(true); + }); + + it('returns false for custom Error subclass with 5xx status', () => { + class DbError extends Error { + status = 500; + } + expect(isExpectedError(new DbError('connection lost'))).toBe(false); + }); + + it('returns true for plain object with 4xx status', () => { + expect(isExpectedError({ status: 404, message: 'Not Found' })).toBe(true); + expect(isExpectedError({ status: 400 })).toBe(true); + }); + + it('returns false for plain object with 5xx status', () => { + expect(isExpectedError({ status: 500, message: 'Internal Server Error' })).toBe(false); + }); + }); + + describe('non-expected errors', () => { + it('returns false for plain Error without status', () => { + expect(isExpectedError(new Error('something broke'))).toBe(false); + }); + + it('returns false for non-object values', () => { + expect(isExpectedError('string error')).toBe(false); + expect(isExpectedError(42)).toBe(false); + expect(isExpectedError(null)).toBe(false); + expect(isExpectedError(undefined)).toBe(false); + expect(isExpectedError(true)).toBe(false); + }); + + it('returns false when status is not a number', () => { + expect(isExpectedError({ status: '404' })).toBe(false); + expect(isExpectedError({ status: null })).toBe(false); + expect(isExpectedError({ status: undefined })).toBe(false); + }); + + it('returns true for 3xx status', () => { + expect(isExpectedError({ status: 301 })).toBe(true); + expect(isExpectedError({ status: 302 })).toBe(true); + expect(isExpectedError({ status: 399 })).toBe(true); + }); + + it('returns false for 2xx status', () => { + expect(isExpectedError({ status: 200 })).toBe(false); + expect(isExpectedError({ status: 299 })).toBe(false); + }); + }); +}); diff --git a/packages/hono/test/shared/middlewareHandlers.test.ts b/packages/hono/test/shared/middlewareHandlers.test.ts index 83099370320c..b8e4cdef1062 100644 --- a/packages/hono/test/shared/middlewareHandlers.test.ts +++ b/packages/hono/test/shared/middlewareHandlers.test.ts @@ -55,7 +55,7 @@ describe('responseHandler', () => { }); }); - it('captures error regardless of status code', () => { + it('captures 5xx HTTPException', () => { const mockCaptureException = vi.fn(); getClientMock.mockReturnValue({ captureException: mockCaptureException, @@ -101,7 +101,6 @@ describe('responseHandler', () => { // oxlint-disable-next-line typescript/no-explicit-any responseHandler(createMockContext(500, error) as any); - // captureException is called — it handles deduplication internally via checkOrSetAlreadyCaught expect(mockCaptureException).toHaveBeenCalledWith(error, { mechanism: { handled: false, type: 'auto.http.hono.context_error' }, });