feat!: Use handler span op for terminal request handlers - #22871
Conversation
size-limit report 📦
|
msonnb
changed the title
function span op for terminal request handlersfunction span op for terminal request handlers
nicohrubec
left a comment
nicohrubec
left a comment
There was a problem hiding this comment.
looks like the PR needs to be rebased because it looks like the diff still includes the changes from #22852
isaacs
left a comment
isaacs
left a comment
There was a problem hiding this comment.
Some minor notes/questions, nothing worth gating, imo. Looks good!
At some point when these are all finished, we're going to likely want to add a note in MIGRATION.md to the effect that ignoreSpans: [{ op: 'express.router.middleware' }] or whatever will no longer work, because the op field is getting blunter.
|
|
||
| const attributes: Record<string, string> = { | ||
| [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: ORIGIN, | ||
| [SENTRY_OP]: REQUEST_HANDLER_OP, |
There was a problem hiding this comment.
If I'm reading this right, this puts a op: 'function' span as the parent of another op: 'function' span (on line 356), which seems a bit odd? Do we need an op here?
There was a problem hiding this comment.
it's indeed a bit odd, will take a look in a follow-up 👍
| /** | ||
| * Patches `app.request()` on a Hono instance so that each internal dispatch | ||
| * is traced as a `hono.request` span — child of whatever span is active at | ||
| * is traced as an `http.server` span — child of whatever span is active at |
There was a problem hiding this comment.
I had remembered seeing cases where http.server was assumed to be on root spans, and went looking for cases where it might be used as an indicator of root-ness. But they do not. Every one of them already holds the root before testing the op. So, I think this is safe.
|
|
||
| function startMetadataSpan(metadata: SpanMetadata, original: () => unknown): unknown { | ||
| const hapiType = metadata.attributes[AttributeNames.HAPI_TYPE]; | ||
| const op = hapiType === HapiLayerType.PLUGIN ? WEB_SERVER_FUNCTION_SPAN_OP : `${hapiType}.hapi`; |
There was a problem hiding this comment.
So, we still have op fields with .hapi? I thought that we were trying to make them all known conventional types? (If this is coming in a subsequent PR, ignore, it's fine to do these piecemeal, of course.)
There was a problem hiding this comment.
yes this (and koa, express) will be updated to router which needs to land in conventions first
There was a problem hiding this comment.
Same comment here as the .hapi op above
fa5721f to
d78a45b
Compare
![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.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit d78a45b. Configure here.
yup i was planning on doing the MIGRATION note all at once at the end 👍 |
d78a45b to
d09a042
Compare
msonnb
changed the title
function span op for terminal request handlershandler span op for terminal request handlers
|
Discussed with @Lms24 that we'll add a new |
Migrate the terminal request-handler span ops across the server integrations to the cross-framework `function` op, and trace Hono's internal `app.request()` dispatch as an `http.server` span: - express: `request_handler.express` -> `function` - fastify: `request_handler.fastify` -> `function` - elysia: `request_handler.elysia` -> `function` - nestjs: `handler.nestjs` -> `function` - hono: `hono.request` -> `http.server` Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A plugin-registered hapi route runs the user's request handler, so it is a terminal handler like the express/fastify/hono routes in this migration. Map `plugin.hapi` to the cross-framework `function` op; `router.hapi` and `server.ext.hapi` (framework routing/extension lifecycle) keep their ops. Op is set via the `sentry.op` attribute only; `hapi.type` is unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
d09a042 to
9eb802c
Compare
4 participants
Collapses the terminal request-handler span ops across the server frameworks to the cross-framework
handlerop, so every "this is where the user's route handler runs" span shares one op regardless of framework.Hono’s app.request() is an internal HTTP dispatch (one sub-app calling another in-process), so it is semantically an inbound server request and is now traced as
http.serverPart of #22446