-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
fix(core): Allow non-recording spans to carry explicit sampling decisions #21406
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
Open
andreiborza
wants to merge
15
commits into
develop
Choose a base branch
from
ab/nonrecording-span-sampling
base: develop
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
03c0e5f
fix(core): Allow non-recording spans to carry explicit sampling decis…
andreiborza bc7f4f9
Defer idle span sampling decision in TwP
andreiborza 0a77a65
Freeze continued DSC as is
andreiborza 4882b00
Capture scopes on the non-recording idle span placeholder
andreiborza cf23c31
Ensure TwP spans store their scope
andreiborza e03935a
Ensure parent span id is passed along
andreiborza 9a54d7a
Don't add span names to TwP DSC when source is url
andreiborza c117ab2
Fix cf tests
andreiborza 26fea7e
Ensure non-recording spans propagate incoming sampling decision
andreiborza 5d3f186
Ensure DSC stays frozen on nested TwP placeholder spans
andreiborza fd15748
Extract functionality into freezeDscOnTwpRootSpan helper
andreiborza 07dfbf4
Clarify sampling intent
andreiborza 6a476c0
test(react-router): Fix e2e tests (#21485)
chargome ba51d24
Encode span sampling decisions via trace flags and trace state
andreiborza d5887e7
Surface parent_span_id on spanToStreamedSpanJSON fallback
andreiborza File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
20 changes: 20 additions & 0 deletions
20
dev-packages/cloudflare-integration-tests/suites/tracing/dsc-url-source/index.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| import * as Sentry from '@sentry/cloudflare'; | ||
|
|
||
| interface Env { | ||
| SENTRY_DSN: string; | ||
| } | ||
|
|
||
| // Tracing is enabled (not TwP), but the route is a raw, non-parametrized URL so the | ||
| // http.server span source is `url`. The span name must therefore be omitted from the | ||
| // DSC (raw URLs may contain PII), even though a real transaction is recorded. | ||
| export default Sentry.withSentry( | ||
| (env: Env) => ({ | ||
| dsn: env.SENTRY_DSN, | ||
| tracesSampleRate: 1.0, | ||
| }), | ||
| { | ||
| async fetch(_request, _env, _ctx) { | ||
| throw new Error('Test error from URL-source worker'); | ||
| }, | ||
| }, | ||
| ); |
55 changes: 55 additions & 0 deletions
55
dev-packages/cloudflare-integration-tests/suites/tracing/dsc-url-source/test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,55 @@ | ||
| import { expect, it } from 'vitest'; | ||
| import { eventEnvelope } from '../../../expect'; | ||
| import { createRunner } from '../../../runner'; | ||
|
|
||
| it('omits the span name from the DSC for url-source spans when tracing is enabled', async ({ signal }) => { | ||
| const runner = createRunner(__dirname) | ||
| // Error event: because tracing is enabled, the DSC carries the sampling fields. But the span | ||
| // source is `url`, so the span name is omitted from the DSC (raw URLs may contain PII). | ||
| .expect( | ||
| eventEnvelope( | ||
| { | ||
| level: 'error', | ||
| exception: { | ||
| values: [ | ||
| { | ||
| type: 'Error', | ||
| value: 'Test error from URL-source worker', | ||
| stacktrace: { | ||
| frames: expect.any(Array), | ||
| }, | ||
| mechanism: { type: 'auto.http.cloudflare', handled: false }, | ||
| }, | ||
| ], | ||
| }, | ||
| request: { | ||
| headers: expect.any(Object), | ||
| method: 'GET', | ||
| url: expect.any(String), | ||
| }, | ||
| }, | ||
| { includeSamplingFields: true, includeSampleRand: true, includeTransaction: false }, | ||
| ), | ||
| ) | ||
| // Transaction event: proves we are NOT in TwP — the span is recorded with a `url` source and | ||
| // carries the name on the event itself, even though it is intentionally absent from the DSC. | ||
| .expect(envelope => { | ||
| const transactionEvent = envelope[1]?.[0]?.[1]; | ||
| expect(transactionEvent).toEqual( | ||
| expect.objectContaining({ | ||
| type: 'transaction', | ||
| transaction: 'GET /error', | ||
| contexts: expect.objectContaining({ | ||
| trace: expect.objectContaining({ | ||
| op: 'http.server', | ||
| data: expect.objectContaining({ 'sentry.source': 'url' }), | ||
| }), | ||
| }), | ||
| }), | ||
| ); | ||
| }) | ||
| .unordered() | ||
| .start(signal); | ||
| await runner.makeRequest('get', '/error', { expectError: true }); | ||
| await runner.completed(); | ||
| }); |
6 changes: 6 additions & 0 deletions
6
dev-packages/cloudflare-integration-tests/suites/tracing/dsc-url-source/wrangler.jsonc
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| { | ||
| "name": "worker-name", | ||
| "compatibility_date": "2025-06-17", | ||
| "main": "index.ts", | ||
| "compatibility_flags": ["nodejs_compat"], | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
super-l: Not a fan of this tbh as it still limits what we're expecting in the specific values. Why not directly assert on the envelope headers?
This is fine for the PR though, so no need to change it :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, it would be better to assert actual values.
I had a go at this but it snowballs quite a bit so I'll extract that out into a separate PR later.