-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(node): Add an instrumentation interface for Hono #17366
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I extracted and defined only the minimal types necessary for instrumentation from Hono source code.
function Hono(this: HonoInstance, ...args: any): HonoInstance { | ||
const app: HonoInstance = moduleExports.Hono.apply(this, args); | ||
|
||
instrumentation._wrap(app, 'get', instrumentation._patchHandler()); | ||
instrumentation._wrap(app, 'post', instrumentation._patchHandler()); | ||
instrumentation._wrap(app, 'put', instrumentation._patchHandler()); | ||
instrumentation._wrap(app, 'delete', instrumentation._patchHandler()); | ||
instrumentation._wrap(app, 'options', instrumentation._patchHandler()); | ||
instrumentation._wrap(app, 'patch', instrumentation._patchHandler()); | ||
instrumentation._wrap(app, 'all', instrumentation._patchHandler()); | ||
instrumentation._wrap(app, 'on', instrumentation._patchOnHandler()); | ||
instrumentation._wrap(app, 'use', instrumentation._patchMiddlewareHandler()); | ||
|
||
return app; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hono dynamically defines these methods within its constructor, which prevented overriding the prototype, so I decided to patch the constructor instead.
/** | ||
* Patches the route handler to instrument it. | ||
*/ | ||
private _patchHandler(): (original: HandlerInterface) => HandlerInterface { | ||
return function(original: HandlerInterface) { | ||
// eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
return function wrappedHandler(this: HonoInstance, ...args: any) { | ||
// TODO: Add OpenTelemetry tracing logic here | ||
return original.apply(this, args); | ||
}; | ||
}; | ||
} | ||
|
||
/** | ||
* Patches the 'on' handler to instrument it. | ||
*/ | ||
private _patchOnHandler(): (original: OnHandlerInterface) => OnHandlerInterface { | ||
return function(original: OnHandlerInterface) { | ||
// eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
return function wrappedHandler(this: HonoInstance, ...args: any) { | ||
// TODO: Add OpenTelemetry tracing logic here | ||
return original.apply(this, args); | ||
}; | ||
}; | ||
} | ||
|
||
/** | ||
* Patches the middleware handler to instrument it. | ||
*/ | ||
private _patchMiddlewareHandler(): (original: MiddlewareHandlerInterface) => MiddlewareHandlerInterface { | ||
return function(original: MiddlewareHandlerInterface) { | ||
// eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
return function wrappedHandler(this: HonoInstance, ...args: any) { | ||
// TODO: Add OpenTelemetry tracing logic here | ||
return original.apply(this, args); | ||
}; | ||
}; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation of these patches will be submitted later across several PRs.
75251f3
to
0ddfe17
Compare
… instead of function replacement
b40e85a
to
cb1a52e
Compare
6a3abac
to
21fba19
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding this PR!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't use enums
in our repository (also see related PR). However, I think this file can be deleted, as those enums are not used anywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have removed the enums as you pointed out.
These values will be needed when specifying span attributes, so at that time I plan to define them as constants with primitive values.
2a6cd6e
@@ -0,0 +1,88 @@ | |||
import { InstrumentationBase,InstrumentationNodeModuleDefinition } from '@opentelemetry/instrumentation'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Run yarn fix
to fix linting and the code formatting with prettier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’ve fixed it. I’ll be careful not to forget next time.
98a2c37
/** | ||
* Patches the module exports to instrument Hono. | ||
*/ | ||
private _patch(moduleExports: { Hono: Hono }): { Hono: Hono } { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lowercase the hono variable so it's easier to differentiate between the type and the variable.
private _patch(moduleExports: { Hono: Hono }): { Hono: Hono } { | |
private _patch(moduleExports: { hono: Hono }): { hono: Hono } { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we are patching the class exported by hono, wouldn’t it need to be uppercase here, same as the exported class name?
https://github.com/honojs/hono/blob/main/src/hono.ts#L16
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah this makes sense, yes :)
Thank you for this contribution 🙌 |
98a2c37
to
e694aae
Compare
instrumentation._wrap(this, 'use', instrumentation._patchMiddlewareHandler()); | ||
} | ||
}; | ||
return moduleExports; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Hono Instrumentation Fails on Default Imports
The Hono instrumentation only patches the named Hono
export, ignoring the default
export. This prevents instrumentation for applications importing Hono
as a default. Additionally, the Hono
class is replaced in-place without using InstrumentationBase._wrap
, and instance methods are wrapped within the subclass constructor. This design prevents proper unpatching, leading to persistent instrumentation where new Hono
instances remain wrapped even after the instrumentation is disabled, breaking enable/disable semantics.
This PR introduces the scaffolding for Sentry’s tracing integration with Hono by adding interface-only implementations and wiring needed to verify the overall approach before filling in the tracing logic.
Summary
Intent & scope
The goal is to confirm the wrapping points, attribute schema, and initialization flow before we add any span creation, context propagation, or attribute setting.
That follow-up will include span start/finish, setting hono.type/hono.name, request path/method extraction, and trace context propagation.
Rationale
There is an existing Hono OTel package (@hono/otel), but it currently lacks several features we need for a robust Sentry integration—especially middleware instrumentation and Sentry-specific integration points (e.g., seamless correlation with Sentry transactions/spans and future Sentry error handler wiring).
Given these gaps, we’re proceeding with an in-repo implementation tailored for Sentry’s needs.
Related Issue
#15260