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.

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

feat(core)!: Pass root spans to beforeSendSpan and disallow returning null #14831

Open
wants to merge 11 commits into
base: develop
Choose a base branch
from

Conversation

chargome
Copy link
Member

@chargome chargome commented Dec 23, 2024

  • Disallows returning null from beforeSendSpan
  • Passes root spans to beforeSendSpan
  • Adds entry to migration guide and changelog

Looked at two options for passing the root span:

  1. Within the SentrySpan class, before converting the root span into a transaction.
  2. Parsing a SpanJSON out of an event and then writing back the updated values into the event.

Went with (2), passing the root span within processBeforeSend to have all processing hooks centrally in one place and because the other approach would have been too messy.

closes #14336

@chargome chargome self-assigned this Dec 23, 2024
Copy link
Contributor

github-actions bot commented Dec 23, 2024

size-limit report 📦

Path Size % Change Change
@sentry/browser 23.31 KB +2.04% +476 B 🔺
@sentry/browser - with treeshaking flags 21.98 KB +1.84% +405 B 🔺
@sentry/browser (incl. Tracing) 35.79 KB +1.05% +380 B 🔺
@sentry/browser (incl. Tracing, Replay) 73.08 KB +0.51% +376 B 🔺
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags 63.44 KB +0.54% +344 B 🔺
@sentry/browser (incl. Tracing, Replay with Canvas) 77.47 KB +0.47% +371 B 🔺
@sentry/browser (incl. Tracing, Replay, Feedback) 89.84 KB +0.4% +365 B 🔺
@sentry/browser (incl. Feedback) 40.05 KB +1.12% +454 B 🔺
@sentry/browser (incl. sendFeedback) 27.92 KB +1.58% +444 B 🔺
@sentry/browser (incl. FeedbackAsync) 32.68 KB +1.35% +445 B 🔺
@sentry/react 26.05 KB +1.69% +441 B 🔺
@sentry/react (incl. Tracing) 38.63 KB +1.01% +394 B 🔺
@sentry/vue 27.49 KB +1.36% +376 B 🔺
@sentry/vue (incl. Tracing) 37.61 KB +0.9% +340 B 🔺
@sentry/svelte 23.46 KB +1.91% +449 B 🔺
CDN Bundle 24.59 KB +1.51% +373 B 🔺
CDN Bundle (incl. Tracing) 36.13 KB +1.03% +374 B 🔺
CDN Bundle (incl. Tracing, Replay) 71.25 KB +0.52% +374 B 🔺
CDN Bundle (incl. Tracing, Replay, Feedback) 76.47 KB +0.51% +395 B 🔺
CDN Bundle - uncompressed 72.12 KB +1.58% +1.12 KB 🔺
CDN Bundle (incl. Tracing) - uncompressed 107.54 KB +1.01% +1.07 KB 🔺
CDN Bundle (incl. Tracing, Replay) - uncompressed 221.78 KB +0.49% +1.07 KB 🔺
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed 234.65 KB +0.46% +1.07 KB 🔺
@sentry/nextjs (client) 38.92 KB +1% +392 B 🔺
@sentry/sveltekit (client) 36.34 KB +1.11% +406 B 🔺
@sentry/node 163.28 KB +0.2% +319 B 🔺
@sentry/node - without tracing 99.03 KB +0.3% +297 B 🔺
@sentry/aws-serverless 128.89 KB +0.25% +318 B 🔺

View base workflow run

@chargome chargome marked this pull request as ready for review December 31, 2024 11:55
@chargome chargome requested review from mydea and Lms24 January 2, 2025 12:14
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.

This generally looks good to me and I think the conversion is correct. Before we merge this, let's please add a sentence to the PR description why we went with this approach instead of calling beforeSendSpan earlier for posterity.

packages/core/test/lib/baseclient.test.ts Show resolved Hide resolved
packages/core/src/utils/transactionEvent.ts Outdated Show resolved Hide resolved
packages/core/src/baseclient.ts Outdated Show resolved Hide resolved
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.

[v9] Disallow returning null from beforeSendSpan and also pass root spans
2 participants