Skip to content

Commit b6cb7b6

Browse files
authored
fix(hono): Capture transaction name on request for correct culprit (#20801)
Hono error events were showing a wrong culprit (`?(index)`) because the transaction name was only being set in the response handler. When an error was thrown during middleware execution, `captureException` ran before `responseHandler` had a chance to update the span name and isolation scope transaction name. This PR moves the span and transaction name update into `requestHandler` so the correct route name is set before any middleware or route handler executes. The `responseHandler` now only captures errors. Also switches from `routePath(context)` (last matched route so far) to `routePath(context, -1)` ([final matched route](https://hono.dev/docs/helpers/route#using-with-index-parameter)) which avoids wildcard patterns like `/error/*` leaking into the transaction name. Also adds another test for route parametrization when using middleware. Closes #20399
1 parent afe03a3 commit b6cb7b6

6 files changed

Lines changed: 48 additions & 13 deletions

File tree

dev-packages/e2e-tests/test-applications/hono-4/src/route-groups/test-middleware.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ middlewareRoutes.get('/named', c => c.json({ middleware: 'named' }));
77
middlewareRoutes.get('/anonymous', c => c.json({ middleware: 'anonymous' }));
88
middlewareRoutes.get('/multi', c => c.json({ middleware: 'multi' }));
99
middlewareRoutes.get('/error', c => c.text('should not reach'));
10+
middlewareRoutes.get('/param/:id', c => c.json({ paramId: c.req.param('id') }));
1011

1112
// Self-contained sub-app registering its own middleware via .use()
1213
const subAppWithMiddleware = new Hono();
@@ -18,6 +19,7 @@ subAppWithMiddleware.use('/anonymous/*', async (c, next) => {
1819
});
1920
subAppWithMiddleware.use('/multi/*', middlewareA, middlewareB);
2021
subAppWithMiddleware.use('/error/*', failingMiddleware);
22+
subAppWithMiddleware.use('/param/*', middlewareA);
2123

2224
// .all() handler (1 parameter) — should NOT be wrapped as middleware by patchRoute.
2325
subAppWithMiddleware.all('/all-handler', async function allCatchAll(c) {

dev-packages/e2e-tests/test-applications/hono-4/src/routes.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@ export function addRoutes(app: Hono<{ Bindings?: { E2E_TEST_DSN: string } }>): v
3434
});
3535
app.use('/test-middleware/multi/*', middlewareA, middlewareB);
3636
app.use('/test-middleware/error/*', failingMiddleware);
37+
app.use('/test-middleware/param/*', middlewareA);
3738
app.route('/test-middleware', middlewareRoutes);
3839

3940
// Sub-app middleware: registered on the sub-app, wrapped at mount time by route() patching

dev-packages/e2e-tests/test-applications/hono-4/tests/errors.test.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -147,6 +147,7 @@ test.describe('middleware errors', () => {
147147
expect(errorEvent.exception?.values?.[0]?.value).toBe('Service Unavailable from middleware');
148148
expect(errorEvent.exception?.values?.[0]?.mechanism?.type).toBe('auto.middleware.hono');
149149
expect(errorEvent.exception?.values?.[0]?.mechanism?.handled).toBe(false);
150+
expect(errorEvent.transaction).toBe('GET /test-errors/middleware-http-exception');
150151

151152
const transaction = await transactionPromise;
152153
const middlewareSpan = (transaction.spans || []).find(s => s.op === 'middleware.hono');
@@ -183,7 +184,7 @@ test.describe('middleware errors', () => {
183184
const transaction = await transactionPromise;
184185

185186
if (RUNTIME === 'cloudflare') {
186-
expect(transaction.transaction).toBe('GET /test-errors/middleware-http-exception-4xx/*');
187+
expect(transaction.transaction).toBe('GET /test-errors/middleware-http-exception-4xx');
187188

188189
const middlewareSpan = (transaction.spans || []).find(s => s.op === 'middleware.hono');
189190
expect(middlewareSpan?.status).not.toBe('internal_error');

dev-packages/e2e-tests/test-applications/hono-4/tests/middleware.test.ts

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -116,6 +116,9 @@ for (const { name, prefix } of SCENARIOS) {
116116
type: 'auto.middleware.hono',
117117
}),
118118
);
119+
120+
// The transaction name on the error event determines the culprit shown in Sentry.
121+
expect(errorEvent.transaction).toBe(`GET ${prefix}/error`);
119122
});
120123

121124
test('sets error status on middleware span when middleware throws', async ({ baseURL }) => {
@@ -126,7 +129,7 @@ for (const { name, prefix } of SCENARIOS) {
126129
await fetch(`${baseURL}${prefix}/error`);
127130

128131
const transaction = await transactionPromise;
129-
expect(transaction.transaction).toBe(`GET ${prefix}/error/*`);
132+
expect(transaction.transaction).toBe(`GET ${prefix}/error`);
130133

131134
const spans = transaction.spans || [];
132135

@@ -138,6 +141,25 @@ for (const { name, prefix } of SCENARIOS) {
138141
expect(failingSpan?.status).toBe('internal_error');
139142
});
140143

144+
test('uses parameterized route in transaction name', async ({ baseURL }) => {
145+
const transactionPromise = waitForTransaction(APP_NAME, event => {
146+
return event.contexts?.trace?.op === 'http.server' && !!event.transaction?.includes(`${prefix}/param/`);
147+
});
148+
149+
const response = await fetch(`${baseURL}${prefix}/param/42`);
150+
expect(response.status).toBe(200);
151+
152+
const transaction = await transactionPromise;
153+
expect(transaction.transaction).toBe(`GET ${prefix}/param/:id`);
154+
155+
const spans = transaction.spans || [];
156+
const middlewareSpan = spans.find(
157+
(span: { description?: string; op?: string }) =>
158+
span.op === 'middleware.hono' && span.description === 'middlewareA',
159+
);
160+
expect(middlewareSpan).toBeDefined();
161+
});
162+
141163
test('includes request data on error events from middleware', async ({ baseURL }) => {
142164
const errorPromise = waitForError(APP_NAME, event => {
143165
return event.exception?.values?.[0]?.value === 'Middleware error' && !!event.request?.url?.includes(prefix);

packages/hono/src/shared/middlewareHandlers.ts

Lines changed: 16 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import {
66
getRootSpan,
77
SEMANTIC_ATTRIBUTE_SENTRY_SOURCE,
88
updateSpanName,
9+
type Scope,
910
winterCGRequestToRequestData,
1011
} from '@sentry/core';
1112
import type { Context } from 'hono';
@@ -22,6 +23,8 @@ export function requestHandler(context: Context): void {
2223

2324
const isolationScope = defaultScope === currentIsolationScope ? defaultScope : currentIsolationScope;
2425

26+
updateSpanRouteName(isolationScope, context);
27+
2528
isolationScope.setSDKProcessingMetadata({
2629
normalizedRequest: winterCGRequestToRequestData(hasFetchEvent(context) ? context.event.request : context.req.raw),
2730
});
@@ -31,21 +34,25 @@ export function requestHandler(context: Context): void {
3134
* Response handler for Hono framework
3235
*/
3336
export function responseHandler(context: Context): void {
37+
if (context.error && !isExpectedError(context.error)) {
38+
getClient()?.captureException(context.error, {
39+
mechanism: { handled: false, type: 'auto.http.hono.context_error' },
40+
});
41+
}
42+
}
43+
44+
function updateSpanRouteName(isolationScope: Scope, context: Context): void {
3445
const activeSpan = getActiveSpan();
46+
const lastMatchedRoute = routePath(context, -1);
47+
3548
if (activeSpan) {
36-
activeSpan.updateName(`${context.req.method} ${routePath(context)}`);
49+
activeSpan.updateName(`${context.req.method} ${lastMatchedRoute}`);
3750
activeSpan.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, 'route');
3851

3952
const rootSpan = getRootSpan(activeSpan);
40-
updateSpanName(rootSpan, `${context.req.method} ${routePath(context)}`);
53+
updateSpanName(rootSpan, `${context.req.method} ${lastMatchedRoute}`);
4154
rootSpan.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, 'route');
4255
}
4356

44-
getIsolationScope().setTransactionName(`${context.req.method} ${routePath(context)}`);
45-
46-
if (context.error && !isExpectedError(context.error)) {
47-
getClient()?.captureException(context.error, {
48-
mechanism: { handled: false, type: 'auto.http.hono.context_error' },
49-
});
50-
}
57+
isolationScope.setTransactionName(`${context.req.method} ${lastMatchedRoute}`);
5158
}

packages/hono/test/shared/middlewareHandlers.test.ts

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import * as SentryCore from '@sentry/core';
22
import { beforeEach, describe, expect, it, vi } from 'vitest';
3-
import { responseHandler } from '../../src/shared/middlewareHandlers';
3+
import { requestHandler, responseHandler } from '../../src/shared/middlewareHandlers';
44

55
vi.mock('hono/route', () => ({
66
routePath: () => '/test',
@@ -11,6 +11,7 @@ vi.mock('../../src/utils/hono-context', () => ({
1111
}));
1212

1313
const mockSetTransactionName = vi.fn();
14+
const mockSetSDKProcessingMetadata = vi.fn();
1415

1516
vi.mock('@sentry/core', async () => {
1617
const actual = await vi.importActual('@sentry/core');
@@ -19,6 +20,7 @@ vi.mock('@sentry/core', async () => {
1920
getActiveSpan: vi.fn(() => null),
2021
getIsolationScope: vi.fn(() => ({
2122
setTransactionName: mockSetTransactionName,
23+
setSDKProcessingMetadata: mockSetSDKProcessingMetadata,
2224
})),
2325
getClient: vi.fn(() => undefined),
2426
};
@@ -110,7 +112,7 @@ describe('responseHandler', () => {
110112
describe('transaction name', () => {
111113
it('sets transaction name on isolation scope', () => {
112114
// oxlint-disable-next-line typescript/no-explicit-any
113-
responseHandler(createMockContext(200) as any);
115+
requestHandler(createMockContext(200) as any);
114116

115117
expect(mockSetTransactionName).toHaveBeenCalledWith('GET /test');
116118
});

0 commit comments

Comments
 (0)