feat(core): Data Collection - #5759
Conversation
|
📲 Install BuildsAndroid
|
There was a problem hiding this comment.
Reviewed the full Data Collection stack tip (fix/data-collection-opentelemetry-span-description) against data-collection 0.11.0 and the landed/in-flight peers (JS, Python, Cocoa, Ruby, Go).
What looks solid
- Config surface matches the required core:
userInfo,cookies/httpHeaders/urlQueryParamskey-value modes, directionalhttpBodies,graphQL,databaseQueryData. - Explicit-namespace resolution matches peers: once any
dataCollectionfield is set (or an emptyDataCollectionis assigned), omitted fields take spec defaults, notsendDefaultPii. That matches JSresolveDataCollectionOptionsand Go/Ruby “opt into defaults”. - Built-in sensitive terms match the denylist; partial case-insensitive matching and
[Filtered]substitution look correct inHttpUtils. - Integration wiring for Spring/Servlet/OkHttp/Ktor/Apollo/GraphQL is real and tested; Replay kept independent (spec 0.10.0); Android installation ID restored as non-
userInfoidentity (pragmatic, well motivated). - Span descriptions stripping queries (Ktor/OTel) matches the structuring-data rule.
Please fix before merge
1. Legacy path still ships sensitive query values (and dual-path bypasses the denylist)
Spec: sensitive key-value data MUST be replaced with [Filtered] under automatic collection. Peers always run collection through a resolved policy (JS bridges sendDefaultPii → urlQueryParams: true / deny-with-PII-snippets, then filters; Go/Python/Cocoa same idea).
Java does not:
// UrlUtils.filterQueryParams
return resolver.isDataCollectionConfigured()
? HttpUtils.filterQueryParams(query, resolver.getUrlQueryParams())
: query; // raw, including token=secretThis is covered as intentional in UrlUtilsTest (preserve legacy query values), and HTTP integrations fork the same way (isDataCollectionConfigured → new filter, else sendDefaultPii / old header drop). With default options (dataCollection absent, sendDefaultPii=false), request/span/breadcrumb query strings still attach token=secret whenever a query is collected.
Same dual-path issue on cookies: e.g. OkHttp legacy sendDefaultPii=true returns the raw cookie string with no sensitive denylist pass (Spring at least runs filterOutSecurityCookies*).
Fix shape: always resolve an effective KeyValueCollectionBehavior (spec default deny-list, or a legacy bridge like JS defaultPiiToCollectionOptions) and run all query/cookie/header collection through HttpUtils.filter*. Keep category on/off and PII-term bridges in the resolver; do not bypass scrubbing when the namespace is unset.
2. Public databaseQueryData is a no-op
DataCollection / DataCollectionResolver.isDatabaseQueryData() are public and tested, but tip-of-stack has zero JDBC/SQLite (or other) call sites. #5801’s enforcement was dropped (Preserve query descriptions), so users can set the option with no effect.
Spec note: databaseQueryData gates bound params / payloads / results, not necessarily sanitized db.query.text. If Java cannot separate literals safely, either:
- drop the public option until there is a real consumer, or
- document + enforce a concrete policy in
sentry-jdbc/sentry-android-sqlite(and only suppress what the option is defined to control).
3. Non-Spring config surfaces still only know send-default-pii
Spring Boot binding works via SentryProperties extends SentryOptions + mutable KeyValueCollectionBehavior (#5834). ExternalOptions / sentry.properties and Android ManifestMetadataReader still only map send-default-pii. Peers expose dataCollection on their primary config path. At least property-file (and ideally manifest) keys for the new namespace should land with the stack, or the gap should be explicit in the PR/docs so hybrid and non-Spring users are not stuck on the legacy flag.
Spec / peer deltas (non-blocking, track explicitly)
| Area | Java stack | Spec / peers |
|---|---|---|
genAI, queues, stackFrameVariables, frameContextLines |
omitted | required where platform collects that data; OK to defer if unused |
graphql naming |
getGraphql() |
spec graphQL; Ruby/Python also snake/lower — fine if documented |
Boolean shorthand for key-value (true/false) |
modes + factories only | JS/Ruby accept bool shorthand — nice-to-have |
| Docs / wizard opt-out snippet | not in stack | spec MUST for init snippets |
| Major-version default flip | stay on sendDefaultPii until explicit dataCollection |
same staged approach as JS pre-v11 / Cocoa v10 |
Bottom line
Architecture and explicit-namespace behavior are in good shape and aligned with other SDKs. I would not merge until (1) sensitive scrubbing cannot be skipped on the legacy path, and (2) databaseQueryData is either enforced or removed from the public API. (3) is strongly preferred in the same release train as the feature.
Happy to re-review after those land on the tip branch.
PR Stack (Data Collection)
📜 Description
Collection PR for the Data Collection stack. The individual PRs add the configuration model, resolution and compatibility bridge, external configuration, filtering, and integration enforcement.
Squash-merge this PR into
mainonly after every stack PR has been merged into this branch using merge commits.💡 Motivation and Context
Introduce the specification-defined
dataCollectionconfiguration while preserving existingsendDefaultPiibehavior for users who do not opt into the new namespace.Refs #5666
💚 How did you test it?
This collection branch contains only an empty commit. Each stack PR carries its own tests.
📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps
Merge the Data Collection stack into this branch in order, then squash-merge this PR into
main.#skip-changelog