feat(hono): Add basic instrumentation for Node runtime#19817
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Wrong
faasmechanism type used for Node runtime- I parameterized the shared Hono response handler mechanism type and made the Node middleware pass
auto.middleware.hono.error_handler, with a regression test asserting the Node-specific mechanism is used.
- I parameterized the shared Hono response handler mechanism type and made the Node middleware pass
Or push these changes by commenting:
@cursor push 587213dbc2
Preview (587213dbc2)
diff --git a/packages/hono/src/node/middleware.ts b/packages/hono/src/node/middleware.ts
--- a/packages/hono/src/node/middleware.ts
+++ b/packages/hono/src/node/middleware.ts
@@ -27,6 +27,6 @@
await next(); // Handler runs in between Request above ⤴ and Response below ⤵
- responseHandler(context);
+ responseHandler(context, 'auto.middleware.hono.error_handler');
};
};
diff --git a/packages/hono/src/shared/middlewareHandlers.ts b/packages/hono/src/shared/middlewareHandlers.ts
--- a/packages/hono/src/shared/middlewareHandlers.ts
+++ b/packages/hono/src/shared/middlewareHandlers.ts
@@ -29,7 +29,7 @@
/**
* Response handler for Hono framework
*/
-export function responseHandler(context: Context): void {
+export function responseHandler(context: Context, mechanismType = 'auto.faas.hono.error_handler'): void {
const activeSpan = getActiveSpan();
if (activeSpan) {
activeSpan.updateName(`${context.req.method} ${routePath(context)}`);
@@ -44,7 +44,7 @@
if (context.error) {
getClient()?.captureException(context.error, {
- mechanism: { handled: false, type: 'auto.faas.hono.error_handler' },
+ mechanism: { handled: false, type: mechanismType },
});
}
}
diff --git a/packages/hono/test/vercel/middleware.test.ts b/packages/hono/test/vercel/middleware.test.ts
--- a/packages/hono/test/vercel/middleware.test.ts
+++ b/packages/hono/test/vercel/middleware.test.ts
@@ -2,12 +2,18 @@
import { SDK_VERSION } from '@sentry/core';
import { Hono } from 'hono';
import { beforeEach, describe, expect, it, type Mock, vi } from 'vitest';
+import * as middlewareHandlers from '../../src/shared/middlewareHandlers';
import { sentry } from '../../src/node/middleware';
vi.mock('@sentry/node', () => ({
init: vi.fn(),
}));
+vi.mock('../../src/shared/middlewareHandlers', () => ({
+ requestHandler: vi.fn(),
+ responseHandler: vi.fn(),
+}));
+
// eslint-disable-next-line @typescript-eslint/consistent-type-imports
const { init: initNodeMock } = await vi.importMock<typeof import('@sentry/node')>('@sentry/node');
@@ -22,6 +28,8 @@
});
const applySdkMetadataMock = SentryCore.applySdkMetadata as Mock;
+const requestHandlerMock = middlewareHandlers.requestHandler as Mock;
+const responseHandlerMock = middlewareHandlers.responseHandler as Mock;
describe('Hono Vercel Middleware', () => {
beforeEach(() => {
@@ -114,6 +122,18 @@
expect(middleware.constructor.name).toBe('AsyncFunction');
});
+ it('uses node mechanism type for response errors', async () => {
+ const app = new Hono();
+ const middleware = sentry(app, {});
+ const context = {} as Parameters<typeof middleware>[0];
+ const next = vi.fn(async () => undefined);
+
+ await middleware(context, next);
+
+ expect(requestHandlerMock).toHaveBeenCalledWith(context);
+ expect(responseHandlerMock).toHaveBeenCalledWith(context, 'auto.middleware.hono.error_handler');
+ });
+
it('includes hono SDK metadata', () => {
const app = new Hono();
const options = {This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.
| await next(); // Handler runs in between Request above ⤴ and Response below ⤵ | ||
|
|
||
| responseHandler(context); | ||
| }; |
There was a problem hiding this comment.
Wrong faas mechanism type used for Node runtime
Medium Severity
The Node middleware calls shared responseHandler, which uses mechanism type auto.faas.hono.error_handler for captureException. The faas (Function as a Service) prefix is specific to Cloudflare/serverless environments in this codebase. Every other Node framework uses auto.middleware.*, auto.function.*, or auto.http.* (e.g., Express uses auto.middleware.express, Fastify uses auto.function.fastify). By reusing the shared handler for Node, the mechanism type misidentifies the runtime context. Additionally, since the sentry middleware gets wrapped by wrapMiddlewareWithSpan (due to patchAppUse patching app.use before registration), the wrapping span has origin auto.middleware.hono, which doesn't align with the auto.faas.hono.error_handler mechanism type.
Additional Locations (1)
Triggered by project rule: PR Review Guidelines for Cursor Bot
size-limit report 📦
|
| @@ -0,0 +1,3 @@ | |||
| export { sentry } from './node/middleware'; | |||
|
|
|||
| export * from '@sentry/node'; | |||
There was a problem hiding this comment.
l/q: Do you think it would make sense to re-export init and add applySdkMetadata(opts, 'hono', ['hono', 'node']); here on top?
There was a problem hiding this comment.
We currently don't document this way of setting it up (calling init manually) but that's a good idea!
packages/hono/src/node/middleware.ts
Outdated
|
|
||
| isDebug && debug.log('Initialized Sentry Hono middleware (Node)'); | ||
|
|
||
| applySdkMetadata(options, 'hono'); |
There was a problem hiding this comment.
l: Shouldn't this be the following?
| applySdkMetadata(options, 'hono'); | |
| applySdkMetadata(options, 'hono', ['hono', 'node']); |
# Conflicts: # packages/hono/README.md
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| patchAppUse(app); | ||
|
|
||
| return async (context, next) => { | ||
| requestHandler(context); |
There was a problem hiding this comment.
Bug: The hasFetchEvent() check incorrectly returns true in Node.js because accessing the undefined c.event property doesn't throw, leading to a TypeError when context.event.request is accessed.
Severity: CRITICAL
Suggested Fix
The hasFetchEvent utility should be updated to correctly identify the Node.js environment. Instead of relying on a try-catch block, it should perform a truthiness check on c.event (e.g., return !!c.event;). This will ensure it returns false for Node.js, causing the code to correctly fall back to using context.req.raw and avoid the TypeError.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: packages/hono/src/node/middleware.ts#L22
Potential issue: The Node.js Hono middleware will crash on every request due to a
`TypeError`. The shared `requestHandler` calls `hasFetchEvent()` to determine the
runtime environment. This function uses a `try-catch` block around `c.event` access,
assuming an error is thrown if `c.event` is unavailable. However, in the Node.js
environment, accessing `c.event` returns `undefined` without throwing an error. This
causes `hasFetchEvent()` to incorrectly return `true`, leading the subsequent code to
attempt accessing `context.event.request`, which fails because `context.event` is
`undefined`.
Did we get this right? 👍 / 👎 to inform future reviews.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
There are 3 total unresolved issues (including 1 from previous review).
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
|
|
||
| const rootSpan = getRootSpan(activeSpan); | ||
| updateSpanName(rootSpan, `${context.req.method} ${routePath(context)}`); | ||
| rootSpan.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, 'route'); |
There was a problem hiding this comment.
Incorrect faas mechanism type for Node runtime
Medium Severity
The captureException call uses mechanism type auto.faas.hono.error_handler, where faas (Function as a Service) is specific to serverless environments like Cloudflare Workers. Now that this shared code is used for the Node runtime too, the mechanism type is semantically incorrect for Node HTTP server contexts. The existing @sentry/node Hono error handler uses auto.middleware.hono for its mechanism type. The mechanism type needs to vary by runtime or use a runtime-agnostic value.
Triggered by project rule: PR Review Guidelines for Cursor Bot
There was a problem hiding this comment.
Will be done in another PR as this affects not just Node
Mentioned here: #19817 (comment) Closes #19832 (added automatically)
2 participants
Adds basic instrumentation for Hono in a Node runtime environment.
Reference (closed PR): #19761
Closes #19818 (added automatically)