Skip to content
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

feat(pii-scrubbing): PII scrub span.data by default #1953

Merged
merged 7 commits into from
Mar 22, 2023

Conversation

iker-barriocanal
Copy link
Contributor

@iker-barriocanal iker-barriocanal commented Mar 20, 2023

This PR reverts #1889 and enables PII data scrubbing of span.data by default. For the context of the reason for the revert, see #1915 (comment).

Related to: #1955.
Ref: #1855, #1915.

@iker-barriocanal iker-barriocanal requested a review from a team March 20, 2023 19:52
@iker-barriocanal iker-barriocanal self-assigned this Mar 20, 2023
Copy link
Member

@jjbayer jjbayer left a comment

Choose a reason for hiding this comment

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

LGTM, @iker-barriocanal could you post in the client infra channel when this goes live? Just so everybody's aware that they will have to start explicitly typing fields in span.data in Relay if the new behavior breaks something for them.

@iker-barriocanal iker-barriocanal merged commit 011e56b into master Mar 22, 2023
@iker-barriocanal iker-barriocanal deleted the iker/feat/scrub-http-breadcrumb-span branch March 22, 2023 10:01
jan-auer added a commit that referenced this pull request Mar 24, 2023
* master:
  feat(normalization): Mark scrubbed transactions as sanitized (#1960)
  fix(pii): Scrub sensitive cookies (#1951)
  release: 23.3.1
  feat(pii-scrubbing): PII scrub span.data by default (#1953)
  test(scrubbing): Add tests for PII scrubbing in breadcrumb.data (#1955)
  build(deps): bump sentry-sdk from 1.11.0 to 1.14.0 (#1959)
  ref(envelope_manager): Remove from_registry calls from the service (#1956)
  cd: add placeholder deployment pipeline (#1954)
  Assert array fields are capped to 100 items (#1910)
  fix(pii): Early return if no text left (#1957)
@cleptric cleptric mentioned this pull request Apr 17, 2023
3 tasks
jjbayer added a commit that referenced this pull request Oct 4, 2023
Up until now, we've materialized span tags into `span.data`. This
creates several problems:

* Potential clashes between keys set by the user and keys set by sentry.
* Pollution of the UI because `span.data` entries show up in the event
details view.
* Most importantly: `span.data` is [subject to PII
scrubbing](#1953), so tags
computed by Relay are sometimes overwritten by PII scrubbing in the next
Relay instance.

Instead of writing into `span.data`, create a new top-level object in
the span called `sentry_tags`. The same naming has been used
[here](https://github.com/getsentry/sentry-kafka-schemas/blob/79fb0900e3a9a4da6f0db15eab1b5d27f42ffeb7/schemas/snuba-spans.v1.schema.json#L81-L83).

For now, double-write span tags into both `span.data` and
`span.sentry_tags` until all users of these tags have switched to
`sentry_tags`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants