Skip to content

fix(core): Use consistent continueTrace implementation in core - #14813

Merged
mydea merged 5 commits into
developfrom
fn/continueTraceACS
Dec 20, 2024
Merged

fix(core): Use consistent continueTrace implementation in core#14813
mydea merged 5 commits into
developfrom
fn/continueTraceACS

Conversation

@mydea

@mydeamydea commented Dec 20, 2024

Copy link
Copy Markdown
Member

We have a different implementation of continueTrace for OTEL/Node. Until now we relied on actually using the import from @sentry/node vs @sentry/core to ensure this. However, this is a footgun, and actually lead to a problem in NextJS because we used the core variant there. Also, it is simply not isomorphic.

So to fix this, this PR puts continueTrace on the ACS so we can consistently use the core variant in all environments.

Fixes#14787

@mydeamydea self-assigned this Dec 20, 2024
Lms24
Lms24 approved these changes Dec 20, 2024
View reviewed changes

@Lms24Lms24 left a comment

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.

Thanks for fixing this!

@github-actions

github-actionsBot commented Dec 20, 2024•
edited
Loading

Copy link
Copy Markdown
Contributor

size-limit report 📦

PathSize% ChangeChange
@sentry/browser23.16 KB--
@sentry/browser - with treeshaking flags21.83 KB--
@sentry/browser (incl. Tracing)35.66 KB--
@sentry/browser (incl. Tracing, Replay)72.91 KB--
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags63.31 KB--
@sentry/browser (incl. Tracing, Replay with Canvas)77.32 KB--
@sentry/browser (incl. Tracing, Replay, Feedback)89.71 KB--
@sentry/browser (incl. Feedback)39.91 KB--
@sentry/browser (incl. sendFeedback)27.77 KB--
@sentry/browser (incl. FeedbackAsync)32.53 KB--
@sentry/react25.88 KB--
@sentry/react (incl. Tracing)38.51 KB-0.01%-1 B 🔽
@sentry/vue27.38 KB--
@sentry/vue (incl. Tracing)37.51 KB--
@sentry/svelte23.31 KB--
CDN Bundle24.26 KB+0.09%+21 B 🔺
CDN Bundle (incl. Tracing)35.81 KB+0.05%+15 B 🔺
CDN Bundle (incl. Tracing, Replay)70.92 KB+0.03%+18 B 🔺
CDN Bundle (incl. Tracing, Replay, Feedback)76.21 KB+0.03%+17 B 🔺
CDN Bundle - uncompressed71.32 KB+0.12%+81 B 🔺
CDN Bundle (incl. Tracing) - uncompressed106.78 KB+0.08%+81 B 🔺
CDN Bundle (incl. Tracing, Replay) - uncompressed220.85 KB+0.04%+81 B 🔺
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed234.05 KB+0.04%+81 B 🔺
@sentry/nextjs (client)38.79 KB--
@sentry/sveltekit (client)36.18 KB--
@sentry/node157.98 KB+0.02%+32 B 🔺
@sentry/node - without tracing98.87 KB+0.04%+32 B 🔺
@sentry/aws-serverless126.6 KB+0.03%+33 B 🔺

View base workflow run

@codecov

codecovBot commented Dec 20, 2024•
edited
Loading

Copy link
Copy Markdown

❌ 1 Tests Failed:

Tests completedFailedPassedSkipped
6891688298
View the top 1 failed tests by shortest run time
transactions.test.tsShouldsendawrappedserveractionasachildofanextjstransaction
Stack Traces | 0.239s run time
transactions.test.ts:79:5Shouldsendawrappedserveractionasachildofanextjstransaction

To view more test analytics, go to the Test Analytics Dashboard
📢 Thoughts on this report? Let us know!

@mydea

mydea commented Dec 20, 2024•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

Hmm, so with this change next.js tests fail 🤔

This test: Should send a wrapped server action as a child of a nextjs transaction

Fails, because now the POST /server-action and the serverAction/myServerAction both have the same parent_span_id, instead of the server action being the child of the POST. Any idea why this is the case/if/why this is wrong/correct? 🤔 @lforst and @chargome

Adding some logs, I see:

NEXT SPAN IS HERE {
[WebServer] span_id: '7cbbd9156b3c695e',
[WebServer] trace_id: 'd59a8a7dd8014861b05937614d443e80',
[WebServer] data: {
[WebServer] 'next.span_name': 'POST /server-action',
[WebServer] 'next.span_type': 'BaseServer.handleRequest',
[WebServer] 'http.method': 'POST',
[WebServer] 'http.target': '/server-action',
[WebServer] 'sentry.sample_rate': 1,
[WebServer] 'sentry.parentIsRemote': true
[WebServer] },
[WebServer] description: 'POST /server-action',
[WebServer] parent_span_id: 'bd2a69529b385a7e',
[WebServer] start_timestamp: 1734692699.014
[WebServer] }
[WebServer] {
[WebServer] sentryTraceHeader: 'd59a8a7dd8014861b05937614d443e80-bd2a69529b385a7e-1',
[WebServer] baggageHeader: 'sentry-environment=qa,sentry-release=fqG4zegKOdcAQm8W9PhP-,sentry-public_key=3b6c388182fb435097f41d181be2b2ba,sentry-trace_id=d59a8a7dd8014861b05937614d443e80,sentry-sample_rate=1,sentry-sampled=true'
[WebServer] }
[WebServer] {
[WebServer] span_id: '5e6e2e489641d3b2',
[WebServer] trace_id: 'd59a8a7dd8014861b05937614d443e80',
[WebServer] data: {
[WebServer] 'sentry.op': 'function.server_action',
[WebServer] 'sentry.source': 'route',
[WebServer] 'sentry.sample_rate': 1,
[WebServer] 'sentry.parentIsRemote': true
[WebServer] },
[WebServer] description: 'serverAction/myServerAction',
[WebServer] parent_span_id: 'bd2a69529b385a7e',
[WebServer] start_timestamp: 1734692699.024,
[WebServer] op: 'function.server_action'
[WebServer] }

Which shows:

  1. the POST span is started by Sentry
  2. Then, continueTrace gets the sentryTrace header, where the span ID is the parent of the POST span
  3. This is "correctly" continued then for the serverAction 🤔

This "seems" correct, but...

@lforst

lforst commented Dec 20, 2024

Copy link
Copy Markdown
Contributor

Right, so I guess the desired behavior is that if there is a POST server span we attach the server action span to that one. If there isn't we attach the server action span to the incoming parent span ID.

My first instinct was that we can probably remove the continueTrace call from the server action instrumentation, however some nextjs versions have problems with http instrumentation so we would probably end up with some of the server action spans being detached.

Maybe conditionally calling continueTrace when there is not an active and recording span is the way to go?

@andreiborza

andreiborza commented Dec 20, 2024

Copy link
Copy Markdown
Member

I had the same issue in the solidstart sdk and we ended up just dropping continueTrace here https://github.com/getsentry/sentry-javascript/blob/develop/packages/solidstart/src/server/withServerActionInstrumentation.ts#L30.

But yea, it didn't come with a baggage of having to support older versions.

@lforstlforst mentioned this pull request Dec 20, 2024
Closed
@mydea

mydea commented Dec 20, 2024

Copy link
Copy Markdown
MemberAuthor

