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(replays): Use user configured PII scrubbers #1731

Merged
merged 4 commits into from
Feb 3, 2023

Conversation

cmanallen
Copy link
Member

@cmanallen cmanallen commented Jan 9, 2023

Removes statically defined scrubbers with data scrubbing and PII config from the project state. This includes a refactor that moves replay scrubbing on a struct.

closes: https://github.com/getsentry/replay-backend/issues/243

@cmanallen cmanallen requested a review from a team January 9, 2023 14:50
@cmanallen
Copy link
Member Author

@JoshFerge

@cmanallen cmanallen requested a review from JoshFerge January 17, 2023 20:03
@jan-auer
Copy link
Member

jan-auer commented Feb 1, 2023

This will have to be updated to #1800 and requires a bit of cleanup in the StringTranscoder. I'll look into this.

@jan-auer jan-auer marked this pull request as draft February 1, 2023 12:47
@jan-auer jan-auer self-assigned this Feb 1, 2023
@jan-auer jan-auer force-pushed the replays-use-user-configured-scrubbers branch from fbd0c9a to fb0ad6f Compare February 2, 2023 11:06
@jan-auer jan-auer marked this pull request as ready for review February 2, 2023 11:07
* master:
  feat(metrics): Count transactions toward root project [TET-627] (#1734)
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.

The implementation looks good to me, just one remark about the test config.

Comment on lines 180 to 184
fn default_pii_config() -> PiiConfig {
let mut pii_config = PiiConfig::default();
pii_config.applications = [(SelectorSpec::And(vec![]), vec!["@common".to_string()])].into();
pii_config
}
Copy link
Member

Choose a reason for hiding this comment

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

To have the tests resemble the default production use case, I would use a configuration derived from DataScrubbingConfig here, like for example

fn simple_enabled_config() -> DataScrubbingConfig {
let mut scrub_config = DataScrubbingConfig::default();
scrub_config.scrub_data = true;
scrub_config.scrub_defaults = true;
scrub_config.scrub_ip_addresses = true;
scrub_config
}

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, updated. This actually shows us that the filter has changed.

@cmanallen note that email addresses aren't being scrubbed anymore, apparently the default data scrubbers don't do this. No action item imho, this is in line with the other data scrubbers.

@jan-auer jan-auer requested a review from jjbayer February 2, 2023 13:42
@cmanallen
Copy link
Member Author

@jjbayer @jan-auer Should this be merged?

@jan-auer
Copy link
Member

jan-auer commented Feb 3, 2023

Yes. I was waiting for a response on #1731 (comment).

Merging now.

@jan-auer jan-auer merged commit 44fbca1 into master Feb 3, 2023
@jan-auer jan-auer deleted the replays-use-user-configured-scrubbers branch February 3, 2023 08:50
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.

3 participants