Skip to content

ref: Remove usage of span tags #10808

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
Merged
merged 3 commits into from
Feb 27, 2024
Merged

ref: Remove usage of span tags #10808

merged 3 commits into from
Feb 27, 2024

Conversation

mydea
Copy link
Member

@mydea mydea commented Feb 26, 2024 •
edited
Loading

Mostly, we rewrite this to just use attributes.

This affects esp.:

  • browser profiling
  • angular tracing
  • http status (only set as attribute now)
  • remix tracing
  • remove all routing.instrumentation type tags completely (this is covered by origin now anyhow)

Not covered by this are browser metrics, which is a dedicated PR here: #10823

@mydea mydea self-assigned this Feb 26, 2024
@mydea mydea mentioned this pull request Feb 26, 2024
Merged
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Feb 26, 2024 •
edited
Loading

size-limit report 📦

Path Size
@sentry/browser (incl. Tracing, Replay, Feedback) - Webpack (gzipped) 77.29 KB (+0.03% 🔺)
@sentry/browser (incl. Tracing, Replay) - Webpack (gzipped) 68.55 KB (+0.02% 🔺)
@sentry/browser (incl. Tracing, Replay with Canvas) - Webpack (gzipped) 72.46 KB (+0.02% 🔺)
@sentry/browser (incl. Tracing, Replay) - Webpack with treeshaking flags (gzipped) 62.05 KB (+0.01% 🔺)
@sentry/browser (incl. Tracing) - Webpack (gzipped) 32.73 KB (+0.03% 🔺)
@sentry/browser (incl. browserTracingIntegration) - Webpack (gzipped) 32.73 KB (+0.03% 🔺)
@sentry/browser (incl. Feedback) - Webpack (gzipped) 30.91 KB (0%)
@sentry/browser (incl. sendFeedback) - Webpack (gzipped) 30.91 KB (0%)
@sentry/browser - Webpack (gzipped) 22.19 KB (0%)
@sentry/browser (incl. Tracing, Replay, Feedback) - ES6 CDN Bundle (gzipped) 75.69 KB (-0.08% 🔽)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (gzipped) 67.46 KB (-0.06% 🔽)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (gzipped) 33.24 KB (-0.15% 🔽)
@sentry/browser - ES6 CDN Bundle (gzipped) 24.74 KB (+0.06% 🔺)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (minified & uncompressed) 211.04 KB (-0.12% 🔽)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (minified & uncompressed) 99.76 KB (-0.25% 🔽)
@sentry/browser - ES6 CDN Bundle (minified & uncompressed) 73.96 KB (+0.25% 🔺)
@sentry/browser (incl. Tracing) - ES5 CDN Bundle (gzipped) 36.29 KB (-0.2% 🔽)
@sentry/react (incl. Tracing, Replay) - Webpack (gzipped) 68.83 KB (+0.02% 🔺)
@sentry/react - Webpack (gzipped) 22.22 KB (0%)
@sentry/nextjs Client (incl. Tracing, Replay) - Webpack (gzipped) 85.24 KB (-0.02% 🔽)
@sentry/nextjs Client - Webpack (gzipped) 49.62 KB (-0.08% 🔽)
@sentry-internal/feedback - Webpack (gzipped) 17.14 KB (0%)

@mydea mydea force-pushed the fn/stop-using-span-tags branch from 060bcf8 to 83b1506 Compare February 26, 2024 15:05
@mydea mydea force-pushed the fn/stop-using-span-tags branch from 83b1506 to 077a19e Compare February 26, 2024 16:28
@mydea
Copy link
Member Author

mydea commented Feb 27, 2024

Actually, I need to double check with performance folks if we are good to migrate the browser metrics from tags to attributes, not sure if something relies on these...!

@mydea
ref: Refactor most usage of tags on spans
deab246 
Only missing are browser metrics, where we need to verify this can be refactored already.
@mydea mydea mentioned this pull request Feb 27, 2024
Merged
@mydea mydea force-pushed the fn/stop-using-span-tags branch from c237db8 to deab246 Compare February 27, 2024 12:28
@mydea
Copy link
Member Author

mydea commented Feb 27, 2024

I ended up extracting the browser metrics changes out into #10823, so we can already merge the "clearer" stuff.

mydea
mydea commented Feb 27, 2024
View reviewed changes
packages/nextjs/src/client/routing/appRouterRoutingInstrumentation.ts
attributes: {
from: currPathname,
Copy link
Member Author

Choose a reason for hiding this comment

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

@lforst Just a reminder for ourselves during the v8 alpha: we need to review all our auto instrumentation for what attribute we set, what we actually need/want, etc.

@mydea mydea merged commit a6a4e53 into develop Feb 27, 2024
@mydea mydea deleted the fn/stop-using-span-tags branch February 27, 2024 15:08
mydea added a commit that referenced this pull request Mar 7, 2024
@mydea
ref(browser): Store browser metrics as attributes instead of tags (#1…
c7c6bb2 
…0823)

Extracted this out from
#10808, as this is a
bit more impactful, possibly.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@onurtemizkan onurtemizkan onurtemizkan approved these changes

@Lms24 Lms24 Lms24 approved these changes

@AbhiPrasad AbhiPrasad AbhiPrasad approved these changes

@JonasBa JonasBa Awaiting requested review from JonasBa

Assignees

@mydea mydea

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mydea @onurtemizkan @Lms24 @AbhiPrasad