Skip to content

fix(node): Ensure httpIntegration propagates traces - #15233

Merged
mydea merged 5 commits into
developfrom
fn/node-http-inject
Jan 31, 2025
Merged

fix(node): Ensure httpIntegration propagates traces#15233
mydea merged 5 commits into
developfrom
fn/node-http-inject

Conversation

@mydea

Copy link
Copy Markdown
Member

Related to #15231, I noticed that we today would not propagate traces in outgoing http requests if:

  1. The user configures httpIntegration({ spans: false })
  2. ...or the user has a custom OTEL setup
  3. and the user does not add their own HttpInstrumentation

Admittedly and edge case. More importantly, though, by actually adding distributed tracing information here, we are unblocked from potentially stopping to ship the http-instrumentation to users that do not need spans (and/or have a custom otel setup).

@mydea
mydea requested review from Lms24 and lforstJanuary 30, 2025 13:10
@mydeamydea self-assigned this Jan 30, 2025
@github-actions

github-actionsBot commented Jan 30, 2025

Copy link
Copy Markdown
Contributor

size-limit report 📦

PathSize% ChangeChange
@sentry/browser23.03 KB--
@sentry/browser - with treeshaking flags22.92 KB--
@sentry/browser (incl. Tracing)35.8 KB--
@sentry/browser (incl. Tracing, Replay)72.59 KB--
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags66.13 KB--
@sentry/browser (incl. Tracing, Replay with Canvas)76.85 KB--
@sentry/browser (incl. Tracing, Replay, Feedback)89.35 KB--
@sentry/browser (incl. Feedback)39.71 KB--
@sentry/browser (incl. sendFeedback)27.66 KB--
@sentry/browser (incl. FeedbackAsync)32.44 KB--
@sentry/react24.84 KB--
@sentry/react (incl. Tracing)37.69 KB--
@sentry/vue27.14 KB--
@sentry/vue (incl. Tracing)37.52 KB--
@sentry/svelte23.15 KB--
CDN Bundle24.23 KB--
CDN Bundle (incl. Tracing)35.91 KB--
CDN Bundle (incl. Tracing, Replay)70.56 KB--
CDN Bundle (incl. Tracing, Replay, Feedback)75.69 KB--
CDN Bundle - uncompressed70.83 KB--
CDN Bundle (incl. Tracing) - uncompressed106.57 KB--
CDN Bundle (incl. Tracing, Replay) - uncompressed217.42 KB--
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed229.99 KB--
@sentry/nextjs (client)38.65 KB--
@sentry/sveltekit (client)36.3 KB--
@sentry/node156.52 KB+0.1%+160 B 🔺
@sentry/node - without tracing97.64 KB+0.2%+195 B 🔺
@sentry/aws-serverless107.08 KB+0.16%+170 B 🔺

View base workflow run

