Skip to content

Conversation

Karibash
Copy link
Contributor

@Karibash Karibash commented Aug 18, 2025

Summary

This PR enhances the Hono integration by adding comprehensive handler instrumentation, error handling capabilities, and thorough test coverage. The changes build upon the basic Hono integration to provide a complete tracing and error monitoring solution.

New Features

  • Handler Instrumentation: Added instrumentation for Hono handlers and middleware, providing detailed tracing capabilities
  • Error Handler: Implemented setupHonoErrorHandler() function to capture and report errors to Sentry with configurable error filtering
  • Public API: Added Hono integration to the main package exports, making it available as @sentry/node
  • Tracing Module: Included Hono integration in the tracing integrations index

Bug Fixes

  • CJS Compatibility: Fixed an issue where applying patches failed in CommonJS environments
  • Type Corrections: Fixed incorrect MiddlewareHandler type definition to ensure proper TypeScript support

Implementation Details

  • Instrumentation: Created HonoInstrumentation class that wraps Hono middleware handlers via class extension instead of function replacement for better compatibility
  • Type Definitions: Added comprehensive TypeScript type definitions vendored from Hono's official types
  • Constants: Defined Hono-specific attribute names for OpenTelemetry integration
  • CJS Compatibility: Fixed patching issues in CommonJS environments

Testing

  • Integration Tests: Added comprehensive test suite covering:
    • ESM and CJS compatibility
    • Multiple HTTP methods (GET, POST, PUT, DELETE, PATCH)
    • Various route patterns (sync/async, different paths)
    • Middleware and handler instrumentation verification
    • Error handling scenarios
    • Span attribute validation

Related Issue

close #15260

cursor[bot]

This comment was marked as outdated.

@Karibash Karibash force-pushed the feature/hono-instrumentation-handler branch from 32e3a23 to 505c480 Compare August 18, 2025 02:31
cursor[bot]

This comment was marked as outdated.

@s1gr1d s1gr1d assigned s1gr1d and unassigned s1gr1d Aug 25, 2025
@s1gr1d s1gr1d requested review from s1gr1d and Lms24 August 25, 2025 11:06
@Karibash Karibash force-pushed the feature/hono-instrumentation-handler branch from 505c480 to a7ef204 Compare August 25, 2025 11:22
@Karibash
Copy link
Contributor Author

Karibash commented Sep 2, 2025

@s1gr1d @Lms24
Hi, just wanted to kindly follow up on this PR.
Is there anything I can do to help move it forward, or any feedback you’d like me to address?

Copy link
Member

@s1gr1d s1gr1d left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your contribution 🙌
I just have some suggestions and follow-up questions.

And sorry for the long wait, we had some busy weeks and I try to look at your PR sooner next time :)

}

const path = c.req.path;
const spanName = `${type.replace('_', ' ')} - ${path}`;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would be an example span name here? I think there are no tests that check this name so I couldn't find an example.

Would be good to also have tests for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked other test codes, but I couldn’t find a way to test the span name.
I also looked into transaction.spans, but it doesn’t seem to include the span name.

{
  "span_id": "c6b2cd926a07da13",
  "trace_id": "8d75c4926828d1b7b98dc2b9356c3fc9",
  "data": {
    "sentry.origin": "auto.http.otel.hono",
    "sentry.op": "request_handler.hono",
    "hono.type": "request_handler",
    "hono.name": "/async"
  },
  "description": "/async",
  "parent_span_id": "b3270618aac86a93",
  "start_timestamp": 1757852841.829,
  "timestamp": 1757852841.8291407,
  "status": "ok",
  "op": "request_handler.hono",
  "origin": "auto.http.otel.hono"
}

Do you know how we can test the span name in this case?

if (
result &&
typeof result === 'object' &&
typeof Object.getOwnPropertyDescriptor(result, 'then')?.value === 'function'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use isThenable here from @sentry/core

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I’ve fixed this in the following commit: 8dc3939

* Safely executes a function and handles errors.
*/
// eslint-disable-next-line @typescript-eslint/no-explicit-any
private _safeExecute(execute: () => any, onSuccess: () => void, onFailure: (error: unknown) => void): () => any {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if it would be possible to type this instead of relying on any here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the return value of handler.apply(this, [c, next]) is any, I think it would be difficult to type this explicitly.

path,
...handlers.map((handler, index) =>
instrumentation._wrapHandler(
index + 1 === handlers.length ? HonoTypes.REQUEST_HANDLER : HonoTypes.MIDDLEWARE,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we be sure that the handlers are always those two in this order?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I overlooked the case where the middleware is registered on its own.
That has been fixed in the following commit: 7172211

@s1gr1d
Copy link
Member

s1gr1d commented Sep 9, 2025

@Karibash just a general heads-up: It's awesome that you are working on this instrumentation right now. Keep in mind that we are planning to start working on a dedicated hono SDK in about a month. So your integration can be used in the meantime but we might need to deprecate it once the new SDK is stable :)

@Karibash
Copy link
Contributor Author

Karibash commented Sep 9, 2025

@s1gr1d Thanks for the heads-up!
Just to clarify — will the upcoming SDK take a different approach than using @opentelemetry/instrumentation?
I’m also curious to hear a bit more about what approach you’re planning to take for the new implementation.

@s1gr1d
Copy link
Member

s1gr1d commented Sep 9, 2025

It will use OTel under the hood but we'll focus on making it work on Cloudflare as a first priority.

@Karibash
Copy link
Contributor Author

Karibash commented Sep 9, 2025

Got it — so in that sense, the approach in this PR won’t differ too much from what you’re planning for the new SDK (aside from the implementation details that will naturally evolve), right?

@s1gr1d
Copy link
Member

s1gr1d commented Sep 11, 2025

Probably not, but it's not planned out in detail yet

@Karibash Karibash force-pushed the feature/hono-instrumentation-handler branch from a7ef204 to bafc2be Compare September 13, 2025 09:22
cursor[bot]

This comment was marked as outdated.

@s1gr1d
Copy link
Member

s1gr1d commented Sep 15, 2025

Can you check the missing exports?
https://github.com/getsentry/sentry-javascript/actions/runs/17712340530/job/50332837288?pr=17428#step:16:41

@Karibash
Copy link
Contributor Author

Can you check the missing exports? https://github.com/getsentry/sentry-javascript/actions/runs/17712340530/job/50332837288?pr=17428#step:16:41

I’ve fixed this in the following commit: 20f5ac8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement Hono SDK
2 participants