Uh oh!
There was an error while loading. Please reload this page.
feat(server-utils): Implement orchestrion-based express integration - #21889
Conversation
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
b086dd5 to
9b43cbbCompareUh oh!
There was an error while loading. Please reload this page.
d72f9d1 to
e9acef3Compare| // Express only treats a 4-arg handler as an error handler and skips it in | ||
| // the normal request pipeline; match the OTel integration and don't trace it. | ||
| if (layer.handle?.length === 4) { |
There was a problem hiding this comment.
super-l: In theory we could add all of these checks in one if:
!layer||!Array.isArray(args)||layer.handle?.length===4|| ...There was a problem hiding this comment.
I think it's fine to keep it like this to keep the comments and things a bit clearer 🤔
Uh oh!
There was an error while loading. Please reload this page.
| if (isOrchestrionEnabled()) { | ||
| // The orchestrion router span stays open until the response finishes, so | ||
| // it spans the whole sub-stack it dispatched (~the 100ms handler delay). | ||
| expect(routerDurationMs).toBeGreaterThan(50); |
There was a problem hiding this comment.
l: Maybe it makes sense to also have an upper boundary as well?
There was a problem hiding this comment.
not sure what would be reasonable, we basically just want to test that there is any reasonable duration, I don't think we care about an upper limit?
There was a problem hiding this comment.
yeah good question about what a reasonable duration is. let's see if this leads to any issues. fine by me to leave it as is for now
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 0405e01. Configure here.
| // The OTel integration ends router spans immediately, so the router span | ||
| // is a ~0ms marker regardless of how long its sub-stack runs. | ||
| expect(routerDurationMs).toBeLessThan(50); | ||
| } |
There was a problem hiding this comment.
Test uses conditional instead of separate test cases
Low Severity
The test at this location uses if (isOrchestrionEnabled()) { ... } else { ... } to branch assertions inside a single test body. Per testing conventions, conditionals in a single test are flagged — the recommendation is to split the different paths into separate tests (e.g., one for orchestrion behavior and one for OTel behavior) to improve clarity and avoid accidentally hiding failures in one branch.
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit 0405e01. Configure here.
Uh oh!
There was an error while loading. Please reload this page.
0405e01 to
c98196cCompareUh oh!
There was an error while loading. Please reload this page.
size-limit report 📦
|
c98196c to
e637718CompareUh oh!
There was an error while loading. Please reload this page.
e637718 to
430b676Compare
Closes#20917
This adds a new
expressChannelIntegrationbased on orchestrion. It should keep feature parity — we have a lot of Express tests, and the core tracing tests now run against this integration (via theorchestrionvariant), which gives good coverage for now.A follow up would be to also handle the error handler automatically where possible.
Route resolution: metadata is only set for genuinely matched routes
The OpenTelemetry Express integration patches every layer's
handle, reconstructs the route from the layers that ran (getConstructedRoute), and validates it against the request URL (getActualMatchedRoute) to discard paths that didn't truly match.This integration instead hooks Express's per-layer
Layer.prototype.handle_request(v4) /handleRequest(v5, therouterpackage). Express only calls that method for a layer whose path and method matched, so arequest_handlerspan only ever exists for a route that genuinely matched. Route metadata —http.route, the roothttp.serverspan's route attribute, and the transaction name — is therefore taken directly from the matched route, with no separate reconstruct/validate step.The observable consequence, which is intentional:
request_handlerspan is created and no route (or route-based transaction name) is set.middleware-type spans, which never set route attributes.This is a different mechanism from the OTel path, but it lands on the same net result:
getActualMatchedRoutereturnsundefinedin exactly these cases, so the OTel integration also emits no route there. We get it structurally, from the injection point, rather than by validating after the fact.