Skip to content

ref(node): Avoid double wrapping http module for vercel on Node - #16178

Closed
mydea wants to merge 3 commits into
developfrom
fn/patch-response-channel
Closed

ref(node): Avoid double wrapping http module for vercel on Node#16178
mydea wants to merge 3 commits into
developfrom
fn/patch-response-channel

Conversation

@mydea

@mydeamydea commented Apr 30, 2025•
edited
Loading

Copy link
Copy Markdown
Member

This PR is a follow up to #16177

There, we found out that double-wrapping (with stealthWrap) the http module seems to break in ESM mode, for whatever reason - us double-wrapping this leads to the OTEL-instrumentation creating spans twice, for whatever reason.

We removed this for the core sentry-http instrumentation, but we still do it for vercel-edge here. For consistency, this cleans this up so we should be safe.

As an added benefit, it again leads to all the sentry-specific instrumentation code to be in one place, and to us not having to care about instrumentation order, hopefully, if this actually works.

@mydea
mydea requested a review from lforstApril 30, 2025 14:25
@mydeamydea self-assigned this Apr 30, 2025
@mydea
mydeaforce-pushed the fn/patch-response-channel branch from 67ee12d to 512e319CompareApril 30, 2025 14:25
@github-actions

github-actionsBot commented Apr 30, 2025•
edited
Loading

Copy link
Copy Markdown
Contributor

size-limit report 📦

PathSize% ChangeChange
@sentry/browser23.34 KB--
@sentry/browser - with treeshaking flags23.16 KB--
@sentry/browser (incl. Tracing)37.22 KB--
@sentry/browser (incl. Tracing, Replay)74.45 KB--
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags68.32 KB--
@sentry/browser (incl. Tracing, Replay with Canvas)79.11 KB--
@sentry/browser (incl. Tracing, Replay, Feedback)90.93 KB--
@sentry/browser (incl. Feedback)39.72 KB--
@sentry/browser (incl. sendFeedback)27.96 KB--
@sentry/browser (incl. FeedbackAsync)32.72 KB--
@sentry/react25.14 KB--
@sentry/react (incl. Tracing)39.22 KB--
@sentry/vue27.6 KB--
@sentry/vue (incl. Tracing)38.99 KB--
@sentry/svelte23.36 KB--
CDN Bundle24.55 KB--
CDN Bundle (incl. Tracing)37.3 KB--
CDN Bundle (incl. Tracing, Replay)72.34 KB--
CDN Bundle (incl. Tracing, Replay, Feedback)77.62 KB--
CDN Bundle - uncompressed71.62 KB--
CDN Bundle (incl. Tracing) - uncompressed110.34 KB--
CDN Bundle (incl. Tracing, Replay) - uncompressed221.63 KB--
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed234.15 KB--
@sentry/nextjs (client)40.83 KB--
@sentry/sveltekit (client)37.72 KB--
@sentry/node144.26 KB-0.11%-153 B 🔽
@sentry/node - without tracing96.14 KB-0.16%-150 B 🔽
@sentry/aws-serverless120.49 KB-0.14%-167 B 🔽

View base workflow run

@mydea
mydeaforce-pushed the fn/http-instrumentation-diagnostics-channel branch from 72ddee3 to 596dceeCompareMay 2, 2025 11:14
@lforst

lforst commented May 5, 2025

Copy link
Copy Markdown
Contributor

Would you mind explaining in the PR description why we do this and what this PR does? I can see us going back to this in 3 months and asking ourselves wtf

@mydea
mydeaforce-pushed the fn/http-instrumentation-diagnostics-channel branch from aff551f to 0cd1834CompareMay 5, 2025 08:27
Base automatically changed from fn/http-instrumentation-diagnostics-channel to developMay 5, 2025 12:24
@mydea
mydeaforce-pushed the fn/patch-response-channel branch from 512e319 to 96423b2CompareMay 5, 2025 12:32
@mydeamydea changed the title ref(node): Avoid double wrapping http module for vercel-edgeref(node): Avoid double wrapping http module for vercel on NodeMay 5, 2025
@lforst

lforst commented May 6, 2025

Copy link
Copy Markdown
Contributor

To recapitulate, what needs to happen for the logic to still work is to register the res.on('close') listener after OTEL registers its own res.on('close') listener for ending a span. Reminder that res.on('close') is only fired after res.end() has been called.

Findings so far, because the current approach doesn't work:

  • Putting the patching logic into http.server.response.created does not work because it will run before OTEL applies its .on('close') handler.
  • Putting the patching logic into http.server.response.finish for some reason does not work because the diagnostic handler weirdly runs afterres.on('close') is fired (even though the code in Node.js would indicate otherwise???).

tldr we have no good place to register the res.on('close') handler...

