Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

feat(node): Update httpIntegration handling of incoming requests #17371

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

Merged
mydea merged 16 commits into develop from fn/ref-otel-incoming-request
Sep 3, 2025

Conversation

Copy link
Member

@mydea mydea commented Aug 11, 2025 •
edited
Loading

This PR updates the handling of the Node SDK of incoming requests. Instead of relying on @opentelemetry/instrumentation-http for this, we now handle this internally, ensuring that we can optimize performance as much as possible and avoid interop problems. This will also allow us to extract this out of an OTEL instrumentation, eventually paving the way for a non-OTEL SDK with basic tracing capabilities.

This change should not affect users, unless they are relying on very in-depth implementation details. Importantly, this also drops the _experimentalConfig option of the integration - this will no longer do anything.
Finally, you can still pass instrumentation.{requestHook,responseHook,applyCustomAttributesOnSpan} options, but they are deprecated and will be removed in v11. Instead, you can use the new incomingRequestSpanHook configuration option if you want to adjust the incoming request span.

I have tried to ensure the spans look as similar as possible as before, this should be reflected in the tests.

seer-by-sentry[bot] reacted with hooray emoji
@mydea mydea self-assigned this Aug 11, 2025
Copy link
Contributor

github-actions bot commented Aug 11, 2025 •
edited
Loading

size-limit report 📦

Path Size % Change Change
@sentry/browser 24.16 kB - -
@sentry/browser - with treeshaking flags 22.73 kB - -
@sentry/browser (incl. Tracing) 39.87 kB - -
@sentry/browser (incl. Tracing, Replay) 78.23 kB - -
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags 68.02 kB - -
@sentry/browser (incl. Tracing, Replay with Canvas) 82.91 kB - -
@sentry/browser (incl. Tracing, Replay, Feedback) 95.04 kB - -
@sentry/browser (incl. Feedback) 40.83 kB - -
@sentry/browser (incl. sendFeedback) 28.81 kB - -
@sentry/browser (incl. FeedbackAsync) 33.7 kB - -
@sentry/react 25.88 kB - -
@sentry/react (incl. Tracing) 41.89 kB - -
@sentry/vue 28.64 kB - -
@sentry/vue (incl. Tracing) 41.69 kB - -
@sentry/svelte 24.18 kB - -
CDN Bundle 25.66 kB - -
CDN Bundle (incl. Tracing) 39.75 kB - -
CDN Bundle (incl. Tracing, Replay) 76.03 kB - -
CDN Bundle (incl. Tracing, Replay, Feedback) 81.47 kB - -
CDN Bundle - uncompressed 74.96 kB - -
CDN Bundle (incl. Tracing) - uncompressed 117.59 kB - -
CDN Bundle (incl. Tracing, Replay) - uncompressed 232.69 kB - -
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed 245.31 kB - -
@sentry/nextjs (client) 43.89 kB - -
@sentry/sveltekit (client) 40.33 kB - -
@sentry/node-core 49.61 kB +3.35% +1.61 kB 🔺
@sentry/node 150.26 kB +0.67% +991 B 🔺
@sentry/node - without tracing 92.19 kB -0.07% -64 B 🔽
@sentry/aws-serverless 105.48 kB +0.54% +565 B 🔺

View base workflow run

