feat(core, node): portable Express integration#19928
Conversation
Semver Impact of This PR🟡 Minor (new features) 📋 Changelog PreviewThis is how your changes will appear in the changelog. New Features ✨
Bug Fixes 🐛
Documentation 📚
Internal Changes 🔧
🤖 This preview updates automatically when you update the PR. |
![cursor[bot]](/s/com/githubusercontent/avatars/G.https/in/1210556?s=60&v=4){kind=small}
size-limit report 📦
|
node-overhead report 🧳Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.
|
isaacs
force-pushed
the
isaacschlueter/portable-express-integration
branch
from
3cddcee to
dc1b9cf
Compare
isaacs
force-pushed
the
isaacschlueter/portable-express-integration
branch
5 times, most recently
from
64c45e7 to
fd4fefe
Compare
![sentry[bot]](/s/com/githubusercontent/avatars/G.https/in/12637?s=60&v=4){kind=small}
isaacs
force-pushed
the
isaacschlueter/portable-express-integration
branch
from
7e0d74f to
db667fc
Compare
isaacs
force-pushed
the
isaacschlueter/portable-express-integration
branch
2 times, most recently
from
b2d4e3c to
d5d4e9b
Compare
isaacs
force-pushed
the
isaacschlueter/portable-express-integration
branch
2 times, most recently
from
f694a6b to
23f24a1
Compare
isaacs
force-pushed
the
isaacschlueter/portable-express-integration
branch
from
23f24a1 to
37d4883
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Patched
layer.handlebecomes non-writable unintentionally- Added
writable: trueto theObject.definePropertydescriptor so patchedlayer.handlepreserves Express's original writability for direct reassignment.
- Added
Or push these changes by commenting:
@cursor push 6524f61f18
Preview (6524f61f18)
diff --git a/packages/core/src/integrations/express/patch-layer.ts b/packages/core/src/integrations/express/patch-layer.ts
--- a/packages/core/src/integrations/express/patch-layer.ts
+++ b/packages/core/src/integrations/express/patch-layer.ts
@@ -46,6 +46,7 @@
Object.defineProperty(layer, 'handle', {
enumerable: true,
configurable: true,
+ writable: true,
value: function layerHandlePatched(
this: ExpressLayer,
req: ExpressRequest,This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.
Lms24
left a comment
Lms24
left a comment
There was a problem hiding this comment.
Thanks for porting this instrumentation! I had some questions and suggestions. I got a bit confused how far along we are with "Sentryfying" the instrumentation. I think it's fine if we make certain changes, like simplifying the span name logic, in follow-up PRs if the goal of this PR was primarily to vendor in code. Then again, we definitely made some changes in there already, so I suggested them anyway. Therefore, feel free to address some of my comments in a follow-up PR.
| 'express.type': 'request_handler', | ||
| 'http.route': '/test-transaction', | ||
| 'sentry.origin': 'auto.http.otel.express', | ||
| 'sentry.origin': 'auto.http.express', |
There was a problem hiding this comment.
a comment on the origin change (not just this specific line): I looked up (with Hex) if any alerts, dashboards or saved explore queries depend on sentry.origin (or span.origin which is what the product maps the attribute to for better or worse):
- Looks like no alerts are configured ✅
- only 10 dashboard widgets depend on any
sentry.originattributes. All of these are set on the same dashboard for the same org and none depend directly onauto.http.otel.express✅ - No discover queries (though Discover is a legacy feature anyway) ✅
- I didn't yet find any data for saved explore queries, so we're flying blind here for the moment. My guess is usage will be very low/non-existing.
Which means that I think it's fine to change the value now. No need to wait for a new major. Will try to get some more information on saved explore queries but this shouldn't block us.
dev-packages/e2e-tests/test-applications/tanstackstart-react/package.json
Show resolved
Hide resolved
| return express; | ||
| }; | ||
|
|
||
| export const unpatchExpressModule = (options: ExpressIntegrationOptions) => { |
There was a problem hiding this comment.
m/Q: do we need unpatching functionality at all? I know OTel has this but I'm not aware of a use case where we'd actually unpatch anything from within our SDK. I'm thinking if we can remove that part of the instrumentation, we'd probably get away with less code to worry about. Though if there's a reason I'm missing to keep it around, that's also totally fine!
There was a problem hiding this comment.
That is a really interesting point! If we don't have to worry about unpatching, then it really does simplify things quite a lot. No need to keep the original functions around, makes preventing double-wrap easier, etc.
I'm not aware of any need for that either, and I don't see anywhere we do that in our SDKs. So that's likely a good opportunity for simplification of these conversions, I think.
| // TODO: Fix router spans (getRouterPath does not work properly) to | ||
| // have useful names before removing this branch |
There was a problem hiding this comment.
is this a TODO we need to address in this PR or later?
There was a problem hiding this comment.
Actually, that's copied in from the original otel express instrumentation, where it arrived January of 2025. It looks like if the layer is a router type, then the name gets messed up.
If we're going to fix it, it could be with our own implementation or by porting a fix from upstream, but it seemed prudent to try to keep this close to the upstream implementation for now, and fix (or remove) the todo later.
There was a problem hiding this comment.
yeah sounds good, we can keep it!
| const spanName = getSpanName( | ||
| { | ||
| request: req, | ||
| layerType: type, | ||
| route: constructedRoute, | ||
| }, | ||
| metadata.name, | ||
| ); | ||
| return startSpanManual({ name: spanName, attributes }, span => { | ||
| // Also update the name, we don't need to "middleware - " prefix | ||
| const name = attributes[ATTR_EXPRESS_NAME]; | ||
| if (typeof name === 'string') { | ||
| // should this be updateSpanName? | ||
| span.updateName(name); | ||
| } |
There was a problem hiding this comment.
m: I see above we already started setting sentry.op and sentry.origin attributes, so I assume this part of the instrumentation is already "Sentryfied", correct?
In this case, does anything hold us back from already setting the correct span name within the startSpanManual options instead of updating it? Ideally we can clean this up. Once that's done, I believe we can get rid of a bit of code in inferSpanData which extracts name, op and sentry.source (IIRC) from spans. Not 100% sure though because this function covers a lot of categories of spans so chances are the express code path is shared with other server frameworks we didn't port yet.
Anyway, we can also do this afterwards but my thinking is that ideally we can set a span name right from the start and don't have to update it later on.
There was a problem hiding this comment.
Something else that I think we could handle here eventually: We have logic somewhere (sorry I don't recall where exactly but I'm sure we can find it), to update the active root (http.server) span name with the route name when we start a route handler span. At this point, we also need to update the sentry.source attribute of the span so that Relay knows this span has a "good" route name and no longer is a raw URL.
isaacs
force-pushed
the
isaacschlueter/portable-express-integration
branch
from
37d4883 to
a564a72
Compare
| "@tanstack/react-start": "^1.136.0", | ||
| "@tanstack/react-router": "^1.136.0", | ||
| "@tanstack/react-start": "^1.138.2", | ||
| "@tanstack/react-router": "^1.138.2", |
There was a problem hiding this comment.
Accidentally committed tanstackstart dependency version bump
Low Severity
The @tanstack/react-start and @tanstack/react-router dependencies were bumped from ^1.136.0 to ^1.138.2. The PR author confirmed in discussion this is "debugging scar tissue from tanstack update breaking integration tests the other day" and said they would "back out" the change. This version bump is unrelated to the Express integration work.
| // express considers anything but an empty value, "route" or "router" | ||
| // passed to its callback to be an error | ||
| const maybeError = args[0]; | ||
| const isError = ![undefined, null, 'route', 'router'].includes(maybeError); |
There was a problem hiding this comment.
Falsy non-null values incorrectly detected as errors
Low Severity
The error detection ![undefined, null, 'route', 'router'].includes(maybeError) treats falsy values like 0, false, or '' as errors, while Express itself treats only truthy values (other than 'route'/'router') as errors. If middleware calls next(0) or next(false), the span would incorrectly get an error status.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 3 total unresolved issues (including 2 from previous reviews).
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| message, | ||
| }); | ||
| throw anyError; | ||
| /* v8 ignore next - it sees the block end at the throw */ |
There was a problem hiding this comment.
Missing captureException in startSpanManual error path
Medium Severity
The startSpanManual callback has a catch block (line 170) that catches errors thrown by layerHandleOriginal, sets the span status to error, and re-throws — but never calls captureException. Per the review rules, when calling startSpan variants, error cases need to consider calling captureException. The comment says "the error handler we assign does that," but that relies on the user having called setupExpressErrorHandler, which is not guaranteed.
Triggered by project rule: PR Review Guidelines for Cursor Bot
This extracts the functionality from the OTel Express intstrumentation,
replacing it with a portable standalone integration in `@sentry/core`,
which can be extended and applied to patch any Express module import in
whatever way makes sense for the platform in question.
Currently in node, that is still an OpenTelemetry intstrumentation, but
just handling the automatic module load instrumentation, not the entire
tracing integration.
This is somewhat a proof of concept, to see what it takes to port a
fairly invovled OTel integration into a state where it can support all
of the platforms that we care about, but it does impose a bit less of a
translation layer between OTel and Sentry semantics (for example, no
need to use the no-op `span.recordException()`).
User-visible changes (beyond the added export in `@sentry/core`):
- Express router spans have an origin of `auto.http.express` rather than
`auto.http.otel.express`, since it's no longer technically an otel
integration.
- The empty `measurements: {}` object is no longer attached to span
data, as that was an artifact of otel's `span.recordError`, which is a
no-op anyway, and no longer executed.
Obviously this is not a full clean-room reimplementation, and relies on
the fact that the opentelemetry-js-contrib project is Apache 2.0
licensed. I included the link to the upstream license in the index file
for the Express integration, but there may be a more appropriate way to
ensure that the license is respected properly. It was arguably a
derivative work already, but simple redistribution is somewhat different
than re-implementation with subtly different context.
This reduces the node overhead and makes the Express instrumentation
portable to other SDKs, but it of course *increases* the bundle size of
`@sentry/core` considerably. It would be a good idea to explore
splitting out integrations from core, so that they're bundled and
analyzed separately, rather than shipping to all SDKs that extend core.
isaacs
force-pushed
the
isaacschlueter/portable-express-integration
branch
from
6e5aa00 to
ff8586c
Compare
isaacs
force-pushed
the
isaacschlueter/portable-express-integration
branch
from
ff8586c to
c78e960
Compare
| }, | ||
| options?: ExpressHandlerOptions, | ||
| ): void { | ||
| app.use(expressRequestHandler()); |
There was a problem hiding this comment.
Bug: The expressRequestHandler is registered after application routes, preventing it from running on successful requests and capturing request data for Sentry events.
Severity: HIGH
Suggested Fix
The setupExpressErrorHandler function should be refactored. The app.use(expressRequestHandler()) call should be moved to occur before any application routes are defined. The app.use(expressErrorHandler(options)) call should remain after all routes, as is standard for Express error handlers. This ensures request data is captured for all requests, not just those that result in an error.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: packages/core/src/integrations/express/index.ts#L220
Potential issue: The `setupExpressErrorHandler` function registers the
`expressRequestHandler` middleware after all application routes are set up. Because
Express middleware executes sequentially and route handlers for successful requests
typically do not call `next()`, the `expressRequestHandler` is never invoked for
non-error paths. This handler is responsible for setting SDK processing metadata
containing the normalized request details. As a result, Sentry events for successful
requests will be missing crucial request data such as headers, cookies, query
parameters, and the request body, significantly hampering debugging efforts.
2 participants
This extracts the functionality from the OTel Express intstrumentation,
replacing it with a portable standalone integration in
@sentry/core,which can be extended and applied to patch any Express module import in
whatever way makes sense for the platform in question.
Currently in node, that is still an OpenTelemetry intstrumentation, but
just handling the automatic module load instrumentation, not the entire
tracing integration.
This is somewhat a proof of concept, to see what it takes to port a
fairly invovled OTel integration into a state where it can support all
of the platforms that we care about, but it does impose a bit less of a
translation layer between OTel and Sentry semantics (for example, no
need to use the no-op
span.recordException()).User-visible changes (beyond the added export in
@sentry/core):auto.http.expressrather thanauto.http.otel.express, since it's no longer technically an otelintegration.
measurements: {}object is no longer attached to spandata, as that was an artifact of otel's
span.recordError, which is ano-op anyway, and no longer executed.
Obviously this is not a full clean-room reimplementation, and relies on
the fact that the opentelemetry-js-contrib project is Apache 2.0
licensed. I included the link to the upstream license in the index file
for the Express integration, but there may be a more appropriate way to
ensure that the license is respected properly. It was arguably a
derivative work already, but simple redistribution is somewhat different
than re-implementation with subtly different context.
This reduces the node overhead and makes the Express instrumentation
portable to other SDKs, but it of course increases the bundle size of
@sentry/coreconsiderably. It would be a good idea to exploresplitting out integrations from core, so that they're bundled and
analyzed separately, rather than shipping to all SDKs that extend core.
Closes #19929 (added automatically)