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): Add incoming request headers as OTel span attributes #17475

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

Open
s1gr1d wants to merge 13 commits into develop
base: develop
Choose a base branch
Loading
from sig/http-attributes

Conversation

Copy link
Member

@s1gr1d s1gr1d commented Aug 28, 2025 •
edited
Loading

Extracting incoming request data to span attributes.

It's a very long PR but most of it are test edits as the new http attributes are added to existing tests (otherwise, they would fail).

Those are the most important changes:

  • add a function httpHeadersToSpanAttributes in core that is used for generating the attributes
  • adding extra logic in astro, bun, cloudflare, nextjs, remix and sveltekit (custom handler instrumentation) to include the attributes

⚠️ The headers are also added outgoing http.client spans to Koa and in Fastify - I couldn't find out why so far 🤔
This is where I add the attributes in incoming-request. When those attributes are not added, the node-core tests fail as the HTTP header attributes are not added. But the koa and fastify test add the HTTP header as span attributes with this.

Closes #17452

seer-by-sentry[bot] reacted with hooray emoji
Copy link
Member Author

Choose a reason for hiding this comment

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

I ran prettier fix and that's why this and the following three files are included in the diff.

Copy link
Contributor

github-actions bot commented Aug 28, 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 48.2 kB +0.4% +188 B 🔺
@sentry/node 149.56 kB +0.19% +282 B 🔺
@sentry/node - without tracing 92.53 kB +0.31% +277 B 🔺
@sentry/aws-serverless 105.2 kB +0.28% +291 B 🔺

View base workflow run

cursor[bot]

This comment was marked as outdated.

@cleptric cleptric self-assigned this Aug 28, 2025
Copy link
Member

@mydea mydea left a comment

Choose a reason for hiding this comment

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

we need to make sure to delete PII-related headers, similar to what we do in extractNormalizedRequestData!

s1gr1d reacted with thumbs up emoji
cursor[bot]

This comment was marked as outdated.

cursor[bot]

This comment was marked as outdated.

cursor[bot]

This comment was marked as outdated.

@s1gr1d s1gr1d requested review from mydea and Lms24 September 1, 2025 12:05
Comment on lines 116 to 119
'http.request.header.baggage': [expect.any(String)],
'http.request.header.connection': ['keep-alive'],
'http.request.header.host': [expect.any(String)],
'http.request.header.sentry_trace': [expect.stringContaining(traceId || '')],
Copy link
Member Author

Choose a reason for hiding this comment

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

Should we also add the baggage and sentry_trace headers?

Copy link
Member

@cleptric cleptric Sep 2, 2025

Choose a reason for hiding this comment

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

If this is a general question, not related to this test, then yes please. All request headers 😄

@s1gr1d s1gr1d changed the title (削除) feat(node): Add request headers as OTel span attributes (削除ここまで) (追記) feat(node): Add incoming request headers as OTel span attributes (追記ここまで) Sep 1, 2025
cursor[bot]

This comment was marked as outdated.

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 8,595 - 9,033 -5%
GET With Sentry 1,197 14% 1,302 -8%
GET With Sentry (error only) 3,878 45% 4,333 -11%
POST Baseline 1,190 - 1,204 -1%
POST With Sentry 455 38% 484 -6%
POST With Sentry (error only) 913 77% 947 -4%
MYSQL Baseline 3,261 - 3,326 -2%
MYSQL With Sentry 390 12% 404 -3%
MYSQL With Sentry (error only) 2,108 65% 2,203 -4%

View base workflow run

@@ -47,6 +48,21 @@ function onSpanStart(span: Span, parentContext: Context): void {

logSpanStart(span);

if (scopes?.isolationScope) {
Copy link
Member

Choose a reason for hiding this comment

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

m: Why are we setting this here again? Why not just in the http integration directly?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mydea mydea mydea approved these changes

@cleptric cleptric cleptric left review comments

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

@chargome chargome Awaiting requested review from chargome

@Lms24 Lms24 Awaiting requested review from Lms24

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Extract incoming Http request headers to span attributes

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