@mydea mydea force-pushed the fn/ref-otel-incoming-request branch from 8d7f49c to e798a09 Compare August 11, 2025 10:49
@mydea mydea force-pushed the fn/ref-otel-incoming-request branch from e798a09 to 57f8184 Compare August 11, 2025 11:30
@mydea mydea changed the base branch from develop to fn/add-full-attribute-http-span-test August 11, 2025 11:30
const runner = createRunner(__dirname, 'server.js')
.expect({
transaction: {
transaction: 'GET /test/:id/span-updateName-source',
transaction: 'new-name',
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is actually... fixed here now? Not quite sure why/how, but apparently some timing stuff is better now?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this indicates that the retroactive span name extraction is no longer use. Which makes sense to me if we now set a good name right from the start!

(I think this is fine btw. Actually leaves less room for user error in case they don't use the Sentry.updateSpanName helper)

@mydea mydea force-pushed the fn/ref-otel-incoming-request branch 3 times, most recently from 2f90c3c to c120396 Compare August 11, 2025 12:01
mydea added a commit that referenced this pull request Aug 11, 2025 
...17373)
Add tests that definitely show:
1. How get and post spans are captured
2. How URLs are handled with params and fragments
This should make
#17371 much easier to
verify.
Base automatically changed from fn/add-full-attribute-http-span-test to develop August 11, 2025 13:00
@mydea mydea force-pushed the fn/ref-otel-incoming-request branch 2 times, most recently from a280e0c to 7b052e3 Compare August 12, 2025 14:15
status: 'ok',
},
},
describe('custom server.emit', () => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these tests have been added to verify that everything still works even if a user (or some other library) also wraps server.emit. This was the case in the nestjs-distributed-tracing E2E test.

@mydea mydea force-pushed the fn/ref-otel-incoming-request branch from 8a65711 to d9e8b1f Compare August 13, 2025 09:32
span.end();

// Update the transaction name if the route has changed
if (newAttributes['http.route']) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we control this now, we can actually do this in a generalized way instead of doing this per-framework, I think most if not all of them (e.g. express etc.) use this mechanism to set the route, so we could eventually remove the per-integration code for this in many places, I believe (and we'll also automatically support other frameworks in the future). cc @Lms24

Lms24 reacted with thumbs up emoji
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!

// This fails https://github.com/getsentry/sentry-javascript/pull/12587#issuecomment-2181019422
// Skipping this for now so we don't block releases
test.skip('Should record spans from http instrumentation', async ({ request }) => {
test('Should record spans from http instrumentation', async ({ request }) => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure if this is fixed by this specifically or something else, but this seems to work again now.

Lms24 and andreiborza reacted with thumbs up emoji
@mydea mydea force-pushed the fn/ref-otel-incoming-request branch from a6259bf to 434adfe Compare August 13, 2025 13:24
@mydea mydea marked this pull request as ready for review August 13, 2025 13:24
Copy link
Member

@Lms24 Lms24 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Looking forward to basic tracing without always depending on OTel. Had some minor remarks but looks ready to me otherwise!

@@ -224,7 +224,7 @@ module.exports = [
import: createImport('init'),
ignore: [...builtinModules, ...nodePrefixedBuiltinModules],
gzip: true,
limit: '116 KB',
limit: '51 KB',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we just forgot to adjust this? Would be pretty sweet otherwise xD

Copy link
Member

@andreiborza andreiborza Aug 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just counted this wrongly 😅

const runner = createRunner(__dirname, 'server.js')
.expect({
transaction: {
transaction: 'GET /test/:id/span-updateName-source',
transaction: 'new-name',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this indicates that the retroactive span name extraction is no longer use. Which makes sense to me if we now set a good name right from the start!

(I think this is fine btw. Actually leaves less room for user error in case they don't use the Sentry.updateSpanName helper)

span.end();

// Update the transaction name if the route has changed
if (newAttributes['http.route']) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!

mydea added a commit that referenced this pull request Aug 13, 2025 
Extracted out of
#17371
I noticed that we were not fully consistent in instrumentation IDs for
integrations that have multiple instrumentation. The intent is that
users can provide the _integration name_ (e.g. `Http`) and it will
preload all http instrumentation. To achieve this, I adjusted the
preload filter code to look for exact matches as well as
`startsWith(`${name}.id`)`. I also adjusted the test to be more
declarative and mock/reset stuff properly (this lead to issues in the
linked PR, and should generally be a bit cleaner).
I also updated all instrumentation IDs to follow this pattern. We should
be mindful of following this with new instrumentation we add.
@mydea mydea force-pushed the fn/ref-otel-incoming-request branch 2 times, most recently from b8613b0 to ce6aaa1 Compare August 14, 2025 06:51
cursor[bot]

This comment was marked as outdated.

@mydea mydea force-pushed the fn/ref-otel-incoming-request branch from ff6cc6d to cfd2e10 Compare August 29, 2025 11:06
@mydea mydea force-pushed the fn/ref-otel-incoming-request branch from cfd2e10 to c508c61 Compare September 2, 2025 09:55
Copy link
Contributor

github-actions bot commented Sep 2, 2025 •
edited
Loading

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.

Scenario Requests/s % of Baseline Prev. Requests/s Change %
GET Baseline 9,250 - 9,023 +3%
GET With Sentry 1,291 14% 1,291 -
GET With Sentry (error only) 5,865 63% 4,078 +44%
POST Baseline 1,197 - 1,199 -0%
POST With Sentry 495 41% 475 +4%
POST With Sentry (error only) 1,071 89% 946 +13%
MYSQL Baseline 3,361 - 3,313 +1%
MYSQL With Sentry 472 14% 424 +11%
MYSQL With Sentry (error only) 2,698 80% 2,208 +22%

View base workflow run

cursor[bot]

This comment was marked as outdated.

@mydea mydea merged commit 84243e6 into develop Sep 3, 2025
170 checks passed
@mydea mydea deleted the fn/ref-otel-incoming-request branch September 3, 2025 09:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Lms24 Lms24 Lms24 approved these changes

@cursor cursor[bot] cursor[bot] left review comments

@andreiborza andreiborza andreiborza approved these changes

@AbhiPrasad AbhiPrasad Awaiting requested review from AbhiPrasad

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /