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 ✨Deps
Other
Bug Fixes 🐛Cloudflare
Core
Deps
Other
Internal Changes 🔧Deps Dev
Nuxt
Other
🤖 This preview updates automatically when you update the PR. |
![cursor[bot]](https://avatars.githubusercontent.com/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]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| ExpressHandlerOptions, | ||
| ExpressMiddleware, | ||
| ExpressErrorMiddleware, | ||
| } from './integrations/express/types'; |
There was a problem hiding this comment.
Express integration adds to @sentry/core bundle size
Low Severity
Exporting the Express integration (~700 lines across five new files) from @sentry/core increases the core package size that ships to all platforms, including browsers where Express is never used. While tree-shaking can eliminate unused exports, not all bundler configurations guarantee it. The PR author acknowledges this and suggests exploring splitting integrations out of core. Flagged because this was mentioned in the rules file.
Triggered by project rule: PR Review Guidelines for Cursor Bot
There was a problem hiding this comment.
Fwiw, I don't think bundle size is directly a concern here, as long as this instrumentation isn't actually used in browser code. We should assume tree shaking being the standard these days for browser builds and more generally also for platforms where size is an issue (AWS/vercel, Cloudflare). We use tree shaking extensively when we build our own CDN bundles and everyone using relatively modern versions of Webpack, Turbopack, Rollup/-down, Vite, etc benefits from good tree shaking default configurations.
We haven't heard recent complaints about bundle size increases coming directly from adding exports to @sentry/code. The complaints usually relate to already tree-shaken browser code (which is fair but there's relatively little we can do without and maybe a bit more with breaking changes).
There are other reasons though for a better separation, such as import resolving, module parsing and build time performance (see #19885 as an example). So I think long-term, better separating exports from core sounds reasonable but I wonder if we can achieve this via subpath exports rather than separate packages.
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
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
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).
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.
|
|
||
| Object.defineProperty(layer, 'handle', { | ||
| enumerable: true, | ||
| configurable: true, |
There was a problem hiding this comment.
Patched layer.handle becomes non-writable unintentionally
Medium Severity
The Object.defineProperty call sets value but omits writable, which defaults to false. Express originally creates layer.handle as a writable property (this.handle = fn). After patching, direct assignment like layer.handle = otherFn silently fails in sloppy mode or throws a TypeError in strict mode. This could break other instrumentation tools (e.g., shimmer/wrap) that use direct assignment to wrap layer.handle. Adding writable: true to the descriptor would preserve the original property characteristics.
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)