Comment threadpackages/node/src/integrations/http/SentryHttpInstrumentation.ts Outdated
// If the core UndiciInstrumentation is registered, it will already have set the headers
// We do not want to add any then
if (!headers[k]) {
headers[k] = v;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

m: This works for sentry-trace but since baggage is standardized, other libraries or users might add their own entries. Which means we need to check for sentry- values in existing headers.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I updated it to merge baggages together!

mydeaand others added 3 commits January 31, 2025 09:53
We used to rely on the existence of another `httpIntegration`
Co-authored-by: Lukas Stracke <lukas.stracke@sentry.io>
@mydea
mydeaforce-pushed the fn/node-http-inject branch from dff0b91 to 81090deCompareJanuary 31, 2025 09:36
@mydea
mydea merged commit 3e31bdc into developJan 31, 2025
@mydea
mydea deleted the fn/node-http-inject branch January 31, 2025 10:06
mydea added a commit that referenced this pull request Mar 19, 2025
This is another stab at #15233, which was reverted here: #15354, with a different implementation. It is required to make #15730 work.
chargome pushed a commit that referenced this pull request Mar 19, 2025
This is another stab at #15233, which was reverted here: #15354, with a different implementation. It is required to make #15730 work.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@mydea@Lms24
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
fix(node): Ensure `httpIntegration` propagates traces by mydea · Pull Request #15233 · getsentry/sentry-javascript · GitHub
Skip to content

fix(node): Ensure httpIntegration propagates traces - #15233

Merged
mydea merged 5 commits into
developfrom
fn/node-http-inject
Jan 31, 2025
Merged

fix(node): Ensure httpIntegration propagates traces#15233
mydea merged 5 commits into
developfrom
fn/node-http-inject

Conversation

@mydea

Copy link
Copy Markdown
Member

Related to #15231, I noticed that we today would not propagate traces in outgoing http requests if:

  1. The user configures httpIntegration({ spans: false })
  2. ...or the user has a custom OTEL setup
  3. and the user does not add their own HttpInstrumentation

Admittedly and edge case. More importantly, though, by actually adding distributed tracing information here, we are unblocked from potentially stopping to ship the http-instrumentation to users that do not need spans (and/or have a custom otel setup).

@mydea
mydea requested review from Lms24 and lforstJanuary 30, 2025 13:10
@mydeamydea self-assigned this Jan 30, 2025
@github-actions

github-actionsBot commented Jan 30, 2025

Copy link
Copy Markdown
Contributor

size-limit report 📦

PathSize% ChangeChange
@sentry/browser23.03 KB--
@sentry/browser - with treeshaking flags22.92 KB--
@sentry/browser (incl. Tracing)35.8 KB--
@sentry/browser (incl. Tracing, Replay)72.59 KB--
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags66.13 KB--
@sentry/browser (incl. Tracing, Replay with Canvas)76.85 KB--
@sentry/browser (incl. Tracing, Replay, Feedback)89.35 KB--
@sentry/browser (incl. Feedback)39.71 KB--
@sentry/browser (incl. sendFeedback)27.66 KB--
@sentry/browser (incl. FeedbackAsync)32.44 KB--
@sentry/react24.84 KB--
@sentry/react (incl. Tracing)37.69 KB--
@sentry/vue27.14 KB--
@sentry/vue (incl. Tracing)37.52 KB--
@sentry/svelte23.15 KB--
CDN Bundle24.23 KB--
CDN Bundle (incl. Tracing)35.91 KB--
CDN Bundle (incl. Tracing, Replay)70.56 KB--
CDN Bundle (incl. Tracing, Replay, Feedback)75.69 KB--
CDN Bundle - uncompressed70.83 KB--
CDN Bundle (incl. Tracing) - uncompressed106.57 KB--
CDN Bundle (incl. Tracing, Replay) - uncompressed217.42 KB--
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed229.99 KB--
@sentry/nextjs (client)38.65 KB--
@sentry/sveltekit (client)36.3 KB--
@sentry/node156.52 KB+0.1%+160 B 🔺
@sentry/node - without tracing97.64 KB+0.2%+195 B 🔺
@sentry/aws-serverless107.08 KB+0.16%+170 B 🔺

View base workflow run

Comment threadpackages/node/src/integrations/http/SentryHttpInstrumentation.ts Outdated
// If the core UndiciInstrumentation is registered, it will already have set the headers
// We do not want to add any then
if (!headers[k]) {
headers[k] = v;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

m: This works for sentry-trace but since baggage is standardized, other libraries or users might add their own entries. Which means we need to check for sentry- values in existing headers.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I updated it to merge baggages together!

mydeaand others added 3 commits January 31, 2025 09:53
We used to rely on the existence of another `httpIntegration`
Co-authored-by: Lukas Stracke <lukas.stracke@sentry.io>
@mydea
mydeaforce-pushed the fn/node-http-inject branch from dff0b91 to 81090deCompareJanuary 31, 2025 09:36
@mydea
mydea merged commit 3e31bdc into developJan 31, 2025
@mydea
mydea deleted the fn/node-http-inject branch January 31, 2025 10:06
mydea added a commit that referenced this pull request Mar 19, 2025
This is another stab at #15233, which was reverted here: #15354, with a different implementation. It is required to make #15730 work.
chargome pushed a commit that referenced this pull request Mar 19, 2025
This is another stab at #15233, which was reverted here: #15354, with a different implementation. It is required to make #15730 work.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@mydea@Lms24
, 'i'); if (__m === '*' || __re.test(location.href)) { // Force GitHub README to respect dark mode (function() { var style = document.createElement('style'); style.textContent = ' .markdown-body { color-scheme: dark light; } .markdown-body pre { background: #161b22 !important; } .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; } .markdown-body table th, .markdown-body table td { border-color: #30363d !important; } .markdown-body img { background: #0d1117; } .markdown-body blockquote { border-left-color: #8b949e; } .markdown-body hr { border-color: #30363d; } '; document.head.appendChild(style); })(); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' fix(node): Ensure `httpIntegration` propagates traces by mydea · Pull Request #15233 · getsentry/sentry-javascript · GitHub
Skip to content

fix(node): Ensure httpIntegration propagates traces - #15233

Merged
mydea merged 5 commits into
developfrom
fn/node-http-inject
Jan 31, 2025
Merged

fix(node): Ensure httpIntegration propagates traces#15233
mydea merged 5 commits into
developfrom
fn/node-http-inject

Conversation

@mydea

Copy link
Copy Markdown
Member

Related to #15231, I noticed that we today would not propagate traces in outgoing http requests if:

  1. The user configures httpIntegration({ spans: false })
  2. ...or the user has a custom OTEL setup
  3. and the user does not add their own HttpInstrumentation

Admittedly and edge case. More importantly, though, by actually adding distributed tracing information here, we are unblocked from potentially stopping to ship the http-instrumentation to users that do not need spans (and/or have a custom otel setup).

@mydea
mydea requested review from Lms24 and lforstJanuary 30, 2025 13:10
@mydeamydea self-assigned this Jan 30, 2025
@github-actions

github-actionsBot commented Jan 30, 2025

Copy link
Copy Markdown
Contributor

size-limit report 📦

PathSize% ChangeChange
@sentry/browser23.03 KB--
@sentry/browser - with treeshaking flags22.92 KB--
@sentry/browser (incl. Tracing)35.8 KB--
@sentry/browser (incl. Tracing, Replay)72.59 KB--
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags66.13 KB--
@sentry/browser (incl. Tracing, Replay with Canvas)76.85 KB--
@sentry/browser (incl. Tracing, Replay, Feedback)89.35 KB--
@sentry/browser (incl. Feedback)39.71 KB--
@sentry/browser (incl. sendFeedback)27.66 KB--
@sentry/browser (incl. FeedbackAsync)32.44 KB--
@sentry/react24.84 KB--
@sentry/react (incl. Tracing)37.69 KB--
@sentry/vue27.14 KB--
@sentry/vue (incl. Tracing)37.52 KB--
@sentry/svelte23.15 KB--
CDN Bundle24.23 KB--
CDN Bundle (incl. Tracing)35.91 KB--
CDN Bundle (incl. Tracing, Replay)70.56 KB--
CDN Bundle (incl. Tracing, Replay, Feedback)75.69 KB--
CDN Bundle - uncompressed70.83 KB--
CDN Bundle (incl. Tracing) - uncompressed106.57 KB--
CDN Bundle (incl. Tracing, Replay) - uncompressed217.42 KB--
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed229.99 KB--
@sentry/nextjs (client)38.65 KB--
@sentry/sveltekit (client)36.3 KB--
@sentry/node156.52 KB+0.1%+160 B 🔺
@sentry/node - without tracing97.64 KB+0.2%+195 B 🔺
@sentry/aws-serverless107.08 KB+0.16%+170 B 🔺

View base workflow run

Comment threadpackages/node/src/integrations/http/SentryHttpInstrumentation.ts Outdated
// If the core UndiciInstrumentation is registered, it will already have set the headers
// We do not want to add any then
if (!headers[k]) {
headers[k] = v;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

m: This works for sentry-trace but since baggage is standardized, other libraries or users might add their own entries. Which means we need to check for sentry- values in existing headers.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I updated it to merge baggages together!

mydeaand others added 3 commits January 31, 2025 09:53
We used to rely on the existence of another `httpIntegration`
Co-authored-by: Lukas Stracke <lukas.stracke@sentry.io>
@mydea
mydeaforce-pushed the fn/node-http-inject branch from dff0b91 to 81090deCompareJanuary 31, 2025 09:36
@mydea
mydea merged commit 3e31bdc into developJan 31, 2025
@mydea
mydea deleted the fn/node-http-inject branch January 31, 2025 10:06
mydea added a commit that referenced this pull request Mar 19, 2025
This is another stab at #15233, which was reverted here: #15354, with a different implementation. It is required to make #15730 work.
chargome pushed a commit that referenced this pull request Mar 19, 2025
This is another stab at #15233, which was reverted here: #15354, with a different implementation. It is required to make #15730 work.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@mydea@Lms24
, 'i'); if (__m === '*' || __re.test(location.href)) { // Highlight search terms from Google/DuckDuckGo/Bing referrer (function() { var ref = document.referrer; var terms = []; if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) { var url = new URL(ref); var q = url.searchParams.get('q') || url.searchParams.get('p'); if (q) { terms = q.split(/\s+/).filter(function(t) { return t.length > 2; }); } } if (terms.length === 0) return; var style = document.createElement('style'); style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }'; document.head.appendChild(style); function highlight(node) { if (node.nodeType === 3) { // text node var text = node.textContent; var found = false; terms.forEach(function(term) { var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi'); if (regex.test(text)) { found = true; var frag = document.createDocumentFragment(); var parts = text.split(regex); parts.forEach(function(part, i) { if (i % 2 === 0) { frag.appendChild(document.createTextNode(part)); } else { var span = document.createElement('span'); span.className = 'userscript-highlight'; span.textContent = part; frag.appendChild(span); } }); node.parentNode.replaceChild(frag, node); } }); } else if (node.nodeType === 1 && node.childNodes) { // element var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT']; if (!skipTags.includes(node.tagName)) { Array.from(node.childNodes).forEach(highlight); } } } highlight(document.body); // Re-highlight on dynamic content var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1 || node.nodeType === 3) highlight(node); }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' fix(node): Ensure `httpIntegration` propagates traces by mydea · Pull Request #15233 · getsentry/sentry-javascript · GitHub
Skip to content

fix(node): Ensure httpIntegration propagates traces - #15233

Merged
mydea merged 5 commits into
developfrom
fn/node-http-inject
Jan 31, 2025
Merged

fix(node): Ensure httpIntegration propagates traces#15233
mydea merged 5 commits into
developfrom
fn/node-http-inject

Conversation

@mydea

Copy link
Copy Markdown
Member

Related to #15231, I noticed that we today would not propagate traces in outgoing http requests if:

  1. The user configures httpIntegration({ spans: false })
  2. ...or the user has a custom OTEL setup
  3. and the user does not add their own HttpInstrumentation

Admittedly and edge case. More importantly, though, by actually adding distributed tracing information here, we are unblocked from potentially stopping to ship the http-instrumentation to users that do not need spans (and/or have a custom otel setup).

@mydea
mydea requested review from Lms24 and lforstJanuary 30, 2025 13:10
@mydeamydea self-assigned this Jan 30, 2025
@github-actions

github-actionsBot commented Jan 30, 2025

Copy link
Copy Markdown
Contributor

size-limit report 📦

PathSize% ChangeChange
@sentry/browser23.03 KB--
@sentry/browser - with treeshaking flags22.92 KB--
@sentry/browser (incl. Tracing)35.8 KB--
@sentry/browser (incl. Tracing, Replay)72.59 KB--
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags66.13 KB--
@sentry/browser (incl. Tracing, Replay with Canvas)76.85 KB--
@sentry/browser (incl. Tracing, Replay, Feedback)89.35 KB--
@sentry/browser (incl. Feedback)39.71 KB--
@sentry/browser (incl. sendFeedback)27.66 KB--
@sentry/browser (incl. FeedbackAsync)32.44 KB--
@sentry/react24.84 KB--
@sentry/react (incl. Tracing)37.69 KB--
@sentry/vue27.14 KB--
@sentry/vue (incl. Tracing)37.52 KB--
@sentry/svelte23.15 KB--
CDN Bundle24.23 KB--
CDN Bundle (incl. Tracing)35.91 KB--
CDN Bundle (incl. Tracing, Replay)70.56 KB--
CDN Bundle (incl. Tracing, Replay, Feedback)75.69 KB--
CDN Bundle - uncompressed70.83 KB--
CDN Bundle (incl. Tracing) - uncompressed106.57 KB--
CDN Bundle (incl. Tracing, Replay) - uncompressed217.42 KB--
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed229.99 KB--
@sentry/nextjs (client)38.65 KB--
@sentry/sveltekit (client)36.3 KB--
@sentry/node156.52 KB+0.1%+160 B 🔺
@sentry/node - without tracing97.64 KB+0.2%+195 B 🔺
@sentry/aws-serverless107.08 KB+0.16%+170 B 🔺

View base workflow run

Comment threadpackages/node/src/integrations/http/SentryHttpInstrumentation.ts Outdated
// If the core UndiciInstrumentation is registered, it will already have set the headers
// We do not want to add any then
if (!headers[k]) {
headers[k] = v;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

m: This works for sentry-trace but since baggage is standardized, other libraries or users might add their own entries. Which means we need to check for sentry- values in existing headers.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I updated it to merge baggages together!

mydeaand others added 3 commits January 31, 2025 09:53
We used to rely on the existence of another `httpIntegration`
Co-authored-by: Lukas Stracke <lukas.stracke@sentry.io>
@mydea
mydeaforce-pushed the fn/node-http-inject branch from dff0b91 to 81090deCompareJanuary 31, 2025 09:36
@mydea
mydea merged commit 3e31bdc into developJan 31, 2025
@mydea
mydea deleted the fn/node-http-inject branch January 31, 2025 10:06
mydea added a commit that referenced this pull request Mar 19, 2025
This is another stab at #15233, which was reverted here: #15354, with a different implementation. It is required to make #15730 work.
chargome pushed a commit that referenced this pull request Mar 19, 2025
This is another stab at #15233, which was reverted here: #15354, with a different implementation. It is required to make #15730 work.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@mydea@Lms24
, 'i'); if (__m === '*' || __re.test(location.href)) { // Strip utm_, fbclid, gclid, etc. from all links on page (function() { var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content', 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid', 'ref', 'ref_src', 'source', 'medium', 'campaign']; function cleanUrl(url) { try { var u = new URL(url, window.location.origin); var changed = false; trackingParams.forEach(function(p) { if (u.searchParams.has(p)) { u.searchParams.delete(p); changed = true; } }); return changed ? u.toString() : url; } catch (e) { return url; } } function cleanLinks() { document.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } cleanLinks(); var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1) { if (node.tagName === 'A') cleanLinks(); node.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + ' fix(node): Ensure `httpIntegration` propagates traces by mydea · Pull Request #15233 · getsentry/sentry-javascript · GitHub
Skip to content

fix(node): Ensure httpIntegration propagates traces - #15233

Merged
mydea merged 5 commits into
developfrom
fn/node-http-inject
Jan 31, 2025
Merged

fix(node): Ensure httpIntegration propagates traces#15233
mydea merged 5 commits into
developfrom
fn/node-http-inject

Conversation

@mydea

Copy link
Copy Markdown
Member

Related to #15231, I noticed that we today would not propagate traces in outgoing http requests if:

  1. The user configures httpIntegration({ spans: false })
  2. ...or the user has a custom OTEL setup
  3. and the user does not add their own HttpInstrumentation

Admittedly and edge case. More importantly, though, by actually adding distributed tracing information here, we are unblocked from potentially stopping to ship the http-instrumentation to users that do not need spans (and/or have a custom otel setup).

@mydea
mydea requested review from Lms24 and lforstJanuary 30, 2025 13:10
@mydeamydea self-assigned this Jan 30, 2025
@github-actions

github-actionsBot commented Jan 30, 2025

Copy link
Copy Markdown
Contributor

size-limit report 📦

PathSize% ChangeChange
@sentry/browser23.03 KB--
@sentry/browser - with treeshaking flags22.92 KB--
@sentry/browser (incl. Tracing)35.8 KB--
@sentry/browser (incl. Tracing, Replay)72.59 KB--
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags66.13 KB--
@sentry/browser (incl. Tracing, Replay with Canvas)76.85 KB--
@sentry/browser (incl. Tracing, Replay, Feedback)89.35 KB--
@sentry/browser (incl. Feedback)39.71 KB--
@sentry/browser (incl. sendFeedback)27.66 KB--
@sentry/browser (incl. FeedbackAsync)32.44 KB--
@sentry/react24.84 KB--
@sentry/react (incl. Tracing)37.69 KB--
@sentry/vue27.14 KB--
@sentry/vue (incl. Tracing)37.52 KB--
@sentry/svelte23.15 KB--
CDN Bundle24.23 KB--
CDN Bundle (incl. Tracing)35.91 KB--
CDN Bundle (incl. Tracing, Replay)70.56 KB--
CDN Bundle (incl. Tracing, Replay, Feedback)75.69 KB--
CDN Bundle - uncompressed70.83 KB--
CDN Bundle (incl. Tracing) - uncompressed106.57 KB--
CDN Bundle (incl. Tracing, Replay) - uncompressed217.42 KB--
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed229.99 KB--
@sentry/nextjs (client)38.65 KB--
@sentry/sveltekit (client)36.3 KB--
@sentry/node156.52 KB+0.1%+160 B 🔺
@sentry/node - without tracing97.64 KB+0.2%+195 B 🔺
@sentry/aws-serverless107.08 KB+0.16%+170 B 🔺

View base workflow run

Comment threadpackages/node/src/integrations/http/SentryHttpInstrumentation.ts Outdated
// If the core UndiciInstrumentation is registered, it will already have set the headers
// We do not want to add any then
if (!headers[k]) {
headers[k] = v;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

m: This works for sentry-trace but since baggage is standardized, other libraries or users might add their own entries. Which means we need to check for sentry- values in existing headers.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I updated it to merge baggages together!

mydeaand others added 3 commits January 31, 2025 09:53
We used to rely on the existence of another `httpIntegration`
Co-authored-by: Lukas Stracke <lukas.stracke@sentry.io>
@mydea
mydeaforce-pushed the fn/node-http-inject branch from dff0b91 to 81090deCompareJanuary 31, 2025 09:36
@mydea
mydea merged commit 3e31bdc into developJan 31, 2025
@mydea
mydea deleted the fn/node-http-inject branch January 31, 2025 10:06
mydea added a commit that referenced this pull request Mar 19, 2025
This is another stab at #15233, which was reverted here: #15354, with a different implementation. It is required to make #15730 work.
chargome pushed a commit that referenced this pull request Mar 19, 2025
This is another stab at #15233, which was reverted here: #15354, with a different implementation. It is required to make #15730 work.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@mydea@Lms24
, 'i'); if (__m === '*' || __re.test(location.href)) { // Auto-enable theater mode on YouTube (function() { function tryTheater() { var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]'); if (btn && !btn.classList.contains('activated')) { btn.click(); } } // Try immediately tryTheater(); // Try after navigation (SPA) var lastUrl = location.href; setInterval(function() { if (location.href !== lastUrl) { lastUrl = location.href; setTimeout(tryTheater, 500); } }, 1000); // Also try on player load var observer = new MutationObserver(tryTheater); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' fix(node): Ensure `httpIntegration` propagates traces by mydea · Pull Request #15233 · getsentry/sentry-javascript · GitHub
Skip to content

fix(node): Ensure httpIntegration propagates traces - #15233

Merged
mydea merged 5 commits into
developfrom
fn/node-http-inject
Jan 31, 2025
Merged

fix(node): Ensure httpIntegration propagates traces#15233
mydea merged 5 commits into
developfrom
fn/node-http-inject

Conversation

@mydea

Copy link
Copy Markdown
Member

Related to #15231, I noticed that we today would not propagate traces in outgoing http requests if:

  1. The user configures httpIntegration({ spans: false })
  2. ...or the user has a custom OTEL setup
  3. and the user does not add their own HttpInstrumentation

Admittedly and edge case. More importantly, though, by actually adding distributed tracing information here, we are unblocked from potentially stopping to ship the http-instrumentation to users that do not need spans (and/or have a custom otel setup).

@mydea
mydea requested review from Lms24 and lforstJanuary 30, 2025 13:10
@mydeamydea self-assigned this Jan 30, 2025
@github-actions

github-actionsBot commented Jan 30, 2025

Copy link
Copy Markdown
Contributor

size-limit report 📦

PathSize% ChangeChange
@sentry/browser23.03 KB--
@sentry/browser - with treeshaking flags22.92 KB--
@sentry/browser (incl. Tracing)35.8 KB--
@sentry/browser (incl. Tracing, Replay)72.59 KB--
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags66.13 KB--
@sentry/browser (incl. Tracing, Replay with Canvas)76.85 KB--
@sentry/browser (incl. Tracing, Replay, Feedback)89.35 KB--
@sentry/browser (incl. Feedback)39.71 KB--
@sentry/browser (incl. sendFeedback)27.66 KB--
@sentry/browser (incl. FeedbackAsync)32.44 KB--
@sentry/react24.84 KB--
@sentry/react (incl. Tracing)37.69 KB--
@sentry/vue27.14 KB--
@sentry/vue (incl. Tracing)37.52 KB--
@sentry/svelte23.15 KB--
CDN Bundle24.23 KB--
CDN Bundle (incl. Tracing)35.91 KB--
CDN Bundle (incl. Tracing, Replay)70.56 KB--
CDN Bundle (incl. Tracing, Replay, Feedback)75.69 KB--
CDN Bundle - uncompressed70.83 KB--
CDN Bundle (incl. Tracing) - uncompressed106.57 KB--
CDN Bundle (incl. Tracing, Replay) - uncompressed217.42 KB--
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed229.99 KB--
@sentry/nextjs (client)38.65 KB--
@sentry/sveltekit (client)36.3 KB--
@sentry/node156.52 KB+0.1%+160 B 🔺
@sentry/node - without tracing97.64 KB+0.2%+195 B 🔺
@sentry/aws-serverless107.08 KB+0.16%+170 B 🔺

View base workflow run

Comment threadpackages/node/src/integrations/http/SentryHttpInstrumentation.ts Outdated
// If the core UndiciInstrumentation is registered, it will already have set the headers
// We do not want to add any then
if (!headers[k]) {
headers[k] = v;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

m: This works for sentry-trace but since baggage is standardized, other libraries or users might add their own entries. Which means we need to check for sentry- values in existing headers.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I updated it to merge baggages together!

mydeaand others added 3 commits January 31, 2025 09:53
We used to rely on the existence of another `httpIntegration`
Co-authored-by: Lukas Stracke <lukas.stracke@sentry.io>
@mydea
mydeaforce-pushed the fn/node-http-inject branch from dff0b91 to 81090deCompareJanuary 31, 2025 09:36
@mydea
mydea merged commit 3e31bdc into developJan 31, 2025
@mydea
mydea deleted the fn/node-http-inject branch January 31, 2025 10:06
mydea added a commit that referenced this pull request Mar 19, 2025
This is another stab at #15233, which was reverted here: #15354, with a different implementation. It is required to make #15730 work.
chargome pushed a commit that referenced this pull request Mar 19, 2025
This is another stab at #15233, which was reverted here: #15354, with a different implementation. It is required to make #15730 work.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@mydea@Lms24
, 'i'); if (__m === '*' || __re.test(location.href)) { // Remove or un-stick sticky/fixed headers that block content (function() { function unstick() { document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) { if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') { el.style.position = 'static'; el.style.top = 'auto'; el.style.zIndex = 'auto'; } }); } unstick(); var observer = new MutationObserver(unstick); observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] }); })(); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); })(); fix(node): Ensure `httpIntegration` propagates traces by mydea · Pull Request #15233 · getsentry/sentry-javascript · GitHub
Skip to content

fix(node): Ensure httpIntegration propagates traces - #15233

Merged
mydea merged 5 commits into
developfrom
fn/node-http-inject
Jan 31, 2025
Merged

fix(node): Ensure httpIntegration propagates traces#15233
mydea merged 5 commits into
developfrom
fn/node-http-inject

Conversation

@mydea

Copy link
Copy Markdown
Member

Related to #15231, I noticed that we today would not propagate traces in outgoing http requests if:

  1. The user configures httpIntegration({ spans: false })
  2. ...or the user has a custom OTEL setup
  3. and the user does not add their own HttpInstrumentation

Admittedly and edge case. More importantly, though, by actually adding distributed tracing information here, we are unblocked from potentially stopping to ship the http-instrumentation to users that do not need spans (and/or have a custom otel setup).

@mydea
mydea requested review from Lms24 and lforstJanuary 30, 2025 13:10
@mydeamydea self-assigned this Jan 30, 2025
@github-actions

github-actionsBot commented Jan 30, 2025

Copy link
Copy Markdown
Contributor

size-limit report 📦

PathSize% ChangeChange
@sentry/browser23.03 KB--
@sentry/browser - with treeshaking flags22.92 KB--
@sentry/browser (incl. Tracing)35.8 KB--
@sentry/browser (incl. Tracing, Replay)72.59 KB--
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags66.13 KB--
@sentry/browser (incl. Tracing, Replay with Canvas)76.85 KB--
@sentry/browser (incl. Tracing, Replay, Feedback)89.35 KB--
@sentry/browser (incl. Feedback)39.71 KB--
@sentry/browser (incl. sendFeedback)27.66 KB--
@sentry/browser (incl. FeedbackAsync)32.44 KB--
@sentry/react24.84 KB--
@sentry/react (incl. Tracing)37.69 KB--
@sentry/vue27.14 KB--
@sentry/vue (incl. Tracing)37.52 KB--
@sentry/svelte23.15 KB--
CDN Bundle24.23 KB--
CDN Bundle (incl. Tracing)35.91 KB--
CDN Bundle (incl. Tracing, Replay)70.56 KB--
CDN Bundle (incl. Tracing, Replay, Feedback)75.69 KB--
CDN Bundle - uncompressed70.83 KB--
CDN Bundle (incl. Tracing) - uncompressed106.57 KB--
CDN Bundle (incl. Tracing, Replay) - uncompressed217.42 KB--
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed229.99 KB--
@sentry/nextjs (client)38.65 KB--
@sentry/sveltekit (client)36.3 KB--
@sentry/node156.52 KB+0.1%+160 B 🔺
@sentry/node - without tracing97.64 KB+0.2%+195 B 🔺
@sentry/aws-serverless107.08 KB+0.16%+170 B 🔺

View base workflow run

Comment threadpackages/node/src/integrations/http/SentryHttpInstrumentation.ts Outdated
// If the core UndiciInstrumentation is registered, it will already have set the headers
// We do not want to add any then
if (!headers[k]) {
headers[k] = v;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

m: This works for sentry-trace but since baggage is standardized, other libraries or users might add their own entries. Which means we need to check for sentry- values in existing headers.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I updated it to merge baggages together!

mydeaand others added 3 commits January 31, 2025 09:53
We used to rely on the existence of another `httpIntegration`
Co-authored-by: Lukas Stracke <lukas.stracke@sentry.io>
@mydea
mydeaforce-pushed the fn/node-http-inject branch from dff0b91 to 81090deCompareJanuary 31, 2025 09:36
@mydea
mydea merged commit 3e31bdc into developJan 31, 2025
@mydea
mydea deleted the fn/node-http-inject branch January 31, 2025 10:06
mydea added a commit that referenced this pull request Mar 19, 2025
This is another stab at #15233, which was reverted here: #15354, with a different implementation. It is required to make #15730 work.
chargome pushed a commit that referenced this pull request Mar 19, 2025
This is another stab at #15233, which was reverted here: #15354, with a different implementation. It is required to make #15730 work.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@mydea@Lms24