-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
feat(hono): add orchestrion-based auto-instrumentation #24497
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
Changes from all commits
75b563c
277348c
8529de9
8941894
1465586
14c7d0d
4862fd6
d7880fd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,6 +13,7 @@ interface SentryTestVariant { | |
| 'build-command': string; | ||
| 'assert-command'?: string; | ||
| label?: string; | ||
| skip?: boolean; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| } | ||
|
|
||
| interface PackageJson { | ||
|
|
@@ -82,7 +83,13 @@ async function getVariantBuildCommand( | |
| packageJsonPath: string, | ||
| variantLabel: string, | ||
| testAppPath: string, | ||
| ): Promise<{ buildCommand: string; assertCommand: string; testLabel: string; matchedVariantLabel?: string }> { | ||
| ): Promise<{ | ||
| buildCommand: string; | ||
| assertCommand: string; | ||
| testLabel: string; | ||
| matchedVariantLabel?: string; | ||
| skip?: boolean; | ||
| }> { | ||
| try { | ||
| const packageJsonContent = await readFile(packageJsonPath, 'utf-8'); | ||
| const packageJson: PackageJson = JSON.parse(packageJsonContent); | ||
|
|
@@ -100,6 +107,7 @@ async function getVariantBuildCommand( | |
| assertCommand: matchingVariant['assert-command'] || 'pnpm test:assert', | ||
| testLabel: matchingVariant.label || testAppPath, | ||
| matchedVariantLabel: matchingVariant.label, | ||
| skip: matchingVariant.skip, | ||
| }; | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| # prod | ||
| dist/ | ||
|
|
||
| # dev | ||
| .yarn/ | ||
| !.yarn/releases | ||
| .vscode/* | ||
| !.vscode/launch.json | ||
| !.vscode/*.code-snippets | ||
| .idea/workspace.xml | ||
| .idea/usage.statistics.xml | ||
| .idea/shelf | ||
|
|
||
| # deps | ||
| node_modules/ | ||
| .wrangler | ||
|
|
||
| # env | ||
| .env | ||
| .env.production | ||
| .dev.vars | ||
|
|
||
| # logs | ||
| logs/ | ||
| *.log | ||
| npm-debug.log* | ||
| yarn-debug.log* | ||
| yarn-error.log* | ||
| pnpm-debug.log* | ||
| lerna-debug.log* | ||
|
|
||
| # test | ||
| test-results | ||
|
|
||
| # misc | ||
| .DS_Store |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,28 @@ | ||||||
| // Builds `src/entry.bun.ts` with the orchestrion `bun build` plugin, emitting `dist/entry.bun.js` | ||||||
| // for the server to run. The plugin injects the `orchestrion:hono:honoConstructor` diagnostics | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| // channel into the bundled `hono`, which the `honoIntegration` default subscribes to. | ||||||
|
|
||||||
| // @ts-ignore -- subpath export resolved by Bun at runtime; the package tsconfig's node module | ||||||
| // resolution can't see `exports` subpaths. | ||||||
| import { sentryBunPlugin } from '@sentry/bun/plugin'; | ||||||
| import { join } from 'path'; | ||||||
|
|
||||||
| void (async () => { | ||||||
| const result = await Bun.build({ | ||||||
| entrypoints: [join(__dirname, 'src/entry.bun.ts')], | ||||||
| target: 'bun', | ||||||
| outdir: join(__dirname, 'dist'), | ||||||
| // `@sentry/bun` (and its deps) stay external, so we don't bundle the whole SDK stack. | ||||||
| external: ['@sentry/bun'], | ||||||
| plugins: [sentryBunPlugin()], | ||||||
| }); | ||||||
|
|
||||||
| if (!result.success) { | ||||||
| // eslint-disable-next-line no-console | ||||||
| console.error('BUILD_FAILED', result.logs); | ||||||
| process.exit(1); | ||||||
| } | ||||||
|
|
||||||
| // eslint-disable-next-line no-console | ||||||
| console.log('BUILD_OK'); | ||||||
| })(); | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| { | ||
| "imports": { | ||
| "@sentry/deno": "npm:@sentry/deno", | ||
| "@sentry/core": "npm:@sentry/core", | ||
| "@opentelemetry/api": "npm:@opentelemetry/api@^1.9.0", | ||
| "hono": "npm:hono@^4.13.1" | ||
| }, | ||
| "nodeModulesDir": "manual" | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,57 @@ | ||
| { | ||
| "name": "hono-4", | ||
| "type": "module", | ||
| "version": "0.0.0", | ||
| "private": true, | ||
| "scripts": { | ||
| "dev:node": "node --import tsx/esm --import ./src/instrument.node.ts src/entry.node.ts", | ||
| "dev:bun": "bun run dist/entry.bun.js", | ||
| "dev:deno": "deno run --allow-net --allow-env --allow-read --preload=npm:@sentry/deno/import src/entry.deno.ts", | ||
|
cursor[bot] marked this conversation as resolved.
|
||
| "dev:cloudflare": "wrangler dev --config ./dist/hono_4/wrangler.json --var \"E2E_TEST_DSN:$E2E_TEST_DSN\" --log-level=$(test $CI && echo 'none' || echo 'log')", | ||
| "test:build": "pnpm install", | ||
| "test": "TEST_ENV=production playwright test", | ||
| "test:assert": "pnpm test:assert:node", | ||
| "test:assert:node": "RUNTIME=node pnpm test", | ||
| "test:assert:bun": "bun run build-bun.ts && RUNTIME=bun pnpm test", | ||
| "test:assert:deno": "RUNTIME=deno pnpm test", | ||
| "test:assert:cloudflare": "vite build && RUNTIME=cloudflare pnpm test" | ||
| }, | ||
| "dependencies": { | ||
| "@sentry/bun": "latest || *", | ||
| "@sentry/cloudflare": "latest || *", | ||
| "@sentry/deno": "latest || *", | ||
| "@sentry/node": "latest || *", | ||
| "@hono/node-server": "^2.0.5", | ||
| "hono": "^4.13.1" | ||
| }, | ||
| "devDependencies": { | ||
| "@playwright/test": "~1.63.0", | ||
| "@cloudflare/vite-plugin": "^1.47.0", | ||
| "@cloudflare/workers-types": "^4.20240725.0", | ||
| "@sentry-internal/test-utils": "link:../../../test-utils", | ||
| "tsx": "4.21.0", | ||
| "typescript": "^5.5.2", | ||
| "vite": "^8.1.5", | ||
| "wrangler": "^4.114.0" | ||
| }, | ||
| "volta": { | ||
| "node": "24.15.0", | ||
| "extends": "../../package.json" | ||
| }, | ||
| "sentryTest": { | ||
| "variants": [ | ||
| { | ||
| "assert-command": "pnpm test:assert:bun", | ||
| "label": "hono-4 (bun)" | ||
| }, | ||
| { | ||
| "assert-command": "pnpm test:assert:deno", | ||
| "label": "hono-4 (deno)" | ||
| }, | ||
| { | ||
| "assert-command": "pnpm test:assert:cloudflare", | ||
| "label": "hono-4 (cloudflare)" | ||
| } | ||
| ] | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| import { getPlaywrightConfig } from '@sentry-internal/test-utils'; | ||
| import { RUNTIME, type Runtime } from './tests/constants'; | ||
|
|
||
| const testEnv = process.env.TEST_ENV; | ||
|
|
||
| if (!testEnv) { | ||
| throw new Error('No test env defined'); | ||
| } | ||
|
|
||
| const APP_PORT = 38787; | ||
|
|
||
| const startCommands: Record<Runtime, string> = { | ||
| cloudflare: `pnpm dev:cloudflare --port ${APP_PORT}`, | ||
| node: `pnpm dev:node`, | ||
| bun: `pnpm dev:bun`, | ||
| deno: `pnpm dev:deno`, | ||
| }; | ||
|
|
||
| const config = getPlaywrightConfig( | ||
| { | ||
| startCommand: startCommands[RUNTIME], | ||
| port: APP_PORT, | ||
| }, | ||
| { | ||
| workers: '100%', | ||
| retries: 0, | ||
| }, | ||
| ); | ||
|
|
||
| export default config; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| // Import the Sentry init first so `honoIntegration` is set up (and subscribed to Hono's per-request | ||
| // Context channel) before any request is handled. | ||
| import './instrument.bun'; | ||
| import { Hono } from 'hono'; | ||
| import { addRoutes } from './routes'; | ||
|
|
||
| const app = new Hono(); | ||
|
|
||
| addRoutes(app); | ||
|
|
||
| const port = Number(process.env.PORT || 38787); | ||
|
|
||
| export default { | ||
| port, | ||
| fetch: app.fetch, | ||
| }; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| import { Hono } from 'hono'; | ||
| import { addRoutes } from './routes'; | ||
|
|
||
| const app = new Hono<{ Bindings: { E2E_TEST_DSN: string } }>(); | ||
|
|
||
| addRoutes(app); | ||
|
|
||
| export default app; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| // Import the Sentry init first so `honoIntegration` is set up (and subscribed to Hono's per-request | ||
| // Context channel) before any request is handled. | ||
| import './instrument.deno'; | ||
| import { Hono } from 'hono'; | ||
| import { addRoutes } from './routes'; | ||
|
|
||
| const app = new Hono(); | ||
|
|
||
| addRoutes(app); | ||
|
|
||
| const port = Number(Deno.env.get('PORT') || 38787); | ||
|
|
||
| Deno.serve({ port }, app.fetch); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| import { serve } from '@hono/node-server'; | ||
| import { Hono } from 'hono'; | ||
| import { addRoutes } from './routes'; | ||
|
|
||
| const app = new Hono(); | ||
|
|
||
| addRoutes(app); | ||
|
|
||
| const port = Number(process.env.PORT || 38787); | ||
|
|
||
| serve({ fetch: app.fetch, port }, () => { | ||
| // eslint-disable-next-line no-console | ||
| console.log(`Hono (Node) listening on port ${port}`); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| import * as Sentry from '@sentry/bun'; | ||
|
|
||
| Sentry.init({ | ||
| dsn: process.env.E2E_TEST_DSN, | ||
| environment: 'qa', | ||
| tracesSampleRate: 1.0, | ||
| tunnel: 'http://localhost:3031/', | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| import * as Sentry from '@sentry/deno'; | ||
|
|
||
| Sentry.init({ | ||
| dsn: Deno.env.get('E2E_TEST_DSN'), | ||
| environment: 'qa', | ||
| dataCollection: {}, | ||
| tracesSampleRate: 1.0, | ||
| tunnel: 'http://localhost:3031/', | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| import * as Sentry from '@sentry/node'; | ||
|
|
||
| Sentry.init({ | ||
| dsn: process.env.E2E_TEST_DSN, | ||
| environment: 'qa', | ||
| tracesSampleRate: 1.0, | ||
| tunnel: 'http://localhost:3031/', | ||
| }); |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,7 @@ | ||||||||||||||||||||||||||||||||
| // Cloudflare-SDK init | ||||||||||||||||||||||||||||||||
| export default (env: { E2E_TEST_DSN: string }) => ({ | ||||||||||||||||||||||||||||||||
| dsn: env.E2E_TEST_DSN, | ||||||||||||||||||||||||||||||||
| environment: 'qa', | ||||||||||||||||||||||||||||||||
| tracesSampleRate: 1.0, | ||||||||||||||||||||||||||||||||
| tunnel: 'http://localhost:3031/', | ||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||
|
Comment on lines
+1
to
+7
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have to investigate why this is not suggested by Clankers (assuming this was written by them)
Suggested change
|
||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| import type { MiddlewareHandler } from 'hono'; | ||
|
|
||
| // Defined as anonymous function expressions so the function name is inferred from the `const` | ||
| // binding. A named expression (`const middlewareA = async function middlewareA() {}`) collides with | ||
| // the binding when bundled, and Bun renames the inner function (→ `middlewareA2`), which would then | ||
| // surface as the middleware span name. The inferred name stays stable across all runtimes. | ||
| export const middlewareA: MiddlewareHandler = async function (c, next) { | ||
| // Add some delay | ||
| await new Promise(resolve => setTimeout(resolve, 50)); | ||
| await next(); | ||
| }; | ||
|
|
||
| export const middlewareB: MiddlewareHandler = async function (_c, next) { | ||
| // Add some delay | ||
| await new Promise(resolve => setTimeout(resolve, 60)); | ||
| await next(); | ||
| }; | ||
|
|
||
| let failingMiddlewareCount = 0; | ||
| export const failingMiddleware: MiddlewareHandler = async function (_c, _next) { | ||
| // Each throw gets a unique suffix so the Dedupe integration doesn't collapse the identical errors | ||
| // that several tests (and their retries) trigger through this shared middleware — otherwise only the | ||
| // first would be reported and the other tests' `waitForError` would time out. Tests match on the | ||
| // stable `Middleware error` prefix. | ||
| throw new Error(`Middleware error #${(failingMiddlewareCount += 1)}`); | ||
| }; | ||
|
|
||
| // Intentionally a NAMED function expression (unlike the anonymous ones above) so the named-function | ||
| // path stays covered: the span name is taken from the function's own name. Under bundled Bun the inner | ||
| // name collides with the `const` binding and is suffixed (→ `namedMiddleware2`), so the test matches on | ||
| // the `namedMiddleware` prefix rather than an exact name. | ||
| export const namedMiddleware: MiddlewareHandler = async function namedMiddleware(_c, next) { | ||
| await next(); | ||
| }; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,45 @@ | ||
| import { Hono } from 'hono'; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This whole e2e test is very heavily overlapping with hono-4-legacy. I think that's fine for now, but there's probably some opportunity to collapse these. (This is relevant to some of the test issues discussed this morning, can definitely be part of a bigger effort to come later.) |
||
| 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 }; | ||
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.
l: That is not used (anymore), right? Not sure if this is a left over or needed for another PR.