Skip to content

ref: Read propagation context off of scope and isolation scope when propagating and applying trace context #10297

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 6 commits into from
Jan 29, 2024
Merged

ref: Read propagation context off of scope and isolation scope when propagating and applying trace context #10297

merged 6 commits into from
Jan 29, 2024

Conversation

lforst
Copy link
Contributor

@lforst lforst commented Jan 23, 2024 •
edited
Loading

  • Read propagation context off of current scope and isolation scope when propagating
  • Align how we propagate in http, fetch, xhr, node-fetch instrumentation
  • Don't put propagation context on processing metadata
  • Read propagation context off of current scope and isolation scope when creating trace context
  • Appropriately freeze DSC in tracingContextFromHeaders.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jan 23, 2024 •
edited
Loading

size-limit report 📦

Path Size
@sentry/browser (incl. Tracing, Replay, Feedback) - Webpack (gzipped) 78.1 KB (-0.05% 🔽)
@sentry/browser (incl. Tracing, Replay) - Webpack (gzipped) 69.32 KB (-0.04% 🔽)
@sentry/browser (incl. Tracing, Replay with Canvas) - Webpack (gzipped) 73.2 KB (-0.06% 🔽)
@sentry/browser (incl. Tracing, Replay) - Webpack with treeshaking flags (gzipped) 62.94 KB (-0.05% 🔽)
@sentry/browser (incl. Tracing) - Webpack (gzipped) 33.34 KB (-0.13% 🔽)
@sentry/browser (incl. browserTracingIntegration) - Webpack (gzipped) 33.23 KB (-0.14% 🔽)
@sentry/browser (incl. Feedback) - Webpack (gzipped) 31.33 KB (+0.01% 🔺)
@sentry/browser (incl. sendFeedback) - Webpack (gzipped) 31.34 KB (+0.01% 🔺)
@sentry/browser - Webpack (gzipped) 22.6 KB (-0.02% 🔽)
@sentry/browser (incl. Tracing, Replay, Feedback) - ES6 CDN Bundle (gzipped) 75.88 KB (-0.1% 🔽)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (gzipped) 67.44 KB (-0.09% 🔽)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (gzipped) 33.32 KB (-0.15% 🔽)
@sentry/browser - ES6 CDN Bundle (gzipped) 24.51 KB (-0.03% 🔽)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (minified & uncompressed) 212.52 KB (-0.02% 🔽)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (minified & uncompressed) 100.53 KB (-0.04% 🔽)
@sentry/browser - ES6 CDN Bundle (minified & uncompressed) 73.44 KB (+0.13% 🔺)
@sentry/browser (incl. Tracing) - ES5 CDN Bundle (gzipped) 36.4 KB (-0.18% 🔽)
@sentry/react (incl. Tracing, Replay) - Webpack (gzipped) 69.71 KB (-0.08% 🔽)
@sentry/react - Webpack (gzipped) 22.63 KB (-0.02% 🔽)
@sentry/nextjs Client (incl. Tracing, Replay) - Webpack (gzipped) 86.38 KB (-0.05% 🔽)
@sentry/nextjs Client - Webpack (gzipped) 50.69 KB (-0.08% 🔽)
@sentry-internal/feedback - Webpack (gzipped) 17.21 KB (-0.04% 🔽)

mydea
mydea reviewed Jan 23, 2024
View reviewed changes
@lforst lforst force-pushed the lforst-isolation-scope-propagation branch from 4e4f46e to 36d4cfe Compare January 25, 2024 13:40
@lforst lforst changed the title ref: Read propagation context off of scope and isolation scope when propagating ref: Read propagation context off of scope and isolation scope when propagating and applying trace context Jan 25, 2024
@lforst lforst marked this pull request as ready for review January 25, 2024 15:06
@lforst lforst requested a review from mydea January 25, 2024 15:06
@lforst lforst assigned Lms24 and AbhiPrasad and unassigned Lms24 and AbhiPrasad Jan 25, 2024
@lforst lforst requested review from Lms24 and AbhiPrasad January 25, 2024 16:08
mydea
mydea reviewed Jan 29, 2024
View reviewed changes
packages/node/src/integrations/undici/index.ts
const sentryBaggageHeader = dynamicSamplingContextToSentryBaggageHeader(dynamicSamplingContext);
setHeadersOnRequest(request, sentryTrace, sentryBaggageHeader);
}
const { traceId, spanId, sampled, dsc } = {
Copy link
Member

Choose a reason for hiding this comment

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

l: It feels like maybe we could extract this into a util 🤔 as this appears to be the same as for http? (and maybe others)

mydea
mydea reviewed Jan 29, 2024
View reviewed changes
packages/types/src/tracing.ts
@@ -2,10 +2,43 @@ import type { DynamicSamplingContext } from './envelope';

export type TracePropagationTargets = (string | RegExp)[];

/**
Copy link
Member

Choose a reason for hiding this comment

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

Great comments ❤️

mydea
mydea approved these changes Jan 29, 2024
View reviewed changes
Copy link
Member

@mydea mydea left a comment

Choose a reason for hiding this comment

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

great work on that - it is always a bit hard to follow along everywhere and ensure the behavior is "expected" but this seems to me like a massive simplification & streamlining of all of that.

@lforst lforst force-pushed the lforst-isolation-scope-propagation branch from 0f28ba7 to 3314892 Compare January 29, 2024 13:20
@lforst lforst merged commit b3af0dc into develop Jan 29, 2024
@lforst lforst deleted the lforst-isolation-scope-propagation branch January 29, 2024 14:35
@lforst lforst mentioned this pull request Jan 29, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mydea mydea mydea approved these changes

@AbhiPrasad AbhiPrasad AbhiPrasad approved these changes

@Lms24 Lms24 Awaiting requested review from Lms24

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

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