Ahh, that makes sense. I will fix this then, I have an idea (let's see if that works).

@mydea
mydeaforce-pushed the fn/continueTraceACS branch from b3ce32e to 0b5edbdCompareDecember 20, 2024 12:41
@mydea
mydea merged commit 65531f3 into developDec 20, 2024
@mydea
mydea deleted the fn/continueTraceACS branch December 20, 2024 13:25
s1gr1d pushed a commit that referenced this pull request Dec 20, 2024
@mydea@s1gr1d
fix(v8/core): Use consistentcontinueTraceimplementation in core (#…
1f5013a
…14813)
We have a different implementation of `continueTrace` for OTEL/Node.
Until now we relied on actually using the import from `@sentry/node` vs
`@sentry/core` to ensure this. However, this is a footgun, and actually
lead to a problem in NextJS because we used the core variant there.
Also, it is simply not isomorphic.
So to fix this, this PR puts `continueTrace` on the ACS so we can
consistently use the core variant in all environments.
Fixes#14787
s1gr1d pushed a commit that referenced this pull request Dec 20, 2024
@mydea@s1gr1d
fix(v8/core): Use consistentcontinueTraceimplementation in core (#…
519af90
…14813)
We have a different implementation of `continueTrace` for OTEL/Node.
Until now we relied on actually using the import from `@sentry/node` vs
`@sentry/core` to ensure this. However, this is a footgun, and actually
lead to a problem in NextJS because we used the core variant there.
Also, it is simply not isomorphic.
So to fix this, this PR puts `continueTrace` on the ACS so we can
consistently use the core variant in all environments.
Fixes#14787
@s1gr1ds1gr1d mentioned this pull request Dec 20, 2024
Merged
@Lms24Lms24 mentioned this pull request Dec 23, 2024
Merged
2 tasks
s1gr1d added a commit that referenced this pull request Dec 30, 2024
@s1gr1d@mydea
fix(v8/core): Use consistentcontinueTraceimplementation in core (#…
576a1ad
…14819)
V8 backport of #14813
We have a different implementation of `continueTrace` for OTEL/Node.
Until now we relied on actually using the import from `@sentry/node` vs
`@sentry/core` to ensure this. However, this is a footgun, and actually
lead to a problem in NextJS because we used the core variant there.
Also, it is simply not isomorphic.
So to fix this, this PR puts `continueTrace` on the ACS so we can
consistently use the core variant in all environments.
Fixes#14787
Co-authored-by: Francesco Gringl-Novy <francesco.novy@sentry.io>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Lms24Lms24Lms24 approved these changes
@lforstlforstAwaiting requested review from lforst
@andreiborzaandreiborzaAwaiting requested review from andreiborza

Assignees

@mydeamydea

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

NextJS Pages API Route traces are not continued from incoming trace

4 participants

@mydea@lforst@andreiborza@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(core): Use consistent `continueTrace` implementation in core by mydea · Pull Request #14813 · getsentry/sentry-javascript · GitHub
Skip to content
fix(core): Use consistent continueTrace implementation in core - #14813
Merged
mydea merged 5 commits into
developfrom 
fn/continueTraceACS
Dec 20, 2024
Merged
fix(core): Use consistent continueTrace implementation in core#14813
mydea merged 5 commits into
developfrom 
fn/continueTraceACS
Conversation
@mydea
@mydeamydea
commented
Dec 20, 2024

Copy link


Copy Markdown

Member
We have a different implementation of continueTrace for OTEL/Node. Until now we relied on actually using the import from @sentry/node vs @sentry/core to ensure this. However, this is a footgun, and actually lead to a problem in NextJS because we used the core variant there. Also, it is simply not isomorphic.
So to fix this, this PR puts continueTrace on the ACS so we can consistently use the core variant in all environments.
Fixes#14787
@mydeamydea
self-assigned this
Dec 20, 2024
Lms24
Lms24
approved these changes
Dec 20, 2024
View reviewed changes
@Lms24Lms24
left a comment


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.

Thanks for fixing this!
@github-actions
github-actionsBot
commented
Dec 20, 2024•

edited

Loading

Copy link


Copy Markdown

Contributor
size-limit report 📦
PathSize% ChangeChange
@sentry/browser23.16 KB--
@sentry/browser - with treeshaking flags21.83 KB--
@sentry/browser (incl. Tracing)35.66 KB--
@sentry/browser (incl. Tracing, Replay)72.91 KB--
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags63.31 KB--
@sentry/browser (incl. Tracing, Replay with Canvas)77.32 KB--
@sentry/browser (incl. Tracing, Replay, Feedback)89.71 KB--
@sentry/browser (incl. Feedback)39.91 KB--
@sentry/browser (incl. sendFeedback)27.77 KB--
@sentry/browser (incl. FeedbackAsync)32.53 KB--
@sentry/react25.88 KB--
@sentry/react (incl. Tracing)38.51 KB-0.01%-1 B 🔽
@sentry/vue27.38 KB--
@sentry/vue (incl. Tracing)37.51 KB--
@sentry/svelte23.31 KB--
CDN Bundle24.26 KB+0.09%+21 B 🔺
CDN Bundle (incl. Tracing)35.81 KB+0.05%+15 B 🔺
CDN Bundle (incl. Tracing, Replay)70.92 KB+0.03%+18 B 🔺
CDN Bundle (incl. Tracing, Replay, Feedback)76.21 KB+0.03%+17 B 🔺
CDN Bundle - uncompressed71.32 KB+0.12%+81 B 🔺
CDN Bundle (incl. Tracing) - uncompressed106.78 KB+0.08%+81 B 🔺
CDN Bundle (incl. Tracing, Replay) - uncompressed220.85 KB+0.04%+81 B 🔺
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed234.05 KB+0.04%+81 B 🔺
@sentry/nextjs (client)38.79 KB--
@sentry/sveltekit (client)36.18 KB--
@sentry/node157.98 KB+0.02%+32 B 🔺
@sentry/node - without tracing98.87 KB+0.04%+32 B 🔺
@sentry/aws-serverless126.6 KB+0.03%+33 B 🔺
View base workflow run
@codecov
codecovBot
commented
Dec 20, 2024•

edited

Loading

Copy link


Copy Markdown

❌ 1 Tests Failed:
Tests completedFailedPassedSkipped
6891688298
View the top 1 failed tests by shortest run time
transactions.test.tsShouldsendawrappedserveractionasachildofanextjstransaction
Stack Traces | 0.239s run time
transactions.test.ts:79:5Shouldsendawrappedserveractionasachildofanextjstransaction
To view more test analytics, go to the Test Analytics Dashboard

📢 Thoughts on this report? Let us know!
@mydea
mydea
commented
Dec 20, 2024•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Hmm, so with this change next.js tests fail 🤔
This test: Should send a wrapped server action as a child of a nextjs transaction
Fails, because now the POST /server-action and the serverAction/myServerAction both have the same parent_span_id, instead of the server action being the child of the POST. Any idea why this is the case/if/why this is wrong/correct? 🤔 @lforst and @chargome
Adding some logs, I see:
NEXT SPAN IS HERE {
[WebServer] span_id: '7cbbd9156b3c695e',
[WebServer] trace_id: 'd59a8a7dd8014861b05937614d443e80',
[WebServer] data: {
[WebServer] 'next.span_name': 'POST /server-action',
[WebServer] 'next.span_type': 'BaseServer.handleRequest',
[WebServer] 'http.method': 'POST',
[WebServer] 'http.target': '/server-action',
[WebServer] 'sentry.sample_rate': 1,
[WebServer] 'sentry.parentIsRemote': true
[WebServer] },
[WebServer] description: 'POST /server-action',
[WebServer] parent_span_id: 'bd2a69529b385a7e',
[WebServer] start_timestamp: 1734692699.014
[WebServer] }
[WebServer] {
[WebServer] sentryTraceHeader: 'd59a8a7dd8014861b05937614d443e80-bd2a69529b385a7e-1',
[WebServer] baggageHeader: 'sentry-environment=qa,sentry-release=fqG4zegKOdcAQm8W9PhP-,sentry-public_key=3b6c388182fb435097f41d181be2b2ba,sentry-trace_id=d59a8a7dd8014861b05937614d443e80,sentry-sample_rate=1,sentry-sampled=true'
[WebServer] }
[WebServer] {
[WebServer] span_id: '5e6e2e489641d3b2',
[WebServer] trace_id: 'd59a8a7dd8014861b05937614d443e80',
[WebServer] data: {
[WebServer] 'sentry.op': 'function.server_action',
[WebServer] 'sentry.source': 'route',
[WebServer] 'sentry.sample_rate': 1,
[WebServer] 'sentry.parentIsRemote': true
[WebServer] },
[WebServer] description: 'serverAction/myServerAction',
[WebServer] parent_span_id: 'bd2a69529b385a7e',
[WebServer] start_timestamp: 1734692699.024,
[WebServer] op: 'function.server_action'
[WebServer] }

Which shows:
  1. the POST span is started by Sentry
  2. Then, continueTrace gets the sentryTrace header, where the span ID is the parent of the POST span
  3. This is "correctly" continued then for the serverAction 🤔
This "seems" correct, but...
@lforst
lforst
commented
Dec 20, 2024

Copy link


Copy Markdown

Contributor
Right, so I guess the desired behavior is that if there is a POST server span we attach the server action span to that one. If there isn't we attach the server action span to the incoming parent span ID.
My first instinct was that we can probably remove the continueTrace call from the server action instrumentation, however some nextjs versions have problems with http instrumentation so we would probably end up with some of the server action spans being detached.
Maybe conditionally calling continueTrace when there is not an active and recording span is the way to go?
@andreiborza
andreiborza
commented
Dec 20, 2024

Copy link


Copy Markdown

Member
I had the same issue in the solidstart sdk and we ended up just dropping continueTrace here https://github.com/getsentry/sentry-javascript/blob/develop/packages/solidstart/src/server/withServerActionInstrumentation.ts#L30.
But yea, it didn't come with a baggage of having to support older versions.
@lforstlforst
mentioned this pull request
Dec 20, 2024
 Closed

@mydea
mydea
commented
Dec 20, 2024

Copy link


Copy Markdown

MemberAuthor
Ahh, that makes sense. I will fix this then, I have an idea (let's see if that works).
@mydea
mydeaforce-pushed
the
fn/continueTraceACS
branch
from
b3ce32e to
0b5edbdCompareDecember 20, 2024 12:41
@mydea
mydea
merged commit 65531f3
into
developDec 20, 2024
@mydea
mydea
deleted the

fn/continueTraceACS

branch
December 20, 2024 13:25
s1gr1d pushed a commit
that referenced
this pull request
Dec 20, 2024
@mydea@s1gr1d
fix(v8/core): Use consistentcontinueTraceimplementation in core (#…
1f5013a
…14813)
We have a different implementation of `continueTrace` for OTEL/Node.
Until now we relied on actually using the import from `@sentry/node` vs
`@sentry/core` to ensure this. However, this is a footgun, and actually
lead to a problem in NextJS because we used the core variant there.
Also, it is simply not isomorphic.
So to fix this, this PR puts `continueTrace` on the ACS so we can
consistently use the core variant in all environments.
Fixes#14787
s1gr1d pushed a commit
that referenced
this pull request
Dec 20, 2024
@mydea@s1gr1d
fix(v8/core): Use consistentcontinueTraceimplementation in core (#…
519af90
…14813)
We have a different implementation of `continueTrace` for OTEL/Node.
Until now we relied on actually using the import from `@sentry/node` vs
`@sentry/core` to ensure this. However, this is a footgun, and actually
lead to a problem in NextJS because we used the core variant there.
Also, it is simply not isomorphic.
So to fix this, this PR puts `continueTrace` on the ACS so we can
consistently use the core variant in all environments.
Fixes#14787
@s1gr1ds1gr1d
mentioned this pull request
Dec 20, 2024
 Merged

@Lms24Lms24
mentioned this pull request
Dec 23, 2024
 Merged

2 tasks
s1gr1d added a commit
that referenced
this pull request
Dec 30, 2024
@s1gr1d@mydea
fix(v8/core): Use consistentcontinueTraceimplementation in core (#…
576a1ad
…14819)
V8 backport of #14813
We have a different implementation of `continueTrace` for OTEL/Node.
Until now we relied on actually using the import from `@sentry/node` vs
`@sentry/core` to ensure this. However, this is a footgun, and actually
lead to a problem in NextJS because we used the core variant there.
Also, it is simply not isomorphic.
So to fix this, this PR puts `continueTrace` on the ACS so we can
consistently use the core variant in all environments.
Fixes#14787
Co-authored-by: Francesco Gringl-Novy <francesco.novy@sentry.io>
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@Lms24Lms24Lms24 approved these changes
@lforstlforstAwaiting requested review from lforst
@andreiborzaandreiborzaAwaiting requested review from andreiborza

Assignees

@mydeamydea

Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

NextJS Pages API Route traces are not continued from incoming trace


4 participants

@mydea@lforst@andreiborza@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(core): Use consistent `continueTrace` implementation in core by mydea · Pull Request #14813 · getsentry/sentry-javascript · GitHub
Skip to content
fix(core): Use consistent continueTrace implementation in core - #14813
Merged
mydea merged 5 commits into
developfrom 
fn/continueTraceACS
Dec 20, 2024
Merged
fix(core): Use consistent continueTrace implementation in core#14813
mydea merged 5 commits into
developfrom 
fn/continueTraceACS
Conversation
@mydea
@mydeamydea
commented
Dec 20, 2024

Copy link


Copy Markdown

Member
We have a different implementation of continueTrace for OTEL/Node. Until now we relied on actually using the import from @sentry/node vs @sentry/core to ensure this. However, this is a footgun, and actually lead to a problem in NextJS because we used the core variant there. Also, it is simply not isomorphic.
So to fix this, this PR puts continueTrace on the ACS so we can consistently use the core variant in all environments.
Fixes#14787
@mydeamydea
self-assigned this
Dec 20, 2024
Lms24
Lms24
approved these changes
Dec 20, 2024
View reviewed changes
@Lms24Lms24
left a comment


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.

Thanks for fixing this!
@github-actions
github-actionsBot
commented
Dec 20, 2024•

edited

Loading

Copy link


Copy Markdown

Contributor
size-limit report 📦
PathSize% ChangeChange
@sentry/browser23.16 KB--
@sentry/browser - with treeshaking flags21.83 KB--
@sentry/browser (incl. Tracing)35.66 KB--
@sentry/browser (incl. Tracing, Replay)72.91 KB--
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags63.31 KB--
@sentry/browser (incl. Tracing, Replay with Canvas)77.32 KB--
@sentry/browser (incl. Tracing, Replay, Feedback)89.71 KB--
@sentry/browser (incl. Feedback)39.91 KB--
@sentry/browser (incl. sendFeedback)27.77 KB--
@sentry/browser (incl. FeedbackAsync)32.53 KB--
@sentry/react25.88 KB--
@sentry/react (incl. Tracing)38.51 KB-0.01%-1 B 🔽
@sentry/vue27.38 KB--
@sentry/vue (incl. Tracing)37.51 KB--
@sentry/svelte23.31 KB--
CDN Bundle24.26 KB+0.09%+21 B 🔺
CDN Bundle (incl. Tracing)35.81 KB+0.05%+15 B 🔺
CDN Bundle (incl. Tracing, Replay)70.92 KB+0.03%+18 B 🔺
CDN Bundle (incl. Tracing, Replay, Feedback)76.21 KB+0.03%+17 B 🔺
CDN Bundle - uncompressed71.32 KB+0.12%+81 B 🔺
CDN Bundle (incl. Tracing) - uncompressed106.78 KB+0.08%+81 B 🔺
CDN Bundle (incl. Tracing, Replay) - uncompressed220.85 KB+0.04%+81 B 🔺
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed234.05 KB+0.04%+81 B 🔺
@sentry/nextjs (client)38.79 KB--
@sentry/sveltekit (client)36.18 KB--
@sentry/node157.98 KB+0.02%+32 B 🔺
@sentry/node - without tracing98.87 KB+0.04%+32 B 🔺
@sentry/aws-serverless126.6 KB+0.03%+33 B 🔺
View base workflow run
@codecov
codecovBot
commented
Dec 20, 2024•

edited

Loading

Copy link


Copy Markdown

❌ 1 Tests Failed:
Tests completedFailedPassedSkipped
6891688298
View the top 1 failed tests by shortest run time
transactions.test.tsShouldsendawrappedserveractionasachildofanextjstransaction
Stack Traces | 0.239s run time
transactions.test.ts:79:5Shouldsendawrappedserveractionasachildofanextjstransaction
To view more test analytics, go to the Test Analytics Dashboard

📢 Thoughts on this report? Let us know!
@mydea
mydea
commented
Dec 20, 2024•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Hmm, so with this change next.js tests fail 🤔
This test: Should send a wrapped server action as a child of a nextjs transaction
Fails, because now the POST /server-action and the serverAction/myServerAction both have the same parent_span_id, instead of the server action being the child of the POST. Any idea why this is the case/if/why this is wrong/correct? 🤔 @lforst and @chargome
Adding some logs, I see:
NEXT SPAN IS HERE {
[WebServer] span_id: '7cbbd9156b3c695e',
[WebServer] trace_id: 'd59a8a7dd8014861b05937614d443e80',
[WebServer] data: {
[WebServer] 'next.span_name': 'POST /server-action',
[WebServer] 'next.span_type': 'BaseServer.handleRequest',
[WebServer] 'http.method': 'POST',
[WebServer] 'http.target': '/server-action',
[WebServer] 'sentry.sample_rate': 1,
[WebServer] 'sentry.parentIsRemote': true
[WebServer] },
[WebServer] description: 'POST /server-action',
[WebServer] parent_span_id: 'bd2a69529b385a7e',
[WebServer] start_timestamp: 1734692699.014
[WebServer] }
[WebServer] {
[WebServer] sentryTraceHeader: 'd59a8a7dd8014861b05937614d443e80-bd2a69529b385a7e-1',
[WebServer] baggageHeader: 'sentry-environment=qa,sentry-release=fqG4zegKOdcAQm8W9PhP-,sentry-public_key=3b6c388182fb435097f41d181be2b2ba,sentry-trace_id=d59a8a7dd8014861b05937614d443e80,sentry-sample_rate=1,sentry-sampled=true'
[WebServer] }
[WebServer] {
[WebServer] span_id: '5e6e2e489641d3b2',
[WebServer] trace_id: 'd59a8a7dd8014861b05937614d443e80',
[WebServer] data: {
[WebServer] 'sentry.op': 'function.server_action',
[WebServer] 'sentry.source': 'route',
[WebServer] 'sentry.sample_rate': 1,
[WebServer] 'sentry.parentIsRemote': true
[WebServer] },
[WebServer] description: 'serverAction/myServerAction',
[WebServer] parent_span_id: 'bd2a69529b385a7e',
[WebServer] start_timestamp: 1734692699.024,
[WebServer] op: 'function.server_action'
[WebServer] }

Which shows:
  1. the POST span is started by Sentry
  2. Then, continueTrace gets the sentryTrace header, where the span ID is the parent of the POST span
  3. This is "correctly" continued then for the serverAction 🤔
This "seems" correct, but...
@lforst
lforst
commented
Dec 20, 2024

Copy link


Copy Markdown

Contributor
Right, so I guess the desired behavior is that if there is a POST server span we attach the server action span to that one. If there isn't we attach the server action span to the incoming parent span ID.
My first instinct was that we can probably remove the continueTrace call from the server action instrumentation, however some nextjs versions have problems with http instrumentation so we would probably end up with some of the server action spans being detached.
Maybe conditionally calling continueTrace when there is not an active and recording span is the way to go?
@andreiborza
andreiborza
commented
Dec 20, 2024

Copy link


Copy Markdown

Member
I had the same issue in the solidstart sdk and we ended up just dropping continueTrace here https://github.com/getsentry/sentry-javascript/blob/develop/packages/solidstart/src/server/withServerActionInstrumentation.ts#L30.
But yea, it didn't come with a baggage of having to support older versions.
@lforstlforst
mentioned this pull request
Dec 20, 2024
 Closed

@mydea
mydea
commented
Dec 20, 2024

Copy link


Copy Markdown

MemberAuthor
Ahh, that makes sense. I will fix this then, I have an idea (let's see if that works).
@mydea
mydeaforce-pushed
the
fn/continueTraceACS
branch
from
b3ce32e to
0b5edbdCompareDecember 20, 2024 12:41
@mydea
mydea
merged commit 65531f3
into
developDec 20, 2024
@mydea
mydea
deleted the

fn/continueTraceACS

branch
December 20, 2024 13:25
s1gr1d pushed a commit
that referenced
this pull request
Dec 20, 2024
@mydea@s1gr1d
fix(v8/core): Use consistentcontinueTraceimplementation in core (#…
1f5013a
…14813)
We have a different implementation of `continueTrace` for OTEL/Node.
Until now we relied on actually using the import from `@sentry/node` vs
`@sentry/core` to ensure this. However, this is a footgun, and actually
lead to a problem in NextJS because we used the core variant there.
Also, it is simply not isomorphic.
So to fix this, this PR puts `continueTrace` on the ACS so we can
consistently use the core variant in all environments.
Fixes#14787
s1gr1d pushed a commit
that referenced
this pull request
Dec 20, 2024
@mydea@s1gr1d
fix(v8/core): Use consistentcontinueTraceimplementation in core (#…
519af90
…14813)
We have a different implementation of `continueTrace` for OTEL/Node.
Until now we relied on actually using the import from `@sentry/node` vs
`@sentry/core` to ensure this. However, this is a footgun, and actually
lead to a problem in NextJS because we used the core variant there.
Also, it is simply not isomorphic.
So to fix this, this PR puts `continueTrace` on the ACS so we can
consistently use the core variant in all environments.
Fixes#14787
@s1gr1ds1gr1d
mentioned this pull request
Dec 20, 2024
 Merged

@Lms24Lms24
mentioned this pull request
Dec 23, 2024
 Merged

2 tasks
s1gr1d added a commit
that referenced
this pull request
Dec 30, 2024
@s1gr1d@mydea
fix(v8/core): Use consistentcontinueTraceimplementation in core (#…
576a1ad
…14819)
V8 backport of #14813
We have a different implementation of `continueTrace` for OTEL/Node.
Until now we relied on actually using the import from `@sentry/node` vs
`@sentry/core` to ensure this. However, this is a footgun, and actually
lead to a problem in NextJS because we used the core variant there.
Also, it is simply not isomorphic.
So to fix this, this PR puts `continueTrace` on the ACS so we can
consistently use the core variant in all environments.
Fixes#14787
Co-authored-by: Francesco Gringl-Novy <francesco.novy@sentry.io>
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@Lms24Lms24Lms24 approved these changes
@lforstlforstAwaiting requested review from lforst
@andreiborzaandreiborzaAwaiting requested review from andreiborza

Assignees

@mydeamydea

Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

NextJS Pages API Route traces are not continued from incoming trace


4 participants

@mydea@lforst@andreiborza@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(core): Use consistent `continueTrace` implementation in core by mydea · Pull Request #14813 · getsentry/sentry-javascript · GitHub
Skip to content
fix(core): Use consistent continueTrace implementation in core - #14813
Merged
mydea merged 5 commits into
developfrom 
fn/continueTraceACS
Dec 20, 2024
Merged
fix(core): Use consistent continueTrace implementation in core#14813
mydea merged 5 commits into
developfrom 
fn/continueTraceACS
Conversation
@mydea
@mydeamydea
commented
Dec 20, 2024

Copy link


Copy Markdown

Member
We have a different implementation of continueTrace for OTEL/Node. Until now we relied on actually using the import from @sentry/node vs @sentry/core to ensure this. However, this is a footgun, and actually lead to a problem in NextJS because we used the core variant there. Also, it is simply not isomorphic.
So to fix this, this PR puts continueTrace on the ACS so we can consistently use the core variant in all environments.
Fixes#14787
@mydeamydea
self-assigned this
Dec 20, 2024
Lms24
Lms24
approved these changes
Dec 20, 2024
View reviewed changes
@Lms24Lms24
left a comment


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.

Thanks for fixing this!
@github-actions
github-actionsBot
commented
Dec 20, 2024•

edited

Loading

Copy link


Copy Markdown

Contributor
size-limit report 📦
PathSize% ChangeChange
@sentry/browser23.16 KB--
@sentry/browser - with treeshaking flags21.83 KB--
@sentry/browser (incl. Tracing)35.66 KB--
@sentry/browser (incl. Tracing, Replay)72.91 KB--
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags63.31 KB--
@sentry/browser (incl. Tracing, Replay with Canvas)77.32 KB--
@sentry/browser (incl. Tracing, Replay, Feedback)89.71 KB--
@sentry/browser (incl. Feedback)39.91 KB--
@sentry/browser (incl. sendFeedback)27.77 KB--
@sentry/browser (incl. FeedbackAsync)32.53 KB--
@sentry/react25.88 KB--
@sentry/react (incl. Tracing)38.51 KB-0.01%-1 B 🔽
@sentry/vue27.38 KB--
@sentry/vue (incl. Tracing)37.51 KB--
@sentry/svelte23.31 KB--
CDN Bundle24.26 KB+0.09%+21 B 🔺
CDN Bundle (incl. Tracing)35.81 KB+0.05%+15 B 🔺
CDN Bundle (incl. Tracing, Replay)70.92 KB+0.03%+18 B 🔺
CDN Bundle (incl. Tracing, Replay, Feedback)76.21 KB+0.03%+17 B 🔺
CDN Bundle - uncompressed71.32 KB+0.12%+81 B 🔺
CDN Bundle (incl. Tracing) - uncompressed106.78 KB+0.08%+81 B 🔺
CDN Bundle (incl. Tracing, Replay) - uncompressed220.85 KB+0.04%+81 B 🔺
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed234.05 KB+0.04%+81 B 🔺
@sentry/nextjs (client)38.79 KB--
@sentry/sveltekit (client)36.18 KB--
@sentry/node157.98 KB+0.02%+32 B 🔺
@sentry/node - without tracing98.87 KB+0.04%+32 B 🔺
@sentry/aws-serverless126.6 KB+0.03%+33 B 🔺
View base workflow run
@codecov
codecovBot
commented
Dec 20, 2024•

edited

Loading

Copy link


Copy Markdown

❌ 1 Tests Failed:
Tests completedFailedPassedSkipped
6891688298
View the top 1 failed tests by shortest run time
transactions.test.tsShouldsendawrappedserveractionasachildofanextjstransaction
Stack Traces | 0.239s run time
transactions.test.ts:79:5Shouldsendawrappedserveractionasachildofanextjstransaction
To view more test analytics, go to the Test Analytics Dashboard

📢 Thoughts on this report? Let us know!
@mydea
mydea
commented
Dec 20, 2024•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Hmm, so with this change next.js tests fail 🤔
This test: Should send a wrapped server action as a child of a nextjs transaction
Fails, because now the POST /server-action and the serverAction/myServerAction both have the same parent_span_id, instead of the server action being the child of the POST. Any idea why this is the case/if/why this is wrong/correct? 🤔 @lforst and @chargome
Adding some logs, I see:
NEXT SPAN IS HERE {
[WebServer] span_id: '7cbbd9156b3c695e',
[WebServer] trace_id: 'd59a8a7dd8014861b05937614d443e80',
[WebServer] data: {
[WebServer] 'next.span_name': 'POST /server-action',
[WebServer] 'next.span_type': 'BaseServer.handleRequest',
[WebServer] 'http.method': 'POST',
[WebServer] 'http.target': '/server-action',
[WebServer] 'sentry.sample_rate': 1,
[WebServer] 'sentry.parentIsRemote': true
[WebServer] },
[WebServer] description: 'POST /server-action',
[WebServer] parent_span_id: 'bd2a69529b385a7e',
[WebServer] start_timestamp: 1734692699.014
[WebServer] }
[WebServer] {
[WebServer] sentryTraceHeader: 'd59a8a7dd8014861b05937614d443e80-bd2a69529b385a7e-1',
[WebServer] baggageHeader: 'sentry-environment=qa,sentry-release=fqG4zegKOdcAQm8W9PhP-,sentry-public_key=3b6c388182fb435097f41d181be2b2ba,sentry-trace_id=d59a8a7dd8014861b05937614d443e80,sentry-sample_rate=1,sentry-sampled=true'
[WebServer] }
[WebServer] {
[WebServer] span_id: '5e6e2e489641d3b2',
[WebServer] trace_id: 'd59a8a7dd8014861b05937614d443e80',
[WebServer] data: {
[WebServer] 'sentry.op': 'function.server_action',
[WebServer] 'sentry.source': 'route',
[WebServer] 'sentry.sample_rate': 1,
[WebServer] 'sentry.parentIsRemote': true
[WebServer] },
[WebServer] description: 'serverAction/myServerAction',
[WebServer] parent_span_id: 'bd2a69529b385a7e',
[WebServer] start_timestamp: 1734692699.024,
[WebServer] op: 'function.server_action'
[WebServer] }

Which shows:
  1. the POST span is started by Sentry
  2. Then, continueTrace gets the sentryTrace header, where the span ID is the parent of the POST span
  3. This is "correctly" continued then for the serverAction 🤔
This "seems" correct, but...
@lforst
lforst
commented
Dec 20, 2024

Copy link


Copy Markdown

Contributor
Right, so I guess the desired behavior is that if there is a POST server span we attach the server action span to that one. If there isn't we attach the server action span to the incoming parent span ID.
My first instinct was that we can probably remove the continueTrace call from the server action instrumentation, however some nextjs versions have problems with http instrumentation so we would probably end up with some of the server action spans being detached.
Maybe conditionally calling continueTrace when there is not an active and recording span is the way to go?
@andreiborza
andreiborza
commented
Dec 20, 2024

Copy link


Copy Markdown

Member
I had the same issue in the solidstart sdk and we ended up just dropping continueTrace here https://github.com/getsentry/sentry-javascript/blob/develop/packages/solidstart/src/server/withServerActionInstrumentation.ts#L30.
But yea, it didn't come with a baggage of having to support older versions.
@lforstlforst
mentioned this pull request
Dec 20, 2024
 Closed

@mydea
mydea
commented
Dec 20, 2024

Copy link


Copy Markdown

MemberAuthor
Ahh, that makes sense. I will fix this then, I have an idea (let's see if that works).
@mydea
mydeaforce-pushed
the
fn/continueTraceACS
branch
from
b3ce32e to
0b5edbdCompareDecember 20, 2024 12:41
@mydea
mydea
merged commit 65531f3
into
developDec 20, 2024
@mydea
mydea
deleted the

fn/continueTraceACS

branch
December 20, 2024 13:25
s1gr1d pushed a commit
that referenced
this pull request
Dec 20, 2024
@mydea@s1gr1d
fix(v8/core): Use consistentcontinueTraceimplementation in core (#…
1f5013a
…14813)
We have a different implementation of `continueTrace` for OTEL/Node.
Until now we relied on actually using the import from `@sentry/node` vs
`@sentry/core` to ensure this. However, this is a footgun, and actually
lead to a problem in NextJS because we used the core variant there.
Also, it is simply not isomorphic.
So to fix this, this PR puts `continueTrace` on the ACS so we can
consistently use the core variant in all environments.
Fixes#14787
s1gr1d pushed a commit
that referenced
this pull request
Dec 20, 2024
@mydea@s1gr1d
fix(v8/core): Use consistentcontinueTraceimplementation in core (#…
519af90
…14813)
We have a different implementation of `continueTrace` for OTEL/Node.
Until now we relied on actually using the import from `@sentry/node` vs
`@sentry/core` to ensure this. However, this is a footgun, and actually
lead to a problem in NextJS because we used the core variant there.
Also, it is simply not isomorphic.
So to fix this, this PR puts `continueTrace` on the ACS so we can
consistently use the core variant in all environments.
Fixes#14787
@s1gr1ds1gr1d
mentioned this pull request
Dec 20, 2024
 Merged

@Lms24Lms24
mentioned this pull request
Dec 23, 2024
 Merged

2 tasks
s1gr1d added a commit
that referenced
this pull request
Dec 30, 2024
@s1gr1d@mydea
fix(v8/core): Use consistentcontinueTraceimplementation in core (#…
576a1ad
…14819)
V8 backport of #14813
We have a different implementation of `continueTrace` for OTEL/Node.
Until now we relied on actually using the import from `@sentry/node` vs
`@sentry/core` to ensure this. However, this is a footgun, and actually
lead to a problem in NextJS because we used the core variant there.
Also, it is simply not isomorphic.
So to fix this, this PR puts `continueTrace` on the ACS so we can
consistently use the core variant in all environments.
Fixes#14787
Co-authored-by: Francesco Gringl-Novy <francesco.novy@sentry.io>
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@Lms24Lms24Lms24 approved these changes
@lforstlforstAwaiting requested review from lforst
@andreiborzaandreiborzaAwaiting requested review from andreiborza

Assignees

@mydeamydea

Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

NextJS Pages API Route traces are not continued from incoming trace


4 participants

@mydea@lforst@andreiborza@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(core): Use consistent `continueTrace` implementation in core by mydea · Pull Request #14813 · getsentry/sentry-javascript · GitHub
Skip to content
fix(core): Use consistent continueTrace implementation in core - #14813
Merged
mydea merged 5 commits into
developfrom 
fn/continueTraceACS
Dec 20, 2024
Merged
fix(core): Use consistent continueTrace implementation in core#14813
mydea merged 5 commits into
developfrom 
fn/continueTraceACS
Conversation
@mydea
@mydeamydea
commented
Dec 20, 2024

Copy link


Copy Markdown

Member
We have a different implementation of continueTrace for OTEL/Node. Until now we relied on actually using the import from @sentry/node vs @sentry/core to ensure this. However, this is a footgun, and actually lead to a problem in NextJS because we used the core variant there. Also, it is simply not isomorphic.
So to fix this, this PR puts continueTrace on the ACS so we can consistently use the core variant in all environments.
Fixes#14787
@mydeamydea
self-assigned this
Dec 20, 2024
Lms24
Lms24
approved these changes
Dec 20, 2024
View reviewed changes
@Lms24Lms24
left a comment


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.

Thanks for fixing this!
@github-actions
github-actionsBot
commented
Dec 20, 2024•

edited

Loading

Copy link


Copy Markdown

Contributor
size-limit report 📦
PathSize% ChangeChange
@sentry/browser23.16 KB--
@sentry/browser - with treeshaking flags21.83 KB--
@sentry/browser (incl. Tracing)35.66 KB--
@sentry/browser (incl. Tracing, Replay)72.91 KB--
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags63.31 KB--
@sentry/browser (incl. Tracing, Replay with Canvas)77.32 KB--
@sentry/browser (incl. Tracing, Replay, Feedback)89.71 KB--
@sentry/browser (incl. Feedback)39.91 KB--
@sentry/browser (incl. sendFeedback)27.77 KB--
@sentry/browser (incl. FeedbackAsync)32.53 KB--
@sentry/react25.88 KB--
@sentry/react (incl. Tracing)38.51 KB-0.01%-1 B 🔽
@sentry/vue27.38 KB--
@sentry/vue (incl. Tracing)37.51 KB--
@sentry/svelte23.31 KB--
CDN Bundle24.26 KB+0.09%+21 B 🔺
CDN Bundle (incl. Tracing)35.81 KB+0.05%+15 B 🔺
CDN Bundle (incl. Tracing, Replay)70.92 KB+0.03%+18 B 🔺
CDN Bundle (incl. Tracing, Replay, Feedback)76.21 KB+0.03%+17 B 🔺
CDN Bundle - uncompressed71.32 KB+0.12%+81 B 🔺
CDN Bundle (incl. Tracing) - uncompressed106.78 KB+0.08%+81 B 🔺
CDN Bundle (incl. Tracing, Replay) - uncompressed220.85 KB+0.04%+81 B 🔺
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed234.05 KB+0.04%+81 B 🔺
@sentry/nextjs (client)38.79 KB--
@sentry/sveltekit (client)36.18 KB--
@sentry/node157.98 KB+0.02%+32 B 🔺
@sentry/node - without tracing98.87 KB+0.04%+32 B 🔺
@sentry/aws-serverless126.6 KB+0.03%+33 B 🔺
View base workflow run
@codecov
codecovBot
commented
Dec 20, 2024•

edited

Loading

Copy link


Copy Markdown

❌ 1 Tests Failed:
Tests completedFailedPassedSkipped
6891688298
View the top 1 failed tests by shortest run time
transactions.test.tsShouldsendawrappedserveractionasachildofanextjstransaction
Stack Traces | 0.239s run time
transactions.test.ts:79:5Shouldsendawrappedserveractionasachildofanextjstransaction
To view more test analytics, go to the Test Analytics Dashboard

📢 Thoughts on this report? Let us know!
@mydea
mydea
commented
Dec 20, 2024•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Hmm, so with this change next.js tests fail 🤔
This test: Should send a wrapped server action as a child of a nextjs transaction
Fails, because now the POST /server-action and the serverAction/myServerAction both have the same parent_span_id, instead of the server action being the child of the POST. Any idea why this is the case/if/why this is wrong/correct? 🤔 @lforst and @chargome
Adding some logs, I see:
NEXT SPAN IS HERE {
[WebServer] span_id: '7cbbd9156b3c695e',
[WebServer] trace_id: 'd59a8a7dd8014861b05937614d443e80',
[WebServer] data: {
[WebServer] 'next.span_name': 'POST /server-action',
[WebServer] 'next.span_type': 'BaseServer.handleRequest',
[WebServer] 'http.method': 'POST',
[WebServer] 'http.target': '/server-action',
[WebServer] 'sentry.sample_rate': 1,
[WebServer] 'sentry.parentIsRemote': true
[WebServer] },
[WebServer] description: 'POST /server-action',
[WebServer] parent_span_id: 'bd2a69529b385a7e',
[WebServer] start_timestamp: 1734692699.014
[WebServer] }
[WebServer] {
[WebServer] sentryTraceHeader: 'd59a8a7dd8014861b05937614d443e80-bd2a69529b385a7e-1',
[WebServer] baggageHeader: 'sentry-environment=qa,sentry-release=fqG4zegKOdcAQm8W9PhP-,sentry-public_key=3b6c388182fb435097f41d181be2b2ba,sentry-trace_id=d59a8a7dd8014861b05937614d443e80,sentry-sample_rate=1,sentry-sampled=true'
[WebServer] }
[WebServer] {
[WebServer] span_id: '5e6e2e489641d3b2',
[WebServer] trace_id: 'd59a8a7dd8014861b05937614d443e80',
[WebServer] data: {
[WebServer] 'sentry.op': 'function.server_action',
[WebServer] 'sentry.source': 'route',
[WebServer] 'sentry.sample_rate': 1,
[WebServer] 'sentry.parentIsRemote': true
[WebServer] },
[WebServer] description: 'serverAction/myServerAction',
[WebServer] parent_span_id: 'bd2a69529b385a7e',
[WebServer] start_timestamp: 1734692699.024,
[WebServer] op: 'function.server_action'
[WebServer] }

Which shows:
  1. the POST span is started by Sentry
  2. Then, continueTrace gets the sentryTrace header, where the span ID is the parent of the POST span
  3. This is "correctly" continued then for the serverAction 🤔
This "seems" correct, but...
@lforst
lforst
commented
Dec 20, 2024

Copy link


Copy Markdown

Contributor
Right, so I guess the desired behavior is that if there is a POST server span we attach the server action span to that one. If there isn't we attach the server action span to the incoming parent span ID.
My first instinct was that we can probably remove the continueTrace call from the server action instrumentation, however some nextjs versions have problems with http instrumentation so we would probably end up with some of the server action spans being detached.
Maybe conditionally calling continueTrace when there is not an active and recording span is the way to go?
@andreiborza
andreiborza
commented
Dec 20, 2024

Copy link


Copy Markdown

Member
I had the same issue in the solidstart sdk and we ended up just dropping continueTrace here https://github.com/getsentry/sentry-javascript/blob/develop/packages/solidstart/src/server/withServerActionInstrumentation.ts#L30.
But yea, it didn't come with a baggage of having to support older versions.
@lforstlforst
mentioned this pull request
Dec 20, 2024
 Closed

@mydea
mydea
commented
Dec 20, 2024

Copy link


Copy Markdown

MemberAuthor
Ahh, that makes sense. I will fix this then, I have an idea (let's see if that works).
@mydea
mydeaforce-pushed
the
fn/continueTraceACS
branch
from
b3ce32e to
0b5edbdCompareDecember 20, 2024 12:41
@mydea
mydea
merged commit 65531f3
into
developDec 20, 2024
@mydea
mydea
deleted the

fn/continueTraceACS

branch
December 20, 2024 13:25
s1gr1d pushed a commit
that referenced
this pull request
Dec 20, 2024
@mydea@s1gr1d
fix(v8/core): Use consistentcontinueTraceimplementation in core (#…
1f5013a
…14813)
We have a different implementation of `continueTrace` for OTEL/Node.
Until now we relied on actually using the import from `@sentry/node` vs
`@sentry/core` to ensure this. However, this is a footgun, and actually
lead to a problem in NextJS because we used the core variant there.
Also, it is simply not isomorphic.
So to fix this, this PR puts `continueTrace` on the ACS so we can
consistently use the core variant in all environments.
Fixes#14787
s1gr1d pushed a commit
that referenced
this pull request
Dec 20, 2024
@mydea@s1gr1d
fix(v8/core): Use consistentcontinueTraceimplementation in core (#…
519af90
…14813)
We have a different implementation of `continueTrace` for OTEL/Node.
Until now we relied on actually using the import from `@sentry/node` vs
`@sentry/core` to ensure this. However, this is a footgun, and actually
lead to a problem in NextJS because we used the core variant there.
Also, it is simply not isomorphic.
So to fix this, this PR puts `continueTrace` on the ACS so we can
consistently use the core variant in all environments.
Fixes#14787
@s1gr1ds1gr1d
mentioned this pull request
Dec 20, 2024
 Merged

@Lms24Lms24
mentioned this pull request
Dec 23, 2024
 Merged

2 tasks
s1gr1d added a commit
that referenced
this pull request
Dec 30, 2024
@s1gr1d@mydea
fix(v8/core): Use consistentcontinueTraceimplementation in core (#…
576a1ad
…14819)
V8 backport of #14813
We have a different implementation of `continueTrace` for OTEL/Node.
Until now we relied on actually using the import from `@sentry/node` vs
`@sentry/core` to ensure this. However, this is a footgun, and actually
lead to a problem in NextJS because we used the core variant there.
Also, it is simply not isomorphic.
So to fix this, this PR puts `continueTrace` on the ACS so we can
consistently use the core variant in all environments.
Fixes#14787
Co-authored-by: Francesco Gringl-Novy <francesco.novy@sentry.io>
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@Lms24Lms24Lms24 approved these changes
@lforstlforstAwaiting requested review from lforst
@andreiborzaandreiborzaAwaiting requested review from andreiborza

Assignees

@mydeamydea

Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

NextJS Pages API Route traces are not continued from incoming trace


4 participants

@mydea@lforst@andreiborza@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(core): Use consistent `continueTrace` implementation in core by mydea · Pull Request #14813 · getsentry/sentry-javascript · GitHub
Skip to content
fix(core): Use consistent continueTrace implementation in core - #14813
Merged
mydea merged 5 commits into
developfrom 
fn/continueTraceACS
Dec 20, 2024
Merged
fix(core): Use consistent continueTrace implementation in core#14813
mydea merged 5 commits into
developfrom 
fn/continueTraceACS
Conversation
@mydea
@mydeamydea
commented
Dec 20, 2024

Copy link


Copy Markdown

Member
We have a different implementation of continueTrace for OTEL/Node. Until now we relied on actually using the import from @sentry/node vs @sentry/core to ensure this. However, this is a footgun, and actually lead to a problem in NextJS because we used the core variant there. Also, it is simply not isomorphic.
So to fix this, this PR puts continueTrace on the ACS so we can consistently use the core variant in all environments.
Fixes#14787
@mydeamydea
self-assigned this
Dec 20, 2024
Lms24
Lms24
approved these changes
Dec 20, 2024
View reviewed changes
@Lms24Lms24
left a comment


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.

Thanks for fixing this!
@github-actions
github-actionsBot
commented
Dec 20, 2024•

edited

Loading

Copy link


Copy Markdown

Contributor
size-limit report 📦
PathSize% ChangeChange
@sentry/browser23.16 KB--
@sentry/browser - with treeshaking flags21.83 KB--
@sentry/browser (incl. Tracing)35.66 KB--
@sentry/browser (incl. Tracing, Replay)72.91 KB--
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags63.31 KB--
@sentry/browser (incl. Tracing, Replay with Canvas)77.32 KB--
@sentry/browser (incl. Tracing, Replay, Feedback)89.71 KB--
@sentry/browser (incl. Feedback)39.91 KB--
@sentry/browser (incl. sendFeedback)27.77 KB--
@sentry/browser (incl. FeedbackAsync)32.53 KB--
@sentry/react25.88 KB--
@sentry/react (incl. Tracing)38.51 KB-0.01%-1 B 🔽
@sentry/vue27.38 KB--
@sentry/vue (incl. Tracing)37.51 KB--
@sentry/svelte23.31 KB--
CDN Bundle24.26 KB+0.09%+21 B 🔺
CDN Bundle (incl. Tracing)35.81 KB+0.05%+15 B 🔺
CDN Bundle (incl. Tracing, Replay)70.92 KB+0.03%+18 B 🔺
CDN Bundle (incl. Tracing, Replay, Feedback)76.21 KB+0.03%+17 B 🔺
CDN Bundle - uncompressed71.32 KB+0.12%+81 B 🔺
CDN Bundle (incl. Tracing) - uncompressed106.78 KB+0.08%+81 B 🔺
CDN Bundle (incl. Tracing, Replay) - uncompressed220.85 KB+0.04%+81 B 🔺
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed234.05 KB+0.04%+81 B 🔺
@sentry/nextjs (client)38.79 KB--
@sentry/sveltekit (client)36.18 KB--
@sentry/node157.98 KB+0.02%+32 B 🔺
@sentry/node - without tracing98.87 KB+0.04%+32 B 🔺
@sentry/aws-serverless126.6 KB+0.03%+33 B 🔺
View base workflow run
@codecov
codecovBot
commented
Dec 20, 2024•

edited

Loading

Copy link


Copy Markdown

❌ 1 Tests Failed:
Tests completedFailedPassedSkipped
6891688298
View the top 1 failed tests by shortest run time
transactions.test.tsShouldsendawrappedserveractionasachildofanextjstransaction
Stack Traces | 0.239s run time
transactions.test.ts:79:5Shouldsendawrappedserveractionasachildofanextjstransaction
To view more test analytics, go to the Test Analytics Dashboard

📢 Thoughts on this report? Let us know!
@mydea
mydea
commented
Dec 20, 2024•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Hmm, so with this change next.js tests fail 🤔
This test: Should send a wrapped server action as a child of a nextjs transaction
Fails, because now the POST /server-action and the serverAction/myServerAction both have the same parent_span_id, instead of the server action being the child of the POST. Any idea why this is the case/if/why this is wrong/correct? 🤔 @lforst and @chargome
Adding some logs, I see:
NEXT SPAN IS HERE {
[WebServer] span_id: '7cbbd9156b3c695e',
[WebServer] trace_id: 'd59a8a7dd8014861b05937614d443e80',
[WebServer] data: {
[WebServer] 'next.span_name': 'POST /server-action',
[WebServer] 'next.span_type': 'BaseServer.handleRequest',
[WebServer] 'http.method': 'POST',
[WebServer] 'http.target': '/server-action',
[WebServer] 'sentry.sample_rate': 1,
[WebServer] 'sentry.parentIsRemote': true
[WebServer] },
[WebServer] description: 'POST /server-action',
[WebServer] parent_span_id: 'bd2a69529b385a7e',
[WebServer] start_timestamp: 1734692699.014
[WebServer] }
[WebServer] {
[WebServer] sentryTraceHeader: 'd59a8a7dd8014861b05937614d443e80-bd2a69529b385a7e-1',
[WebServer] baggageHeader: 'sentry-environment=qa,sentry-release=fqG4zegKOdcAQm8W9PhP-,sentry-public_key=3b6c388182fb435097f41d181be2b2ba,sentry-trace_id=d59a8a7dd8014861b05937614d443e80,sentry-sample_rate=1,sentry-sampled=true'
[WebServer] }
[WebServer] {
[WebServer] span_id: '5e6e2e489641d3b2',
[WebServer] trace_id: 'd59a8a7dd8014861b05937614d443e80',
[WebServer] data: {
[WebServer] 'sentry.op': 'function.server_action',
[WebServer] 'sentry.source': 'route',
[WebServer] 'sentry.sample_rate': 1,
[WebServer] 'sentry.parentIsRemote': true
[WebServer] },
[WebServer] description: 'serverAction/myServerAction',
[WebServer] parent_span_id: 'bd2a69529b385a7e',
[WebServer] start_timestamp: 1734692699.024,
[WebServer] op: 'function.server_action'
[WebServer] }

Which shows:
  1. the POST span is started by Sentry
  2. Then, continueTrace gets the sentryTrace header, where the span ID is the parent of the POST span
  3. This is "correctly" continued then for the serverAction 🤔
This "seems" correct, but...
@lforst
lforst
commented
Dec 20, 2024

Copy link


Copy Markdown

Contributor
Right, so I guess the desired behavior is that if there is a POST server span we attach the server action span to that one. If there isn't we attach the server action span to the incoming parent span ID.
My first instinct was that we can probably remove the continueTrace call from the server action instrumentation, however some nextjs versions have problems with http instrumentation so we would probably end up with some of the server action spans being detached.
Maybe conditionally calling continueTrace when there is not an active and recording span is the way to go?
@andreiborza
andreiborza
commented
Dec 20, 2024

Copy link


Copy Markdown

Member
I had the same issue in the solidstart sdk and we ended up just dropping continueTrace here https://github.com/getsentry/sentry-javascript/blob/develop/packages/solidstart/src/server/withServerActionInstrumentation.ts#L30.
But yea, it didn't come with a baggage of having to support older versions.
@lforstlforst
mentioned this pull request
Dec 20, 2024
 Closed

@mydea
mydea
commented
Dec 20, 2024

Copy link


Copy Markdown

MemberAuthor
Ahh, that makes sense. I will fix this then, I have an idea (let's see if that works).
@mydea
mydeaforce-pushed
the
fn/continueTraceACS
branch
from
b3ce32e to
0b5edbdCompareDecember 20, 2024 12:41
@mydea
mydea
merged commit 65531f3
into
developDec 20, 2024
@mydea
mydea
deleted the

fn/continueTraceACS

branch
December 20, 2024 13:25
s1gr1d pushed a commit
that referenced
this pull request
Dec 20, 2024
@mydea@s1gr1d
fix(v8/core): Use consistentcontinueTraceimplementation in core (#…
1f5013a
…14813)
We have a different implementation of `continueTrace` for OTEL/Node.
Until now we relied on actually using the import from `@sentry/node` vs
`@sentry/core` to ensure this. However, this is a footgun, and actually
lead to a problem in NextJS because we used the core variant there.
Also, it is simply not isomorphic.
So to fix this, this PR puts `continueTrace` on the ACS so we can
consistently use the core variant in all environments.
Fixes#14787
s1gr1d pushed a commit
that referenced
this pull request
Dec 20, 2024
@mydea@s1gr1d
fix(v8/core): Use consistentcontinueTraceimplementation in core (#…
519af90
…14813)
We have a different implementation of `continueTrace` for OTEL/Node.
Until now we relied on actually using the import from `@sentry/node` vs
`@sentry/core` to ensure this. However, this is a footgun, and actually
lead to a problem in NextJS because we used the core variant there.
Also, it is simply not isomorphic.
So to fix this, this PR puts `continueTrace` on the ACS so we can
consistently use the core variant in all environments.
Fixes#14787
@s1gr1ds1gr1d
mentioned this pull request
Dec 20, 2024
 Merged

@Lms24Lms24
mentioned this pull request
Dec 23, 2024
 Merged

2 tasks
s1gr1d added a commit
that referenced
this pull request
Dec 30, 2024
@s1gr1d@mydea
fix(v8/core): Use consistentcontinueTraceimplementation in core (#…
576a1ad
…14819)
V8 backport of #14813
We have a different implementation of `continueTrace` for OTEL/Node.
Until now we relied on actually using the import from `@sentry/node` vs
`@sentry/core` to ensure this. However, this is a footgun, and actually
lead to a problem in NextJS because we used the core variant there.
Also, it is simply not isomorphic.
So to fix this, this PR puts `continueTrace` on the ACS so we can
consistently use the core variant in all environments.
Fixes#14787
Co-authored-by: Francesco Gringl-Novy <francesco.novy@sentry.io>
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@Lms24Lms24Lms24 approved these changes
@lforstlforstAwaiting requested review from lforst
@andreiborzaandreiborzaAwaiting requested review from andreiborza

Assignees

@mydeamydea

Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

NextJS Pages API Route traces are not continued from incoming trace


4 participants

@mydea@lforst@andreiborza@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); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
fix(core): Use consistent `continueTrace` implementation in core by mydea · Pull Request #14813 · getsentry/sentry-javascript · GitHub
Skip to content
fix(core): Use consistent continueTrace implementation in core - #14813
Merged
mydea merged 5 commits into
developfrom 
fn/continueTraceACS
Dec 20, 2024
Merged
fix(core): Use consistent continueTrace implementation in core#14813
mydea merged 5 commits into
developfrom 
fn/continueTraceACS
Conversation
@mydea
@mydeamydea
commented
Dec 20, 2024

Copy link


Copy Markdown

Member
We have a different implementation of continueTrace for OTEL/Node. Until now we relied on actually using the import from @sentry/node vs @sentry/core to ensure this. However, this is a footgun, and actually lead to a problem in NextJS because we used the core variant there. Also, it is simply not isomorphic.
So to fix this, this PR puts continueTrace on the ACS so we can consistently use the core variant in all environments.
Fixes#14787
@mydeamydea
self-assigned this
Dec 20, 2024
Lms24
Lms24
approved these changes
Dec 20, 2024
View reviewed changes
@Lms24Lms24
left a comment


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.

Thanks for fixing this!
@github-actions
github-actionsBot
commented
Dec 20, 2024•

edited

Loading

Copy link


Copy Markdown

Contributor
size-limit report 📦
PathSize% ChangeChange
@sentry/browser23.16 KB--
@sentry/browser - with treeshaking flags21.83 KB--
@sentry/browser (incl. Tracing)35.66 KB--
@sentry/browser (incl. Tracing, Replay)72.91 KB--
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags63.31 KB--
@sentry/browser (incl. Tracing, Replay with Canvas)77.32 KB--
@sentry/browser (incl. Tracing, Replay, Feedback)89.71 KB--
@sentry/browser (incl. Feedback)39.91 KB--
@sentry/browser (incl. sendFeedback)27.77 KB--
@sentry/browser (incl. FeedbackAsync)32.53 KB--
@sentry/react25.88 KB--
@sentry/react (incl. Tracing)38.51 KB-0.01%-1 B 🔽
@sentry/vue27.38 KB--
@sentry/vue (incl. Tracing)37.51 KB--
@sentry/svelte23.31 KB--
CDN Bundle24.26 KB+0.09%+21 B 🔺
CDN Bundle (incl. Tracing)35.81 KB+0.05%+15 B 🔺
CDN Bundle (incl. Tracing, Replay)70.92 KB+0.03%+18 B 🔺
CDN Bundle (incl. Tracing, Replay, Feedback)76.21 KB+0.03%+17 B 🔺
CDN Bundle - uncompressed71.32 KB+0.12%+81 B 🔺
CDN Bundle (incl. Tracing) - uncompressed106.78 KB+0.08%+81 B 🔺
CDN Bundle (incl. Tracing, Replay) - uncompressed220.85 KB+0.04%+81 B 🔺
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed234.05 KB+0.04%+81 B 🔺
@sentry/nextjs (client)38.79 KB--
@sentry/sveltekit (client)36.18 KB--
@sentry/node157.98 KB+0.02%+32 B 🔺
@sentry/node - without tracing98.87 KB+0.04%+32 B 🔺
@sentry/aws-serverless126.6 KB+0.03%+33 B 🔺
View base workflow run
@codecov
codecovBot
commented
Dec 20, 2024•

edited

Loading

Copy link


Copy Markdown

❌ 1 Tests Failed:
Tests completedFailedPassedSkipped
6891688298
View the top 1 failed tests by shortest run time
transactions.test.tsShouldsendawrappedserveractionasachildofanextjstransaction
Stack Traces | 0.239s run time
transactions.test.ts:79:5Shouldsendawrappedserveractionasachildofanextjstransaction
To view more test analytics, go to the Test Analytics Dashboard

📢 Thoughts on this report? Let us know!
@mydea
mydea
commented
Dec 20, 2024•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Hmm, so with this change next.js tests fail 🤔
This test: Should send a wrapped server action as a child of a nextjs transaction
Fails, because now the POST /server-action and the serverAction/myServerAction both have the same parent_span_id, instead of the server action being the child of the POST. Any idea why this is the case/if/why this is wrong/correct? 🤔 @lforst and @chargome
Adding some logs, I see:
NEXT SPAN IS HERE {
[WebServer] span_id: '7cbbd9156b3c695e',
[WebServer] trace_id: 'd59a8a7dd8014861b05937614d443e80',
[WebServer] data: {
[WebServer] 'next.span_name': 'POST /server-action',
[WebServer] 'next.span_type': 'BaseServer.handleRequest',
[WebServer] 'http.method': 'POST',
[WebServer] 'http.target': '/server-action',
[WebServer] 'sentry.sample_rate': 1,
[WebServer] 'sentry.parentIsRemote': true
[WebServer] },
[WebServer] description: 'POST /server-action',
[WebServer] parent_span_id: 'bd2a69529b385a7e',
[WebServer] start_timestamp: 1734692699.014
[WebServer] }
[WebServer] {
[WebServer] sentryTraceHeader: 'd59a8a7dd8014861b05937614d443e80-bd2a69529b385a7e-1',
[WebServer] baggageHeader: 'sentry-environment=qa,sentry-release=fqG4zegKOdcAQm8W9PhP-,sentry-public_key=3b6c388182fb435097f41d181be2b2ba,sentry-trace_id=d59a8a7dd8014861b05937614d443e80,sentry-sample_rate=1,sentry-sampled=true'
[WebServer] }
[WebServer] {
[WebServer] span_id: '5e6e2e489641d3b2',
[WebServer] trace_id: 'd59a8a7dd8014861b05937614d443e80',
[WebServer] data: {
[WebServer] 'sentry.op': 'function.server_action',
[WebServer] 'sentry.source': 'route',
[WebServer] 'sentry.sample_rate': 1,
[WebServer] 'sentry.parentIsRemote': true
[WebServer] },
[WebServer] description: 'serverAction/myServerAction',
[WebServer] parent_span_id: 'bd2a69529b385a7e',
[WebServer] start_timestamp: 1734692699.024,
[WebServer] op: 'function.server_action'
[WebServer] }

Which shows:
  1. the POST span is started by Sentry
  2. Then, continueTrace gets the sentryTrace header, where the span ID is the parent of the POST span
  3. This is "correctly" continued then for the serverAction 🤔
This "seems" correct, but...
@lforst
lforst
commented
Dec 20, 2024

Copy link


Copy Markdown

Contributor
Right, so I guess the desired behavior is that if there is a POST server span we attach the server action span to that one. If there isn't we attach the server action span to the incoming parent span ID.
My first instinct was that we can probably remove the continueTrace call from the server action instrumentation, however some nextjs versions have problems with http instrumentation so we would probably end up with some of the server action spans being detached.
Maybe conditionally calling continueTrace when there is not an active and recording span is the way to go?
@andreiborza
andreiborza
commented
Dec 20, 2024

Copy link


Copy Markdown

Member
I had the same issue in the solidstart sdk and we ended up just dropping continueTrace here https://github.com/getsentry/sentry-javascript/blob/develop/packages/solidstart/src/server/withServerActionInstrumentation.ts#L30.
But yea, it didn't come with a baggage of having to support older versions.
@lforstlforst
mentioned this pull request
Dec 20, 2024
 Closed

@mydea
mydea
commented
Dec 20, 2024

Copy link


Copy Markdown

MemberAuthor
Ahh, that makes sense. I will fix this then, I have an idea (let's see if that works).
@mydea
mydeaforce-pushed
the
fn/continueTraceACS
branch
from
b3ce32e to
0b5edbdCompareDecember 20, 2024 12:41
@mydea
mydea
merged commit 65531f3
into
developDec 20, 2024
@mydea
mydea
deleted the

fn/continueTraceACS

branch
December 20, 2024 13:25
s1gr1d pushed a commit
that referenced
this pull request
Dec 20, 2024
@mydea@s1gr1d
fix(v8/core): Use consistentcontinueTraceimplementation in core (#…
1f5013a
…14813)
We have a different implementation of `continueTrace` for OTEL/Node.
Until now we relied on actually using the import from `@sentry/node` vs
`@sentry/core` to ensure this. However, this is a footgun, and actually
lead to a problem in NextJS because we used the core variant there.
Also, it is simply not isomorphic.
So to fix this, this PR puts `continueTrace` on the ACS so we can
consistently use the core variant in all environments.
Fixes#14787
s1gr1d pushed a commit
that referenced
this pull request
Dec 20, 2024
@mydea@s1gr1d
fix(v8/core): Use consistentcontinueTraceimplementation in core (#…
519af90
…14813)
We have a different implementation of `continueTrace` for OTEL/Node.
Until now we relied on actually using the import from `@sentry/node` vs
`@sentry/core` to ensure this. However, this is a footgun, and actually
lead to a problem in NextJS because we used the core variant there.
Also, it is simply not isomorphic.
So to fix this, this PR puts `continueTrace` on the ACS so we can
consistently use the core variant in all environments.
Fixes#14787
@s1gr1ds1gr1d
mentioned this pull request
Dec 20, 2024
 Merged

@Lms24Lms24
mentioned this pull request
Dec 23, 2024
 Merged

2 tasks
s1gr1d added a commit
that referenced
this pull request
Dec 30, 2024
@s1gr1d@mydea
fix(v8/core): Use consistentcontinueTraceimplementation in core (#…
576a1ad
…14819)
V8 backport of #14813
We have a different implementation of `continueTrace` for OTEL/Node.
Until now we relied on actually using the import from `@sentry/node` vs
`@sentry/core` to ensure this. However, this is a footgun, and actually
lead to a problem in NextJS because we used the core variant there.
Also, it is simply not isomorphic.
So to fix this, this PR puts `continueTrace` on the ACS so we can
consistently use the core variant in all environments.
Fixes#14787
Co-authored-by: Francesco Gringl-Novy <francesco.novy@sentry.io>
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@Lms24Lms24Lms24 approved these changes
@lforstlforstAwaiting requested review from lforst
@andreiborzaandreiborzaAwaiting requested review from andreiborza

Assignees

@mydeamydea

Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

NextJS Pages API Route traces are not continued from incoming trace


4 participants

@mydea@lforst@andreiborza@Lms24

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Universal Dark Mode - works on any site
(function() {
var enabled = true;
function applyDarkMode() {
if (!enabled) return;
// Create style element if it doesn't exist
var style = document.getElementById('universal-dark-mode-style');
if (!style) {
style = document.createElement('style');
style.id = 'universal-dark-mode-style';
document.head.appendChild(style);
}
// Dark mode CSS - inverts colors but preserves images/video
style.textContent = '
/* Invert everything except media */
html {
filter: invert(1) hue-rotate(180deg) !important;
background: #1a1a2e !important;
}
/* Restore images, videos, iframes, canvas */
img, video, iframe, canvas, svg, picture, [style*="background-image"] {
filter: invert(1) hue-rotate(180deg) !important;
}
/* Preserve specific elements that should not be inverted */
.no-dark-mode, .no-dark-mode *,
[data-theme="light"], [data-theme="light"],
.ace_editor, .ace_editor *,
.CodeMirror, .CodeMirror *,
.monaco-editor, .monaco-editor *,
.markdown-body pre, .markdown-body pre *,
.highlight, .highlight *,
pre code, pre code * {
filter: none !important;
}
/* Fix common UI elements */
.modal, .popup, .dropdown-menu, .tooltip, .popover {
filter: invert(1) hue-rotate(180deg) !important;
background: #2d2d44 !important;
border-color: #444 !important;
}
/* Scrollbars */
::-webkit-scrollbar { background: #1a1a2e !important; }
::-webkit-scrollbar-thumb { background: #444 !important; }
::-webkit-scrollbar-thumb:hover { background: #555 !important; }
/* Selection */
::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }
::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }
';
}
function removeDarkMode() {
var style = document.getElementById('universal-dark-mode-style');
if (style) style.remove();
}
// Toggle with Alt+Shift+D
document.addEventListener('keydown', function(e) {
if (e.altKey && e.shiftKey && e.key === 'D') {
e.preventDefault();
enabled = !enabled;
if (enabled) {
applyDarkMode();
console.log('[Universal Dark Mode] Enabled');
} else {
removeDarkMode();
console.log('[Universal Dark Mode] Disabled');
}
}
});
// Apply on load
applyDarkMode();
// Re-apply on dynamic content
var observer = new MutationObserver(function(mutations) {
if (enabled && !document.getElementById('universal-dark-mode-style')) {
applyDarkMode();
}
});
observer.observe(document.head, { childList: true });
console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');
})();
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();
fix(core): Use consistent `continueTrace` implementation in core by mydea · Pull Request #14813 · getsentry/sentry-javascript · GitHub
Skip to content
fix(core): Use consistent continueTrace implementation in core - #14813
Merged
mydea merged 5 commits into
developfrom 
fn/continueTraceACS
Dec 20, 2024
Merged
fix(core): Use consistent continueTrace implementation in core#14813
mydea merged 5 commits into
developfrom 
fn/continueTraceACS
Conversation
@mydea
@mydeamydea
commented
Dec 20, 2024

Copy link


Copy Markdown

Member
We have a different implementation of continueTrace for OTEL/Node. Until now we relied on actually using the import from @sentry/node vs @sentry/core to ensure this. However, this is a footgun, and actually lead to a problem in NextJS because we used the core variant there. Also, it is simply not isomorphic.
So to fix this, this PR puts continueTrace on the ACS so we can consistently use the core variant in all environments.
Fixes#14787
@mydeamydea
self-assigned this
Dec 20, 2024
Lms24
Lms24
approved these changes
Dec 20, 2024
View reviewed changes
@Lms24Lms24
left a comment


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.

Thanks for fixing this!
@github-actions
github-actionsBot
commented
Dec 20, 2024•

edited

Loading

Copy link


Copy Markdown

Contributor
size-limit report 📦
PathSize% ChangeChange
@sentry/browser23.16 KB--
@sentry/browser - with treeshaking flags21.83 KB--
@sentry/browser (incl. Tracing)35.66 KB--
@sentry/browser (incl. Tracing, Replay)72.91 KB--
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags63.31 KB--
@sentry/browser (incl. Tracing, Replay with Canvas)77.32 KB--
@sentry/browser (incl. Tracing, Replay, Feedback)89.71 KB--
@sentry/browser (incl. Feedback)39.91 KB--
@sentry/browser (incl. sendFeedback)27.77 KB--
@sentry/browser (incl. FeedbackAsync)32.53 KB--
@sentry/react25.88 KB--
@sentry/react (incl. Tracing)38.51 KB-0.01%-1 B 🔽
@sentry/vue27.38 KB--
@sentry/vue (incl. Tracing)37.51 KB--
@sentry/svelte23.31 KB--
CDN Bundle24.26 KB+0.09%+21 B 🔺
CDN Bundle (incl. Tracing)35.81 KB+0.05%+15 B 🔺
CDN Bundle (incl. Tracing, Replay)70.92 KB+0.03%+18 B 🔺
CDN Bundle (incl. Tracing, Replay, Feedback)76.21 KB+0.03%+17 B 🔺
CDN Bundle - uncompressed71.32 KB+0.12%+81 B 🔺
CDN Bundle (incl. Tracing) - uncompressed106.78 KB+0.08%+81 B 🔺
CDN Bundle (incl. Tracing, Replay) - uncompressed220.85 KB+0.04%+81 B 🔺
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed234.05 KB+0.04%+81 B 🔺
@sentry/nextjs (client)38.79 KB--
@sentry/sveltekit (client)36.18 KB--
@sentry/node157.98 KB+0.02%+32 B 🔺
@sentry/node - without tracing98.87 KB+0.04%+32 B 🔺
@sentry/aws-serverless126.6 KB+0.03%+33 B 🔺
View base workflow run
@codecov
codecovBot
commented
Dec 20, 2024•

edited

Loading

Copy link


Copy Markdown

❌ 1 Tests Failed:
Tests completedFailedPassedSkipped
6891688298
View the top 1 failed tests by shortest run time
transactions.test.tsShouldsendawrappedserveractionasachildofanextjstransaction
Stack Traces | 0.239s run time
transactions.test.ts:79:5Shouldsendawrappedserveractionasachildofanextjstransaction
To view more test analytics, go to the Test Analytics Dashboard

📢 Thoughts on this report? Let us know!
@mydea
mydea
commented
Dec 20, 2024•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Hmm, so with this change next.js tests fail 🤔
This test: Should send a wrapped server action as a child of a nextjs transaction
Fails, because now the POST /server-action and the serverAction/myServerAction both have the same parent_span_id, instead of the server action being the child of the POST. Any idea why this is the case/if/why this is wrong/correct? 🤔 @lforst and @chargome
Adding some logs, I see:
NEXT SPAN IS HERE {
[WebServer] span_id: '7cbbd9156b3c695e',
[WebServer] trace_id: 'd59a8a7dd8014861b05937614d443e80',
[WebServer] data: {
[WebServer] 'next.span_name': 'POST /server-action',
[WebServer] 'next.span_type': 'BaseServer.handleRequest',
[WebServer] 'http.method': 'POST',
[WebServer] 'http.target': '/server-action',
[WebServer] 'sentry.sample_rate': 1,
[WebServer] 'sentry.parentIsRemote': true
[WebServer] },
[WebServer] description: 'POST /server-action',
[WebServer] parent_span_id: 'bd2a69529b385a7e',
[WebServer] start_timestamp: 1734692699.014
[WebServer] }
[WebServer] {
[WebServer] sentryTraceHeader: 'd59a8a7dd8014861b05937614d443e80-bd2a69529b385a7e-1',
[WebServer] baggageHeader: 'sentry-environment=qa,sentry-release=fqG4zegKOdcAQm8W9PhP-,sentry-public_key=3b6c388182fb435097f41d181be2b2ba,sentry-trace_id=d59a8a7dd8014861b05937614d443e80,sentry-sample_rate=1,sentry-sampled=true'
[WebServer] }
[WebServer] {
[WebServer] span_id: '5e6e2e489641d3b2',
[WebServer] trace_id: 'd59a8a7dd8014861b05937614d443e80',
[WebServer] data: {
[WebServer] 'sentry.op': 'function.server_action',
[WebServer] 'sentry.source': 'route',
[WebServer] 'sentry.sample_rate': 1,
[WebServer] 'sentry.parentIsRemote': true
[WebServer] },
[WebServer] description: 'serverAction/myServerAction',
[WebServer] parent_span_id: 'bd2a69529b385a7e',
[WebServer] start_timestamp: 1734692699.024,
[WebServer] op: 'function.server_action'
[WebServer] }

Which shows:
  1. the POST span is started by Sentry
  2. Then, continueTrace gets the sentryTrace header, where the span ID is the parent of the POST span
  3. This is "correctly" continued then for the serverAction 🤔
This "seems" correct, but...
@lforst
lforst
commented
Dec 20, 2024

Copy link


Copy Markdown

Contributor
Right, so I guess the desired behavior is that if there is a POST server span we attach the server action span to that one. If there isn't we attach the server action span to the incoming parent span ID.
My first instinct was that we can probably remove the continueTrace call from the server action instrumentation, however some nextjs versions have problems with http instrumentation so we would probably end up with some of the server action spans being detached.
Maybe conditionally calling continueTrace when there is not an active and recording span is the way to go?
@andreiborza
andreiborza
commented
Dec 20, 2024

Copy link


Copy Markdown

Member
I had the same issue in the solidstart sdk and we ended up just dropping continueTrace here https://github.com/getsentry/sentry-javascript/blob/develop/packages/solidstart/src/server/withServerActionInstrumentation.ts#L30.
But yea, it didn't come with a baggage of having to support older versions.
@lforstlforst
mentioned this pull request
Dec 20, 2024
 Closed

@mydea
mydea
commented
Dec 20, 2024

Copy link


Copy Markdown

MemberAuthor
Ahh, that makes sense. I will fix this then, I have an idea (let's see if that works).
@mydea
mydeaforce-pushed
the
fn/continueTraceACS
branch
from
b3ce32e to
0b5edbdCompareDecember 20, 2024 12:41
@mydea
mydea
merged commit 65531f3
into
developDec 20, 2024
@mydea
mydea
deleted the

fn/continueTraceACS

branch
December 20, 2024 13:25
s1gr1d pushed a commit
that referenced
this pull request
Dec 20, 2024
@mydea@s1gr1d
fix(v8/core): Use consistentcontinueTraceimplementation in core (#…
1f5013a
…14813)
We have a different implementation of `continueTrace` for OTEL/Node.
Until now we relied on actually using the import from `@sentry/node` vs
`@sentry/core` to ensure this. However, this is a footgun, and actually
lead to a problem in NextJS because we used the core variant there.
Also, it is simply not isomorphic.
So to fix this, this PR puts `continueTrace` on the ACS so we can
consistently use the core variant in all environments.
Fixes#14787
s1gr1d pushed a commit
that referenced
this pull request
Dec 20, 2024
@mydea@s1gr1d
fix(v8/core): Use consistentcontinueTraceimplementation in core (#…
519af90
…14813)
We have a different implementation of `continueTrace` for OTEL/Node.
Until now we relied on actually using the import from `@sentry/node` vs
`@sentry/core` to ensure this. However, this is a footgun, and actually
lead to a problem in NextJS because we used the core variant there.
Also, it is simply not isomorphic.
So to fix this, this PR puts `continueTrace` on the ACS so we can
consistently use the core variant in all environments.
Fixes#14787
@s1gr1ds1gr1d
mentioned this pull request
Dec 20, 2024
 Merged

@Lms24Lms24
mentioned this pull request
Dec 23, 2024
 Merged

2 tasks
s1gr1d added a commit
that referenced
this pull request
Dec 30, 2024
@s1gr1d@mydea
fix(v8/core): Use consistentcontinueTraceimplementation in core (#…
576a1ad
…14819)
V8 backport of #14813
We have a different implementation of `continueTrace` for OTEL/Node.
Until now we relied on actually using the import from `@sentry/node` vs
`@sentry/core` to ensure this. However, this is a footgun, and actually
lead to a problem in NextJS because we used the core variant there.
Also, it is simply not isomorphic.
So to fix this, this PR puts `continueTrace` on the ACS so we can
consistently use the core variant in all environments.
Fixes#14787
Co-authored-by: Francesco Gringl-Novy <francesco.novy@sentry.io>
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@Lms24Lms24Lms24 approved these changes
@lforstlforstAwaiting requested review from lforst
@andreiborzaandreiborzaAwaiting requested review from andreiborza

Assignees

@mydeamydea

Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

NextJS Pages API Route traces are not continued from incoming trace


4 participants

@mydea@lforst@andreiborza@Lms24