Skip to content

ref(v8): Remove metadata on transaction #11397

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Apr 3, 2024
Merged

Conversation

mydea
Copy link
Member

@mydea mydea commented Apr 3, 2024

Instead, we freeze the DSC explicitly onto the transaction now.

@mydea mydea requested review from lforst, Lms24 and AbhiPrasad April 3, 2024 11:56
@mydea mydea self-assigned this Apr 3, 2024
Copy link
Contributor

github-actions bot commented Apr 3, 2024

size-limit report 📦

Path Size
@sentry/browser (incl. Tracing, Replay, Feedback) 76.05 KB (-0.16% 🔽)
@sentry/browser (incl. Tracing, Replay) 67.32 KB (-0.2% 🔽)
@sentry/browser (incl. Tracing, Replay with Canvas) 71.15 KB (-0.2% 🔽)
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags 60.88 KB (-0.21% 🔽)
@sentry/browser (incl. Tracing) 31.97 KB (-0.44% 🔽)
@sentry/browser (incl. browserTracingIntegration) 31.97 KB (-0.44% 🔽)
@sentry/browser (incl. feedbackIntegration) 31.28 KB (-0.16% 🔽)
@sentry/browser (incl. feedbackModalIntegration) 31.39 KB (-0.19% 🔽)
@sentry/browser (incl. feedbackScreenshotIntegration) 31.39 KB (-0.19% 🔽)
@sentry/browser (incl. sendFeedback) 27.35 KB (-0.24% 🔽)
@sentry/browser 22.52 KB (-0.23% 🔽)
CDN Bundle (incl. Tracing, Replay, Feedback) 71.08 KB (-0.16% 🔽)
CDN Bundle (incl. Tracing, Replay) 65.79 KB (-0.22% 🔽)
CDN Bundle (incl. Tracing) 32.12 KB (-0.47% 🔽)
CDN Bundle 23.74 KB (-0.26% 🔽)
CDN Bundle (incl. Tracing, Replay) - uncompressed 204.97 KB (-0.26% 🔽)
CDN Bundle (incl. Tracing) - uncompressed 95.62 KB (-0.55% 🔽)
CDN Bundle - uncompressed 70.05 KB (-0.27% 🔽)
@sentry/react (incl. Tracing, Replay) 67.29 KB (-0.23% 🔽)
@sentry/react 22.55 KB (-0.21% 🔽)

Comment on lines 253 to 254
// TODO: We should probably only set this if we know we are continuing a trace
// For now, there isn't really a way to know this from in here, though...
Copy link
Member

Choose a reason for hiding this comment

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

with "this" are you referring to spanContext? Or should this be moved to the if(dsc) part below?

Copy link
Member Author

Choose a reason for hiding this comment

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

ah wait, sorry, I did not move this comment along correctly with code I moved 😅 yeah it belongs to below!

Comment on lines +258 to +266
if (dsc) {
freezeDscOnSpan(span, dsc);
}
Copy link
Member

Choose a reason for hiding this comment

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

Just to check here but I think this is already how it works: Freezing only happens where we're continuing a trace (i.e. if a propagationContext with a DSC exists). In case it doesn't (i.e. we're head of trace || no DSC on propCtx), we don't freeze the DSC at all at the moment, correct? Meaning, we'd populate it for outgoing requests headers or when the span/txn is is processed.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, dsc will only be set on the propagation context if we had an incoming trace, otherwise this will be undefined and we don't freeze it!

mydea added 4 commits April 3, 2024 17:19
Instead, we freeze the DSC explicitly onto the transaction now.

Apply suggestions from code review

Co-authored-by: Lukas Stracke <[email protected]>
fix comments

fix linting
@mydea mydea force-pushed the fn/transaction-metadata branch from b0a881e to 00a8f25 Compare April 3, 2024 15:26
@AbhiPrasad
Copy link
Member

Merging this in so I get more manageable merge conflicts

@AbhiPrasad AbhiPrasad merged commit bcfde90 into develop Apr 3, 2024
@AbhiPrasad AbhiPrasad deleted the fn/transaction-metadata branch April 3, 2024 16:34
cadesalaberry pushed a commit to cadesalaberry/sentry-javascript that referenced this pull request Apr 19, 2024
Instead, we freeze the DSC explicitly onto the transaction now.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants