fix(tracing): Handle traces_sampler raising by falling back#6850
Closed
ericapisani wants to merge 46 commits into
Closed
fix(tracing): Handle traces_sampler raising by falling back#6850ericapisani wants to merge 46 commits into
ericapisani wants to merge 46 commits into
Conversation
…n config `_filter_headers` previously used a hardcoded sensitive-header tuple and a `send_default_pii`/`use_annotated_value` toggle. It now delegates to `_apply_key_value_collection_filtering` from `sentry_sdk.data_collection`, so header scrubbing respects the new `data_collection.http_headers.request` allowlist/denylist/off configuration. Cookie and set-cookie headers are always redacted regardless of mode. Drops the now-unused `use_annotated_value` parameter from all call sites. Work to scrub cookies in a more granular way will be tackled as part of PY-2581/#6741. Fixes PY-2584 Fixes #6744
… _experiments property in the client
…still being needed for the url attribute
…ures The new lambda_functions_with_embedded_sdk fixture directories were missing the .gitignore that the other fixtures use to keep everything except index.py untracked. As a result, certifi and urllib3 packages installed by the test setup got committed, and ruff failed CI linting against them since they're unmodified third-party code. Add the missing .gitignore to each new fixture directory and remove the committed vendored packages; they are regenerated automatically at test time via `uv pip install --target`.
…tarlette, litestar, starlite Extends the granular cookie collection controls (data_collection.cookies) to _wsgi_common, starlette, litestar, and starlite, matching the behavior already used elsewhere. Falls back to should_send_default_pii() when data_collection is not configured for cookies. HTTP "Cookie" and "set-cookie" headers will continue to be completely filtered with the "[Filtered]" value. Fixes PY-2581 Fixes #6741
…de is off Previously the async request extractors attached an empty cookies dict when the data_collection cookies mode was off, while sync route handlers omitted it entirely. Make all integrations consistent by not attaching the cookies field at all when filtering yields no cookies.
…n config `_filter_headers` previously used a hardcoded sensitive-header tuple and a `send_default_pii`/`use_annotated_value` toggle. It now delegates to `_apply_key_value_collection_filtering` from `sentry_sdk.data_collection`, so header scrubbing respects the new `data_collection.http_headers.request` allowlist/denylist/off configuration. Cookie and set-cookie headers are always redacted regardless of mode. Drops the now-unused `use_annotated_value` parameter from all call sites. Work to scrub cookies in a more granular way will be tackled as part of PY-2581/#6741. Fixes PY-2584 Fixes #6744
… _experiments property in the client
…still being needed for the url attribute
…ures The new lambda_functions_with_embedded_sdk fixture directories were missing the .gitignore that the other fixtures use to keep everything except index.py untracked. As a result, certifi and urllib3 packages installed by the test setup got committed, and ruff failed CI linting against them since they're unmodified third-party code. Add the missing .gitignore to each new fixture directory and remove the committed vendored packages; they are regenerated automatically at test time via `uv pip install --target`.
…ntry/sentry-python into py-2584-update-wsgi-filter-headers
…tarlette, litestar, starlite Extends the granular cookie collection controls (data_collection.cookies) to _wsgi_common, starlette, litestar, and starlite, matching the behavior already used elsewhere. Falls back to should_send_default_pii() when data_collection is not configured for cookies. HTTP "Cookie" and "set-cookie" headers will continue to be completely filtered with the "[Filtered]" value. Fixes PY-2581 Fixes #6741
…de is off Previously the async request extractors attached an empty cookies dict when the data_collection cookies mode was off, while sync route handlers omitted it entirely. Make all integrations consistent by not attaching the cookies field at all when filtering yields no cookies.
…into py-2581-cookies
The streaming path no longer emits a client span when there is no current span (#6810), so unpack only the server span.
Filter WSGI request event and span query strings through the data_collection.url_query_params behaviour (denylist/allowlist/off) instead of gating query string capture solely on send_default_pii. This brings WSGI-based integrations in line with the data collection spec's handling of cookies and headers. Rename the data_collection field query_params to url_query_params to match the updated data collection spec, which distinguishes URL query params from other key-value data (cookies, headers). Update the DataCollection/DataCollectionUserOptions TypedDicts, resolution logic in data_collection.py, and all WSGI/Flask/Django test coverage accordingly. Refs PY-2583
…orm with what is shown in the data collection spec
Filter query string parameters in the ASGI request data and span attributes according to the data_collection.url_query_params config, mirroring the existing WSGI behaviour. Falls back to the legacy send_default_pii check when data_collection is not enabled. Refs PY-2583 Refs #6743
Use the new data_collection settings to filter url.query and related span attributes for both the aiohttp server and client integrations, falling back to should_send_default_pii() when data_collection is not configured. Refs PY-2583 Refs #6743
Rebuild url.full from the parsed base URL plus the filtered query string when data_collection is enabled, so filtered/redacted query params are reflected in url.full instead of the raw request query. Previously the unfiltered query string leaked into url.full even when data_collection filtering was active. Update the span-streaming tests to expect the filtered query in url.full (and a bare path when filtering strips all params). Refs PY-2583 Refs #6743
Replace .format(SENSITIVE_DATA_SUBSTITUTE) calls with hard-coded [Filtered] strings in the query param data collection test cases for clearer expectations. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Filter query string parameters in both span attributes (url.full, http.query) and event request_info according to the configured data_collection.url_query_params behaviour, matching the pattern already applied to wsgi, asgi, aiohttp, and tornado. Refs PY-2583 Refs #6743
Refs PY-2583 Refs #6743
Refs PY-2583 Refs #6743
Apply data_collection.user_info filtering to REMOTE_ADDR/client.address, falling back to send_default_pii when data_collection is not configured. Also convert earlier query parameter data collection tests to end-to-end assertions via a real ASGI app. Ref PY-2586 Ref #6746
When a user-provided traces_sampler callback raises, catch the exception, log a warning, and fall back to the parent sample rate or the configured traces_sample_rate instead of propagating the error. Refs PY-2617 Refs #6849
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 4 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit b308065. Configure here.
| attributes["client.address"] = ip | ||
| elif should_send_default_pii(): | ||
| ip = _get_ip(asgi_scope) | ||
| attributes["client.address"] = ip |
There was a problem hiding this comment.
Unbound client_options on non-HTTP ASGI
High Severity
In _get_request_attributes, client_options is only assigned inside the http/websocket branch, but later used when attaching client.address. For other ASGI scope types that still carry a client, that read raises UnboundLocalError and can break span attribute collection. _get_request_data already assigns client_options before the type check.
Reviewed by Cursor Bugbot for commit b308065. Configure here.
| parts.append(f"{key}={item}") | ||
| return "&".join(parts) | ||
|
|
||
| return None |
There was a problem hiding this comment.
Query string rebuilt without encoding
Medium Severity
_apply_data_collection_filtering_to_query_string parses with parse_qs then rejoins as raw key=value pairs without quoting. Values containing reserved characters such as spaces, &, or = are corrupted, so filtered query_string / url.query attributes can be invalid or misleading.
Reviewed by Cursor Bugbot for commit b308065. Configure here.
| sampling_context["parent_sampled"] | ||
| if sampling_context["parent_sampled"] is not None | ||
| else client.options["traces_sample_rate"] | ||
| ) |
There was a problem hiding this comment.
Sampler fallback ignores parent rate
Medium Severity
When traces_sampler raises, the Transaction path falls back to parent_sampled (a boolean) rather than the parent’s actual sample rate. The streaming path uses _get_fallback_sample_rate, which prefers the baggage sample rate. The same failure can therefore sample at 100% or 0% in one path and at the true parent rate in the other.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit b308065. Configure here.
|
|
||
| attributes["url.full"] = get_request_url(environ, use_x_forwarded_for) | ||
|
|
||
| elif should_send_default_pii(): |
There was a problem hiding this comment.
WSGI omits client address under data collection
Medium Severity
In WSGI _get_request_attributes, the data_collection branch sets query/path/url attributes but never sets client.address from user_info. The legacy PII branch still does, and ASGI’s matching path gates client.address on data_collection["user_info"]. With data collection enabled, WSGI spans lose client IP even when user_info is true.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit b308065. Configure here.
Contributor
Codecov Results 📊✅ 94428 passed | ⏭️ 6338 skipped | Total: 100766 | Pass Rate: 93.71% | Execution Time: 332m 30s 📊 Comparison with Base Branch
All tests are passing successfully. ✅ Patch coverage is 87.36%. Project has 2506 uncovered lines. Files with missing lines (9)
Coverage diff@@ Coverage Diff @@
## main #PR +/-##
==========================================
- Coverage 89.68% 89.66% -0.02%
==========================================
Files 193 193 —
Lines 24007 24234 +227
Branches 8342 8508 +166
==========================================
+ Hits 21530 21728 +198
- Misses 2477 2506 +29
- Partials 1386 1396 +10Generated by Codecov Action |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
When a user-provided traces_sampler callback raises, catch the exception,
log a warning, and fall back to the parent sample rate or the configured
traces_sample_rate instead of propagating the error.
Refs PY-2617
Refs #6849
Stack created with GitHub Stacks CLI • Give Feedback 💬