Skip to content
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.

Sign up for GitHub

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

Jump to bottom

ref(core): Ensure non-sampled spans are NonRecordingSpans #14955

Draft
wants to merge 7 commits into
base: develop
Choose a base branch
from
Draft

ref(core): Ensure non-sampled spans are NonRecordingSpans #14955

wants to merge 7 commits into from
+170 −106

Conversation

mydea
Copy link
Member

@mydea mydea commented Jan 9, 2025

Noticed that we were still using a regular SentrySpan for unsampled spans, instead of NonRecordingSpans. This should be more "consistent" now.

@mydea mydea requested review from Lms24 and s1gr1d January 9, 2025 10:54
@mydea mydea self-assigned this Jan 9, 2025
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jan 9, 2025 •
edited
Loading

size-limit report 📦

⚠️ Warning: Base artifact is not the latest one, because the latest workflow run is not done yet. This may lead to incorrect results. Try to re-run all tests to get up to date results.

Path Size % Change Change
@sentry/browser 23.01 KB - -
@sentry/browser - with treeshaking flags 21.68 KB - -
@sentry/browser (incl. Tracing) 35.58 KB +0.14% +50 B 🔺
@sentry/browser (incl. Tracing, Replay) 72.35 KB +0.05% +37 B 🔺
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags 62.87 KB +0.07% +43 B 🔺
@sentry/browser (incl. Tracing, Replay with Canvas) 76.61 KB +0.05% +39 B 🔺
@sentry/browser (incl. Tracing, Replay, Feedback) 88.64 KB +0.06% +53 B 🔺
@sentry/browser (incl. Feedback) 39.23 KB - -
@sentry/browser (incl. sendFeedback) 27.64 KB - -
@sentry/browser (incl. FeedbackAsync) 32.41 KB - -
@sentry/react 25.71 KB - -
@sentry/react (incl. Tracing) 38.36 KB +0.13% +50 B 🔺
@sentry/vue 27.15 KB +0.12% +31 B 🔺
@sentry/vue (incl. Tracing) 37.31 KB +0.12% +45 B 🔺
@sentry/svelte 23.13 KB - -
CDN Bundle 24.28 KB - -
CDN Bundle (incl. Tracing) 35.88 KB +0.19% +69 B 🔺
CDN Bundle (incl. Tracing, Replay) 70.52 KB +0.09% +60 B 🔺
CDN Bundle (incl. Tracing, Replay, Feedback) 75.68 KB +0.08% +59 B 🔺
CDN Bundle - uncompressed 70.78 KB - -
CDN Bundle (incl. Tracing) - uncompressed 106.24 KB +0.15% +162 B 🔺
CDN Bundle (incl. Tracing, Replay) - uncompressed 217.07 KB +0.08% +162 B 🔺
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed 229.6 KB +0.07% +162 B 🔺
@sentry/nextjs (client) 38.47 KB +0.14% +54 B 🔺
@sentry/sveltekit (client) 36.12 KB +0.13% +45 B 🔺
@sentry/node 161.46 KB -0.16% -260 B 🔽
@sentry/node - without tracing 97.24 KB -0.25% -241 B 🔽
@sentry/aws-serverless 127.11 KB -0.2% -250 B 🔽

View base workflow run

Lms24
Lms24 approved these changes Jan 9, 2025
View reviewed changes
Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

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

Thanks, good catch!

@mydea mydea force-pushed the fn/core-unsampled-rootSpans branch from 81cedd8 to a011992 Compare January 9, 2025 13:35
@codecov Codecov
Copy link

codecov bot commented Jan 9, 2025 •
edited
Loading

❌ 8 Tests Failed:

Tests completed Failed Passed Skipped
695 8 687 300
View the top 3 failed tests by shortest run time 
tracing/request/fetch-tracing-without-performance/test.ts should attach `sentry-trace` header to tracing without performance (TWP) fetch requests
Stack Traces | 0.211s run time 
test.ts:6:11 should attach `sentry-trace` header to tracing without performance (TWP) fetch requests
errors.test.ts errors on frontend and backend are connected by the same trace
Stack Traces | 0.27s run time 
errors.test.ts:4:1 errors on frontend and backend are connected by the same trace
tracing/request/xhr-tracing-without-performance/test.ts should attach `sentry-trace` header to tracing without performance (TWP) xhr requests
Stack Traces | 0.297s run time 
test.ts:6:11 should attach `sentry-trace` header to tracing without performance (TWP) xhr requests

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

@mydea mydea marked this pull request as draft January 9, 2025 14:04
This was referenced Jan 9, 2025
Merged
Merged
mydea added a commit that referenced this pull request Jan 10, 2025
@mydea
ref(core): Ensure non-recording root spans have frozen DSC (#14964)
1d98867 
Otherwise, parts of the DSC will be missing - we try to make it as
complete as we can.

Since the span cannot be updated anyhow (e.g. the name cannot be
changed), we can safely freeze this at this time.

Extracted out of
#14955
mydea added a commit that referenced this pull request Jan 10, 2025
@mydea
ref(browser): Improve active span handling for `browserTracingIntegra…
1766d4c 
…tion` (#14959)

Extracted this out of
#14955

We used to rely on implied stuff here quite a bit, which breaks if we
start returning non recording spans. Honestly this just surfaces that
this is not really ideal as it is 😬 We already pass the client around
there everywhere, so this PR updates this so we keep the active idle
span as non-enumerable prop on the client, ensuring this is consistent
and "pure".
@mydea mydea force-pushed the fn/core-unsampled-rootSpans branch from ce4c3ca to 984a0de Compare January 10, 2025 10:32
@mydea mydea force-pushed the fn/core-unsampled-rootSpans branch from ecc9230 to 6bac717 Compare January 13, 2025 07:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@s1gr1d s1gr1d s1gr1d approved these changes

@Lms24 Lms24 Lms24 approved these changes

Assignees

@mydea mydea

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mydea @Lms24 @s1gr1d