@mydeamydea mentioned this pull request May 6, 2025
Merged
mydea added a commit that referenced this pull request May 6, 2025
@mydea
fix(node): Fix vercel flushing logic & add test for it (#16208)
b74f300
As pre-work for
#16178, actually add
a test for this (kind of). This showed that there was actually a
fundamental flaw here, as we looked as the `req` not the `res`, oops.
@mydea
mydeaforce-pushed the fn/patch-response-channel branch from 96423b2 to 8e6d77eCompareMay 7, 2025 08:43
@mydea
mydea marked this pull request as ready for review May 7, 2025 09:17
@mydea

mydea commented May 7, 2025

Copy link
Copy Markdown
MemberAuthor

OK we can't get this to work on Vercel on prod, so we'll just remove this for now until we get a better handle on this 😬 cc @lforst

@lforst

lforst commented May 7, 2025

Copy link
Copy Markdown
Contributor

I am in support of removing

@mydeamydea mentioned this pull request May 7, 2025
Merged
@mydea

mydea commented May 7, 2025

Copy link
Copy Markdown
MemberAuthor

Replaced by #16217

@mydeamydea closed this May 7, 2025
@mydea
mydea deleted the fn/patch-response-channel branch May 7, 2025 13:35
mydea added a commit that referenced this pull request May 7, 2025
@mydea
ref(node): Remove vercel flushing code that does nothing (#16217)
edabe21
We could never get this to apply properly on vercel in production, so
we're removing this for now and may revisit this later.
Replaces #16178
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@lforstlforstlforst approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@mydeamydea

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@mydea@lforst
, '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" + '
ref(node): Avoid double wrapping `http` module for vercel on Node by mydea · Pull Request #16178 · getsentry/sentry-javascript · GitHub
Skip to content
ref(node): Avoid double wrapping http module for vercel on Node - #16178
Closed
mydea wants to merge 3 commits into
developfrom 
fn/patch-response-channel
Closed
ref(node): Avoid double wrapping http module for vercel on Node#16178
mydea wants to merge 3 commits into
developfrom 
fn/patch-response-channel
Conversation
@mydea
@mydeamydea
commented
Apr 30, 2025•

edited

Loading

Copy link


Copy Markdown

Member
This PR is a follow up to #16177
There, we found out that double-wrapping (with stealthWrap) the http module seems to break in ESM mode, for whatever reason - us double-wrapping this leads to the OTEL-instrumentation creating spans twice, for whatever reason.
We removed this for the core sentry-http instrumentation, but we still do it for vercel-edge here. For consistency, this cleans this up so we should be safe.
As an added benefit, it again leads to all the sentry-specific instrumentation code to be in one place, and to us not having to care about instrumentation order, hopefully, if this actually works.
@mydea
mydea
requested a review
from lforstApril 30, 2025 14:25
@mydeamydea
self-assigned this
Apr 30, 2025
@mydea
mydeaforce-pushed
the
fn/patch-response-channel
branch
from
67ee12d to
512e319CompareApril 30, 2025 14:25
@github-actions
github-actionsBot
commented
Apr 30, 2025•

edited

Loading

Copy link


Copy Markdown

Contributor
size-limit report 📦
PathSize% ChangeChange
@sentry/browser23.34 KB--
@sentry/browser - with treeshaking flags23.16 KB--
@sentry/browser (incl. Tracing)37.22 KB--
@sentry/browser (incl. Tracing, Replay)74.45 KB--
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags68.32 KB--
@sentry/browser (incl. Tracing, Replay with Canvas)79.11 KB--
@sentry/browser (incl. Tracing, Replay, Feedback)90.93 KB--
@sentry/browser (incl. Feedback)39.72 KB--
@sentry/browser (incl. sendFeedback)27.96 KB--
@sentry/browser (incl. FeedbackAsync)32.72 KB--
@sentry/react25.14 KB--
@sentry/react (incl. Tracing)39.22 KB--
@sentry/vue27.6 KB--
@sentry/vue (incl. Tracing)38.99 KB--
@sentry/svelte23.36 KB--
CDN Bundle24.55 KB--
CDN Bundle (incl. Tracing)37.3 KB--
CDN Bundle (incl. Tracing, Replay)72.34 KB--
CDN Bundle (incl. Tracing, Replay, Feedback)77.62 KB--
CDN Bundle - uncompressed71.62 KB--
CDN Bundle (incl. Tracing) - uncompressed110.34 KB--
CDN Bundle (incl. Tracing, Replay) - uncompressed221.63 KB--
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed234.15 KB--
@sentry/nextjs (client)40.83 KB--
@sentry/sveltekit (client)37.72 KB--
@sentry/node144.26 KB-0.11%-153 B 🔽
@sentry/node - without tracing96.14 KB-0.16%-150 B 🔽
@sentry/aws-serverless120.49 KB-0.14%-167 B 🔽
View base workflow run
@mydea
mydeaforce-pushed
the
fn/http-instrumentation-diagnostics-channel
branch
from
72ddee3 to
596dceeCompareMay 2, 2025 11:14
@lforst
lforst
commented
May 5, 2025

Copy link


Copy Markdown

Contributor
Would you mind explaining in the PR description why we do this and what this PR does? I can see us going back to this in 3 months and asking ourselves wtf
@mydea
mydeaforce-pushed
the
fn/http-instrumentation-diagnostics-channel
branch
from
aff551f to
0cd1834CompareMay 5, 2025 08:27

Base automatically changed from
fn/http-instrumentation-diagnostics-channel to
developMay 5, 2025 12:24
@mydea
mydeaforce-pushed
the
fn/patch-response-channel
branch
from
512e319 to
96423b2CompareMay 5, 2025 12:32
@mydeamydea
changed the title
ref(node): Avoid double wrapping http module for vercel-edgeref(node): Avoid double wrapping http module for vercel on NodeMay 5, 2025
@lforst
lforst
commented
May 6, 2025

Copy link


Copy Markdown

Contributor
To recapitulate, what needs to happen for the logic to still work is to register the res.on('close') listener after OTEL registers its own res.on('close') listener for ending a span. Reminder that res.on('close') is only fired after res.end() has been called.
Findings so far, because the current approach doesn't work:
  • Putting the patching logic into http.server.response.created does not work because it will run before OTEL applies its .on('close') handler.
  • Putting the patching logic into http.server.response.finish for some reason does not work because the diagnostic handler weirdly runs afterres.on('close') is fired (even though the code in Node.js would indicate otherwise???).
tldr we have no good place to register the res.on('close') handler...
@mydeamydea
mentioned this pull request
May 6, 2025
 Merged

mydea added a commit
that referenced
this pull request
May 6, 2025
@mydea
fix(node): Fix vercel flushing logic & add test for it (#16208)
b74f300
As pre-work for
#16178, actually add
a test for this (kind of). This showed that there was actually a
fundamental flaw here, as we looked as the `req` not the `res`, oops.
@mydea
mydeaforce-pushed
the
fn/patch-response-channel
branch
from
96423b2 to
8e6d77eCompareMay 7, 2025 08:43
@mydea
mydea
marked this pull request as ready for review
May 7, 2025 09:17
@mydea
mydea
commented
May 7, 2025

Copy link


Copy Markdown

MemberAuthor
OK we can't get this to work on Vercel on prod, so we'll just remove this for now until we get a better handle on this 😬 cc @lforst
@lforst
lforst
commented
May 7, 2025

Copy link


Copy Markdown

Contributor
I am in support of removing
@mydeamydea
mentioned this pull request
May 7, 2025
 Merged

@mydea
mydea
commented
May 7, 2025

Copy link


Copy Markdown

MemberAuthor
Replaced by #16217
@mydeamydea
closed this
May 7, 2025
@mydea
mydea
deleted the

fn/patch-response-channel

branch
May 7, 2025 13:35
mydea added a commit
that referenced
this pull request
May 7, 2025
@mydea
ref(node): Remove vercel flushing code that does nothing (#16217)
edabe21
We could never get this to apply properly on vercel in production, so
we're removing this for now and may revisit this later.
Replaces #16178
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers


1 more reviewer

@lforstlforstlforst approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@mydeamydea

Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

2 participants

@mydea@lforst

, '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('^' + ".*" + '
ref(node): Avoid double wrapping `http` module for vercel on Node by mydea · Pull Request #16178 · getsentry/sentry-javascript · GitHub
Skip to content
ref(node): Avoid double wrapping http module for vercel on Node - #16178
Closed
mydea wants to merge 3 commits into
developfrom 
fn/patch-response-channel
Closed
ref(node): Avoid double wrapping http module for vercel on Node#16178
mydea wants to merge 3 commits into
developfrom 
fn/patch-response-channel
Conversation
@mydea
@mydeamydea
commented
Apr 30, 2025•

edited

Loading

Copy link


Copy Markdown

Member
This PR is a follow up to #16177
There, we found out that double-wrapping (with stealthWrap) the http module seems to break in ESM mode, for whatever reason - us double-wrapping this leads to the OTEL-instrumentation creating spans twice, for whatever reason.
We removed this for the core sentry-http instrumentation, but we still do it for vercel-edge here. For consistency, this cleans this up so we should be safe.
As an added benefit, it again leads to all the sentry-specific instrumentation code to be in one place, and to us not having to care about instrumentation order, hopefully, if this actually works.
@mydea
mydea
requested a review
from lforstApril 30, 2025 14:25
@mydeamydea
self-assigned this
Apr 30, 2025
@mydea
mydeaforce-pushed
the
fn/patch-response-channel
branch
from
67ee12d to
512e319CompareApril 30, 2025 14:25
@github-actions
github-actionsBot
commented
Apr 30, 2025•

edited

Loading

Copy link


Copy Markdown

Contributor
size-limit report 📦
PathSize% ChangeChange
@sentry/browser23.34 KB--
@sentry/browser - with treeshaking flags23.16 KB--
@sentry/browser (incl. Tracing)37.22 KB--
@sentry/browser (incl. Tracing, Replay)74.45 KB--
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags68.32 KB--
@sentry/browser (incl. Tracing, Replay with Canvas)79.11 KB--
@sentry/browser (incl. Tracing, Replay, Feedback)90.93 KB--
@sentry/browser (incl. Feedback)39.72 KB--
@sentry/browser (incl. sendFeedback)27.96 KB--
@sentry/browser (incl. FeedbackAsync)32.72 KB--
@sentry/react25.14 KB--
@sentry/react (incl. Tracing)39.22 KB--
@sentry/vue27.6 KB--
@sentry/vue (incl. Tracing)38.99 KB--
@sentry/svelte23.36 KB--
CDN Bundle24.55 KB--
CDN Bundle (incl. Tracing)37.3 KB--
CDN Bundle (incl. Tracing, Replay)72.34 KB--
CDN Bundle (incl. Tracing, Replay, Feedback)77.62 KB--
CDN Bundle - uncompressed71.62 KB--
CDN Bundle (incl. Tracing) - uncompressed110.34 KB--
CDN Bundle (incl. Tracing, Replay) - uncompressed221.63 KB--
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed234.15 KB--
@sentry/nextjs (client)40.83 KB--
@sentry/sveltekit (client)37.72 KB--
@sentry/node144.26 KB-0.11%-153 B 🔽
@sentry/node - without tracing96.14 KB-0.16%-150 B 🔽
@sentry/aws-serverless120.49 KB-0.14%-167 B 🔽
View base workflow run
@mydea
mydeaforce-pushed
the
fn/http-instrumentation-diagnostics-channel
branch
from
72ddee3 to
596dceeCompareMay 2, 2025 11:14
@lforst
lforst
commented
May 5, 2025

Copy link


Copy Markdown

Contributor
Would you mind explaining in the PR description why we do this and what this PR does? I can see us going back to this in 3 months and asking ourselves wtf
@mydea
mydeaforce-pushed
the
fn/http-instrumentation-diagnostics-channel
branch
from
aff551f to
0cd1834CompareMay 5, 2025 08:27

Base automatically changed from
fn/http-instrumentation-diagnostics-channel to
developMay 5, 2025 12:24
@mydea
mydeaforce-pushed
the
fn/patch-response-channel
branch
from
512e319 to
96423b2CompareMay 5, 2025 12:32
@mydeamydea
changed the title
ref(node): Avoid double wrapping http module for vercel-edgeref(node): Avoid double wrapping http module for vercel on NodeMay 5, 2025
@lforst
lforst
commented
May 6, 2025

Copy link


Copy Markdown

Contributor
To recapitulate, what needs to happen for the logic to still work is to register the res.on('close') listener after OTEL registers its own res.on('close') listener for ending a span. Reminder that res.on('close') is only fired after res.end() has been called.
Findings so far, because the current approach doesn't work:
  • Putting the patching logic into http.server.response.created does not work because it will run before OTEL applies its .on('close') handler.
  • Putting the patching logic into http.server.response.finish for some reason does not work because the diagnostic handler weirdly runs afterres.on('close') is fired (even though the code in Node.js would indicate otherwise???).
tldr we have no good place to register the res.on('close') handler...
@mydeamydea
mentioned this pull request
May 6, 2025
 Merged

mydea added a commit
that referenced
this pull request
May 6, 2025
@mydea
fix(node): Fix vercel flushing logic & add test for it (#16208)
b74f300
As pre-work for
#16178, actually add
a test for this (kind of). This showed that there was actually a
fundamental flaw here, as we looked as the `req` not the `res`, oops.
@mydea
mydeaforce-pushed
the
fn/patch-response-channel
branch
from
96423b2 to
8e6d77eCompareMay 7, 2025 08:43
@mydea
mydea
marked this pull request as ready for review
May 7, 2025 09:17
@mydea
mydea
commented
May 7, 2025

Copy link


Copy Markdown

MemberAuthor
OK we can't get this to work on Vercel on prod, so we'll just remove this for now until we get a better handle on this 😬 cc @lforst
@lforst
lforst
commented
May 7, 2025

Copy link


Copy Markdown

Contributor
I am in support of removing
@mydeamydea
mentioned this pull request
May 7, 2025
 Merged

@mydea
mydea
commented
May 7, 2025

Copy link


Copy Markdown

MemberAuthor
Replaced by #16217
@mydeamydea
closed this
May 7, 2025
@mydea
mydea
deleted the

fn/patch-response-channel

branch
May 7, 2025 13:35
mydea added a commit
that referenced
this pull request
May 7, 2025
@mydea
ref(node): Remove vercel flushing code that does nothing (#16217)
edabe21
We could never get this to apply properly on vercel in production, so
we're removing this for now and may revisit this later.
Replaces #16178
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers


1 more reviewer

@lforstlforstlforst approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@mydeamydea

Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

2 participants

@mydea@lforst

, '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('^' + ".*" + '
ref(node): Avoid double wrapping `http` module for vercel on Node by mydea · Pull Request #16178 · getsentry/sentry-javascript · GitHub
Skip to content
ref(node): Avoid double wrapping http module for vercel on Node - #16178
Closed
mydea wants to merge 3 commits into
developfrom 
fn/patch-response-channel
Closed
ref(node): Avoid double wrapping http module for vercel on Node#16178
mydea wants to merge 3 commits into
developfrom 
fn/patch-response-channel
Conversation
@mydea
@mydeamydea
commented
Apr 30, 2025•

edited

Loading

Copy link


Copy Markdown

Member
This PR is a follow up to #16177
There, we found out that double-wrapping (with stealthWrap) the http module seems to break in ESM mode, for whatever reason - us double-wrapping this leads to the OTEL-instrumentation creating spans twice, for whatever reason.
We removed this for the core sentry-http instrumentation, but we still do it for vercel-edge here. For consistency, this cleans this up so we should be safe.
As an added benefit, it again leads to all the sentry-specific instrumentation code to be in one place, and to us not having to care about instrumentation order, hopefully, if this actually works.
@mydea
mydea
requested a review
from lforstApril 30, 2025 14:25
@mydeamydea
self-assigned this
Apr 30, 2025
@mydea
mydeaforce-pushed
the
fn/patch-response-channel
branch
from
67ee12d to
512e319CompareApril 30, 2025 14:25
@github-actions
github-actionsBot
commented
Apr 30, 2025•

edited

Loading

Copy link


Copy Markdown

Contributor
size-limit report 📦
PathSize% ChangeChange
@sentry/browser23.34 KB--
@sentry/browser - with treeshaking flags23.16 KB--
@sentry/browser (incl. Tracing)37.22 KB--
@sentry/browser (incl. Tracing, Replay)74.45 KB--
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags68.32 KB--
@sentry/browser (incl. Tracing, Replay with Canvas)79.11 KB--
@sentry/browser (incl. Tracing, Replay, Feedback)90.93 KB--
@sentry/browser (incl. Feedback)39.72 KB--
@sentry/browser (incl. sendFeedback)27.96 KB--
@sentry/browser (incl. FeedbackAsync)32.72 KB--
@sentry/react25.14 KB--
@sentry/react (incl. Tracing)39.22 KB--
@sentry/vue27.6 KB--
@sentry/vue (incl. Tracing)38.99 KB--
@sentry/svelte23.36 KB--
CDN Bundle24.55 KB--
CDN Bundle (incl. Tracing)37.3 KB--
CDN Bundle (incl. Tracing, Replay)72.34 KB--
CDN Bundle (incl. Tracing, Replay, Feedback)77.62 KB--
CDN Bundle - uncompressed71.62 KB--
CDN Bundle (incl. Tracing) - uncompressed110.34 KB--
CDN Bundle (incl. Tracing, Replay) - uncompressed221.63 KB--
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed234.15 KB--
@sentry/nextjs (client)40.83 KB--
@sentry/sveltekit (client)37.72 KB--
@sentry/node144.26 KB-0.11%-153 B 🔽
@sentry/node - without tracing96.14 KB-0.16%-150 B 🔽
@sentry/aws-serverless120.49 KB-0.14%-167 B 🔽
View base workflow run
@mydea
mydeaforce-pushed
the
fn/http-instrumentation-diagnostics-channel
branch
from
72ddee3 to
596dceeCompareMay 2, 2025 11:14
@lforst
lforst
commented
May 5, 2025

Copy link


Copy Markdown

Contributor
Would you mind explaining in the PR description why we do this and what this PR does? I can see us going back to this in 3 months and asking ourselves wtf
@mydea
mydeaforce-pushed
the
fn/http-instrumentation-diagnostics-channel
branch
from
aff551f to
0cd1834CompareMay 5, 2025 08:27

Base automatically changed from
fn/http-instrumentation-diagnostics-channel to
developMay 5, 2025 12:24
@mydea
mydeaforce-pushed
the
fn/patch-response-channel
branch
from
512e319 to
96423b2CompareMay 5, 2025 12:32
@mydeamydea
changed the title
ref(node): Avoid double wrapping http module for vercel-edgeref(node): Avoid double wrapping http module for vercel on NodeMay 5, 2025
@lforst
lforst
commented
May 6, 2025

Copy link


Copy Markdown

Contributor
To recapitulate, what needs to happen for the logic to still work is to register the res.on('close') listener after OTEL registers its own res.on('close') listener for ending a span. Reminder that res.on('close') is only fired after res.end() has been called.
Findings so far, because the current approach doesn't work:
  • Putting the patching logic into http.server.response.created does not work because it will run before OTEL applies its .on('close') handler.
  • Putting the patching logic into http.server.response.finish for some reason does not work because the diagnostic handler weirdly runs afterres.on('close') is fired (even though the code in Node.js would indicate otherwise???).
tldr we have no good place to register the res.on('close') handler...
@mydeamydea
mentioned this pull request
May 6, 2025
 Merged

mydea added a commit
that referenced
this pull request
May 6, 2025
@mydea
fix(node): Fix vercel flushing logic & add test for it (#16208)
b74f300
As pre-work for
#16178, actually add
a test for this (kind of). This showed that there was actually a
fundamental flaw here, as we looked as the `req` not the `res`, oops.
@mydea
mydeaforce-pushed
the
fn/patch-response-channel
branch
from
96423b2 to
8e6d77eCompareMay 7, 2025 08:43
@mydea
mydea
marked this pull request as ready for review
May 7, 2025 09:17
@mydea
mydea
commented
May 7, 2025

Copy link


Copy Markdown

MemberAuthor
OK we can't get this to work on Vercel on prod, so we'll just remove this for now until we get a better handle on this 😬 cc @lforst
@lforst
lforst
commented
May 7, 2025

Copy link


Copy Markdown

Contributor
I am in support of removing
@mydeamydea
mentioned this pull request
May 7, 2025
 Merged

@mydea
mydea
commented
May 7, 2025

Copy link


Copy Markdown

MemberAuthor
Replaced by #16217
@mydeamydea
closed this
May 7, 2025
@mydea
mydea
deleted the

fn/patch-response-channel

branch
May 7, 2025 13:35
mydea added a commit
that referenced
this pull request
May 7, 2025
@mydea
ref(node): Remove vercel flushing code that does nothing (#16217)
edabe21
We could never get this to apply properly on vercel in production, so
we're removing this for now and may revisit this later.
Replaces #16178
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers


1 more reviewer

@lforstlforstlforst approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@mydeamydea

Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

2 participants

@mydea@lforst

, '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" + '
ref(node): Avoid double wrapping `http` module for vercel on Node by mydea · Pull Request #16178 · getsentry/sentry-javascript · GitHub
Skip to content
ref(node): Avoid double wrapping http module for vercel on Node - #16178
Closed
mydea wants to merge 3 commits into
developfrom 
fn/patch-response-channel
Closed
ref(node): Avoid double wrapping http module for vercel on Node#16178
mydea wants to merge 3 commits into
developfrom 
fn/patch-response-channel
Conversation
@mydea
@mydeamydea
commented
Apr 30, 2025•

edited

Loading

Copy link


Copy Markdown

Member
This PR is a follow up to #16177
There, we found out that double-wrapping (with stealthWrap) the http module seems to break in ESM mode, for whatever reason - us double-wrapping this leads to the OTEL-instrumentation creating spans twice, for whatever reason.
We removed this for the core sentry-http instrumentation, but we still do it for vercel-edge here. For consistency, this cleans this up so we should be safe.
As an added benefit, it again leads to all the sentry-specific instrumentation code to be in one place, and to us not having to care about instrumentation order, hopefully, if this actually works.
@mydea
mydea
requested a review
from lforstApril 30, 2025 14:25
@mydeamydea
self-assigned this
Apr 30, 2025
@mydea
mydeaforce-pushed
the
fn/patch-response-channel
branch
from
67ee12d to
512e319CompareApril 30, 2025 14:25
@github-actions
github-actionsBot
commented
Apr 30, 2025•

edited

Loading

Copy link


Copy Markdown

Contributor
size-limit report 📦
PathSize% ChangeChange
@sentry/browser23.34 KB--
@sentry/browser - with treeshaking flags23.16 KB--
@sentry/browser (incl. Tracing)37.22 KB--
@sentry/browser (incl. Tracing, Replay)74.45 KB--
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags68.32 KB--
@sentry/browser (incl. Tracing, Replay with Canvas)79.11 KB--
@sentry/browser (incl. Tracing, Replay, Feedback)90.93 KB--
@sentry/browser (incl. Feedback)39.72 KB--
@sentry/browser (incl. sendFeedback)27.96 KB--
@sentry/browser (incl. FeedbackAsync)32.72 KB--
@sentry/react25.14 KB--
@sentry/react (incl. Tracing)39.22 KB--
@sentry/vue27.6 KB--
@sentry/vue (incl. Tracing)38.99 KB--
@sentry/svelte23.36 KB--
CDN Bundle24.55 KB--
CDN Bundle (incl. Tracing)37.3 KB--
CDN Bundle (incl. Tracing, Replay)72.34 KB--
CDN Bundle (incl. Tracing, Replay, Feedback)77.62 KB--
CDN Bundle - uncompressed71.62 KB--
CDN Bundle (incl. Tracing) - uncompressed110.34 KB--
CDN Bundle (incl. Tracing, Replay) - uncompressed221.63 KB--
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed234.15 KB--
@sentry/nextjs (client)40.83 KB--
@sentry/sveltekit (client)37.72 KB--
@sentry/node144.26 KB-0.11%-153 B 🔽
@sentry/node - without tracing96.14 KB-0.16%-150 B 🔽
@sentry/aws-serverless120.49 KB-0.14%-167 B 🔽
View base workflow run
@mydea
mydeaforce-pushed
the
fn/http-instrumentation-diagnostics-channel
branch
from
72ddee3 to
596dceeCompareMay 2, 2025 11:14
@lforst
lforst
commented
May 5, 2025

Copy link


Copy Markdown

Contributor
Would you mind explaining in the PR description why we do this and what this PR does? I can see us going back to this in 3 months and asking ourselves wtf
@mydea
mydeaforce-pushed
the
fn/http-instrumentation-diagnostics-channel
branch
from
aff551f to
0cd1834CompareMay 5, 2025 08:27

Base automatically changed from
fn/http-instrumentation-diagnostics-channel to
developMay 5, 2025 12:24
@mydea
mydeaforce-pushed
the
fn/patch-response-channel
branch
from
512e319 to
96423b2CompareMay 5, 2025 12:32
@mydeamydea
changed the title
ref(node): Avoid double wrapping http module for vercel-edgeref(node): Avoid double wrapping http module for vercel on NodeMay 5, 2025
@lforst
lforst
commented
May 6, 2025

Copy link


Copy Markdown

Contributor
To recapitulate, what needs to happen for the logic to still work is to register the res.on('close') listener after OTEL registers its own res.on('close') listener for ending a span. Reminder that res.on('close') is only fired after res.end() has been called.
Findings so far, because the current approach doesn't work:
  • Putting the patching logic into http.server.response.created does not work because it will run before OTEL applies its .on('close') handler.
  • Putting the patching logic into http.server.response.finish for some reason does not work because the diagnostic handler weirdly runs afterres.on('close') is fired (even though the code in Node.js would indicate otherwise???).
tldr we have no good place to register the res.on('close') handler...
@mydeamydea
mentioned this pull request
May 6, 2025
 Merged

mydea added a commit
that referenced
this pull request
May 6, 2025
@mydea
fix(node): Fix vercel flushing logic & add test for it (#16208)
b74f300
As pre-work for
#16178, actually add
a test for this (kind of). This showed that there was actually a
fundamental flaw here, as we looked as the `req` not the `res`, oops.
@mydea
mydeaforce-pushed
the
fn/patch-response-channel
branch
from
96423b2 to
8e6d77eCompareMay 7, 2025 08:43
@mydea
mydea
marked this pull request as ready for review
May 7, 2025 09:17
@mydea
mydea
commented
May 7, 2025

Copy link


Copy Markdown

MemberAuthor
OK we can't get this to work on Vercel on prod, so we'll just remove this for now until we get a better handle on this 😬 cc @lforst
@lforst
lforst
commented
May 7, 2025

Copy link


Copy Markdown

Contributor
I am in support of removing
@mydeamydea
mentioned this pull request
May 7, 2025
 Merged

@mydea
mydea
commented
May 7, 2025

Copy link


Copy Markdown

MemberAuthor
Replaced by #16217
@mydeamydea
closed this
May 7, 2025
@mydea
mydea
deleted the

fn/patch-response-channel

branch
May 7, 2025 13:35
mydea added a commit
that referenced
this pull request
May 7, 2025
@mydea
ref(node): Remove vercel flushing code that does nothing (#16217)
edabe21
We could never get this to apply properly on vercel in production, so
we're removing this for now and may revisit this later.
Replaces #16178
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers


1 more reviewer

@lforstlforstlforst approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@mydeamydea

Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

2 participants

@mydea@lforst

, '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('^' + ".*" + '
ref(node): Avoid double wrapping `http` module for vercel on Node by mydea · Pull Request #16178 · getsentry/sentry-javascript · GitHub
Skip to content
ref(node): Avoid double wrapping http module for vercel on Node - #16178
Closed
mydea wants to merge 3 commits into
developfrom 
fn/patch-response-channel
Closed
ref(node): Avoid double wrapping http module for vercel on Node#16178
mydea wants to merge 3 commits into
developfrom 
fn/patch-response-channel
Conversation
@mydea
@mydeamydea
commented
Apr 30, 2025•

edited

Loading

Copy link


Copy Markdown

Member
This PR is a follow up to #16177
There, we found out that double-wrapping (with stealthWrap) the http module seems to break in ESM mode, for whatever reason - us double-wrapping this leads to the OTEL-instrumentation creating spans twice, for whatever reason.
We removed this for the core sentry-http instrumentation, but we still do it for vercel-edge here. For consistency, this cleans this up so we should be safe.
As an added benefit, it again leads to all the sentry-specific instrumentation code to be in one place, and to us not having to care about instrumentation order, hopefully, if this actually works.
@mydea
mydea
requested a review
from lforstApril 30, 2025 14:25
@mydeamydea
self-assigned this
Apr 30, 2025
@mydea
mydeaforce-pushed
the
fn/patch-response-channel
branch
from
67ee12d to
512e319CompareApril 30, 2025 14:25
@github-actions
github-actionsBot
commented
Apr 30, 2025•

edited

Loading

Copy link


Copy Markdown

Contributor
size-limit report 📦
PathSize% ChangeChange
@sentry/browser23.34 KB--
@sentry/browser - with treeshaking flags23.16 KB--
@sentry/browser (incl. Tracing)37.22 KB--
@sentry/browser (incl. Tracing, Replay)74.45 KB--
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags68.32 KB--
@sentry/browser (incl. Tracing, Replay with Canvas)79.11 KB--
@sentry/browser (incl. Tracing, Replay, Feedback)90.93 KB--
@sentry/browser (incl. Feedback)39.72 KB--
@sentry/browser (incl. sendFeedback)27.96 KB--
@sentry/browser (incl. FeedbackAsync)32.72 KB--
@sentry/react25.14 KB--
@sentry/react (incl. Tracing)39.22 KB--
@sentry/vue27.6 KB--
@sentry/vue (incl. Tracing)38.99 KB--
@sentry/svelte23.36 KB--
CDN Bundle24.55 KB--
CDN Bundle (incl. Tracing)37.3 KB--
CDN Bundle (incl. Tracing, Replay)72.34 KB--
CDN Bundle (incl. Tracing, Replay, Feedback)77.62 KB--
CDN Bundle - uncompressed71.62 KB--
CDN Bundle (incl. Tracing) - uncompressed110.34 KB--
CDN Bundle (incl. Tracing, Replay) - uncompressed221.63 KB--
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed234.15 KB--
@sentry/nextjs (client)40.83 KB--
@sentry/sveltekit (client)37.72 KB--
@sentry/node144.26 KB-0.11%-153 B 🔽
@sentry/node - without tracing96.14 KB-0.16%-150 B 🔽
@sentry/aws-serverless120.49 KB-0.14%-167 B 🔽
View base workflow run
@mydea
mydeaforce-pushed
the
fn/http-instrumentation-diagnostics-channel
branch
from
72ddee3 to
596dceeCompareMay 2, 2025 11:14
@lforst
lforst
commented
May 5, 2025

Copy link


Copy Markdown

Contributor
Would you mind explaining in the PR description why we do this and what this PR does? I can see us going back to this in 3 months and asking ourselves wtf
@mydea
mydeaforce-pushed
the
fn/http-instrumentation-diagnostics-channel
branch
from
aff551f to
0cd1834CompareMay 5, 2025 08:27

Base automatically changed from
fn/http-instrumentation-diagnostics-channel to
developMay 5, 2025 12:24
@mydea
mydeaforce-pushed
the
fn/patch-response-channel
branch
from
512e319 to
96423b2CompareMay 5, 2025 12:32
@mydeamydea
changed the title
ref(node): Avoid double wrapping http module for vercel-edgeref(node): Avoid double wrapping http module for vercel on NodeMay 5, 2025
@lforst
lforst
commented
May 6, 2025

Copy link


Copy Markdown

Contributor
To recapitulate, what needs to happen for the logic to still work is to register the res.on('close') listener after OTEL registers its own res.on('close') listener for ending a span. Reminder that res.on('close') is only fired after res.end() has been called.
Findings so far, because the current approach doesn't work:
  • Putting the patching logic into http.server.response.created does not work because it will run before OTEL applies its .on('close') handler.
  • Putting the patching logic into http.server.response.finish for some reason does not work because the diagnostic handler weirdly runs afterres.on('close') is fired (even though the code in Node.js would indicate otherwise???).
tldr we have no good place to register the res.on('close') handler...
@mydeamydea
mentioned this pull request
May 6, 2025
 Merged

mydea added a commit
that referenced
this pull request
May 6, 2025
@mydea
fix(node): Fix vercel flushing logic & add test for it (#16208)
b74f300
As pre-work for
#16178, actually add
a test for this (kind of). This showed that there was actually a
fundamental flaw here, as we looked as the `req` not the `res`, oops.
@mydea
mydeaforce-pushed
the
fn/patch-response-channel
branch
from
96423b2 to
8e6d77eCompareMay 7, 2025 08:43
@mydea
mydea
marked this pull request as ready for review
May 7, 2025 09:17
@mydea
mydea
commented
May 7, 2025

Copy link


Copy Markdown

MemberAuthor
OK we can't get this to work on Vercel on prod, so we'll just remove this for now until we get a better handle on this 😬 cc @lforst
@lforst
lforst
commented
May 7, 2025

Copy link


Copy Markdown

Contributor
I am in support of removing
@mydeamydea
mentioned this pull request
May 7, 2025
 Merged

@mydea
mydea
commented
May 7, 2025

Copy link


Copy Markdown

MemberAuthor
Replaced by #16217
@mydeamydea
closed this
May 7, 2025
@mydea
mydea
deleted the

fn/patch-response-channel

branch
May 7, 2025 13:35
mydea added a commit
that referenced
this pull request
May 7, 2025
@mydea
ref(node): Remove vercel flushing code that does nothing (#16217)
edabe21
We could never get this to apply properly on vercel in production, so
we're removing this for now and may revisit this later.
Replaces #16178
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers


1 more reviewer

@lforstlforstlforst approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@mydeamydea

Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

2 participants

@mydea@lforst

, '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); }
})();
})();
ref(node): Avoid double wrapping `http` module for vercel on Node by mydea · Pull Request #16178 · getsentry/sentry-javascript · GitHub
Skip to content
ref(node): Avoid double wrapping http module for vercel on Node - #16178
Closed
mydea wants to merge 3 commits into
developfrom 
fn/patch-response-channel
Closed
ref(node): Avoid double wrapping http module for vercel on Node#16178
mydea wants to merge 3 commits into
developfrom 
fn/patch-response-channel
Conversation
@mydea
@mydeamydea
commented
Apr 30, 2025•

edited

Loading

Copy link


Copy Markdown

Member
This PR is a follow up to #16177
There, we found out that double-wrapping (with stealthWrap) the http module seems to break in ESM mode, for whatever reason - us double-wrapping this leads to the OTEL-instrumentation creating spans twice, for whatever reason.
We removed this for the core sentry-http instrumentation, but we still do it for vercel-edge here. For consistency, this cleans this up so we should be safe.
As an added benefit, it again leads to all the sentry-specific instrumentation code to be in one place, and to us not having to care about instrumentation order, hopefully, if this actually works.
@mydea
mydea
requested a review
from lforstApril 30, 2025 14:25
@mydeamydea
self-assigned this
Apr 30, 2025
@mydea
mydeaforce-pushed
the
fn/patch-response-channel
branch
from
67ee12d to
512e319CompareApril 30, 2025 14:25
@github-actions
github-actionsBot
commented
Apr 30, 2025•

edited

Loading

Copy link


Copy Markdown

Contributor
size-limit report 📦
PathSize% ChangeChange
@sentry/browser23.34 KB--
@sentry/browser - with treeshaking flags23.16 KB--
@sentry/browser (incl. Tracing)37.22 KB--
@sentry/browser (incl. Tracing, Replay)74.45 KB--
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags68.32 KB--
@sentry/browser (incl. Tracing, Replay with Canvas)79.11 KB--
@sentry/browser (incl. Tracing, Replay, Feedback)90.93 KB--
@sentry/browser (incl. Feedback)39.72 KB--
@sentry/browser (incl. sendFeedback)27.96 KB--
@sentry/browser (incl. FeedbackAsync)32.72 KB--
@sentry/react25.14 KB--
@sentry/react (incl. Tracing)39.22 KB--
@sentry/vue27.6 KB--
@sentry/vue (incl. Tracing)38.99 KB--
@sentry/svelte23.36 KB--
CDN Bundle24.55 KB--
CDN Bundle (incl. Tracing)37.3 KB--
CDN Bundle (incl. Tracing, Replay)72.34 KB--
CDN Bundle (incl. Tracing, Replay, Feedback)77.62 KB--
CDN Bundle - uncompressed71.62 KB--
CDN Bundle (incl. Tracing) - uncompressed110.34 KB--
CDN Bundle (incl. Tracing, Replay) - uncompressed221.63 KB--
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed234.15 KB--
@sentry/nextjs (client)40.83 KB--
@sentry/sveltekit (client)37.72 KB--
@sentry/node144.26 KB-0.11%-153 B 🔽
@sentry/node - without tracing96.14 KB-0.16%-150 B 🔽
@sentry/aws-serverless120.49 KB-0.14%-167 B 🔽
View base workflow run
@mydea
mydeaforce-pushed
the
fn/http-instrumentation-diagnostics-channel
branch
from
72ddee3 to
596dceeCompareMay 2, 2025 11:14
@lforst
lforst
commented
May 5, 2025

Copy link


Copy Markdown

Contributor
Would you mind explaining in the PR description why we do this and what this PR does? I can see us going back to this in 3 months and asking ourselves wtf
@mydea
mydeaforce-pushed
the
fn/http-instrumentation-diagnostics-channel
branch
from
aff551f to
0cd1834CompareMay 5, 2025 08:27

Base automatically changed from
fn/http-instrumentation-diagnostics-channel to
developMay 5, 2025 12:24
@mydea
mydeaforce-pushed
the
fn/patch-response-channel
branch
from
512e319 to
96423b2CompareMay 5, 2025 12:32
@mydeamydea
changed the title
ref(node): Avoid double wrapping http module for vercel-edgeref(node): Avoid double wrapping http module for vercel on NodeMay 5, 2025
@lforst
lforst
commented
May 6, 2025

Copy link


Copy Markdown

Contributor
To recapitulate, what needs to happen for the logic to still work is to register the res.on('close') listener after OTEL registers its own res.on('close') listener for ending a span. Reminder that res.on('close') is only fired after res.end() has been called.
Findings so far, because the current approach doesn't work:
  • Putting the patching logic into http.server.response.created does not work because it will run before OTEL applies its .on('close') handler.
  • Putting the patching logic into http.server.response.finish for some reason does not work because the diagnostic handler weirdly runs afterres.on('close') is fired (even though the code in Node.js would indicate otherwise???).
tldr we have no good place to register the res.on('close') handler...
@mydeamydea
mentioned this pull request
May 6, 2025
 Merged

mydea added a commit
that referenced
this pull request
May 6, 2025
@mydea
fix(node): Fix vercel flushing logic & add test for it (#16208)
b74f300
As pre-work for
#16178, actually add
a test for this (kind of). This showed that there was actually a
fundamental flaw here, as we looked as the `req` not the `res`, oops.
@mydea
mydeaforce-pushed
the
fn/patch-response-channel
branch
from
96423b2 to
8e6d77eCompareMay 7, 2025 08:43
@mydea
mydea
marked this pull request as ready for review
May 7, 2025 09:17
@mydea
mydea
commented
May 7, 2025

Copy link


Copy Markdown

MemberAuthor
OK we can't get this to work on Vercel on prod, so we'll just remove this for now until we get a better handle on this 😬 cc @lforst
@lforst
lforst
commented
May 7, 2025

Copy link


Copy Markdown

Contributor
I am in support of removing
@mydeamydea
mentioned this pull request
May 7, 2025
 Merged

@mydea
mydea
commented
May 7, 2025

Copy link


Copy Markdown

MemberAuthor
Replaced by #16217
@mydeamydea
closed this
May 7, 2025
@mydea
mydea
deleted the

fn/patch-response-channel

branch
May 7, 2025 13:35
mydea added a commit
that referenced
this pull request
May 7, 2025
@mydea
ref(node): Remove vercel flushing code that does nothing (#16217)
edabe21
We could never get this to apply properly on vercel in production, so
we're removing this for now and may revisit this later.
Replaces #16178
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers


1 more reviewer

@lforstlforstlforst approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@mydeamydea

Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

2 participants

@mydea@lforst