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(normalization): Mark scrubbed transactions as sanitized #1960

Merged
merged 4 commits into from
Mar 23, 2023

Conversation

iker-barriocanal
Copy link
Contributor

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

All transactions that got their names scrubbed will be marked as sanitized, as long as the org feature flag is enabled.

6961c5e includes a small refactor around the usage of feature flags. Although it may seem like it, this doesn't change the behavior of relay around the application of transaction rules, since the initial flag belonging to scrub_identifiers controls the generation of rules in sentry, see.

Ref: https://github.com/getsentry/team-ingest/issues/93

#skip-changelog

All transactions that got their names scrubbed will be marked as
sanitized, as long as the org feature flag is enabled.
@iker-barriocanal iker-barriocanal requested a review from a team March 22, 2023 13:40
@iker-barriocanal iker-barriocanal self-assigned this Mar 22, 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.

@iker-barriocanal did you check if affected orgs currently have any high-cardinality transactions that would destroy the UX with this change?

relay-general/src/store/transactions/processor.rs Outdated Show resolved Hide resolved
@iker-barriocanal
Copy link
Contributor Author

@jjbayer the list of orgs affected by the feature flag is here. I think it's fine, but we can also consider limiting the flag to Sentry orgs for now.

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.

I think it's fine, but we can also consider limiting the flag to Sentry orgs for now.

@iker-barriocanal My question was specifically whether or not the affected org(s) still had a lot of URL transactions with identifiers that we failed to parameterize. In the mean time I found the answer myself: the affected customer project only has a handful of high-volume but low-cardinality transactions labeled as url, so we can go ahead with the change.

relay-general/src/store/transactions/processor.rs Outdated Show resolved Hide resolved
@iker-barriocanal
Copy link
Contributor Author

@jjbayer I checked the projects for that org and sentry org and I believe it's fine. I haven't checked other sentry orgs, but for the purpose of this dogfooding I expect no issues. I should have written a more elaborated answer from the beginning.

@iker-barriocanal iker-barriocanal merged commit 606166f into master Mar 23, 2023
@iker-barriocanal iker-barriocanal deleted the iker/feat/sanitize-scrubbed-txs branch March 23, 2023 15:06
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)
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.

2 participants