Skip to content

Diagnostics: Anonymize events#782

Merged
tonidero merged 5 commits into
diagnosticsfrom
telemetry-anonymization
Feb 16, 2023
Merged

Diagnostics: Anonymize events#782
tonidero merged 5 commits into
diagnosticsfrom
telemetry-anonymization

Conversation

@tonidero

@tonidero tonidero commented Feb 9, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Description

Based on #785
Deals with CSDK-652

This PR introduces some basic anonymization guardrails for diagnostics events. Currently we remove emails, uuids and ips from exception error messages, the only arbitrary piece of data.

@tonidero tonidero added pr:feat A new feature WIP labels Feb 9, 2023
@tonidero tonidero changed the title Anonymize diagnostics data Diagnostics: Anonymize events Feb 9, 2023
@tonidero tonidero mentioned this pull request Feb 9, 2023
Closed
2 tasks
tonidero
tonidero commented Feb 9, 2023
View reviewed changes
Comment thread common/src/main/java/com/revenuecat/purchases/common/diagnostics/DiagnosticsAnonymizer.kt Outdated

@tonidero tonidero Feb 9, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If we end up sending logs, we will need to anonymize more data, but for now, metrics don't really have any place that we could miss to send PII

@tonidero tonidero requested a review from a team February 9, 2023 11:06
@codecov

codecov Bot commented Feb 9, 2023 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

❗ No coverage uploaded for pull request base (diagnostics@366b0c8). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head 9b1d91b differs from pull request most recent head cb6a088. Consider uploading reports for the commit cb6a088 to get more accurate results

@@              Coverage Diff               @@
##             diagnostics     #782   +/-   ##
==============================================
  Coverage               ?   81.96%           
==============================================
  Files                  ?      128           
  Lines                  ?     4148           
  Branches               ?      524           
==============================================
  Hits                   ?     3400           
  Misses                 ?      542           
  Partials               ?      206

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

NachoSoto
NachoSoto reviewed Feb 9, 2023
View reviewed changes
Comment thread purchases/src/main/kotlin/com/revenuecat/purchases/PurchasesFactory.kt Outdated

@NachoSoto NachoSoto Feb 9, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice. Would this run in a background thread? Probably wouldn't want to do a bunch of regex checks in the UI thread.

@tonidero tonidero Feb 9, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yup all the code related to diagnostics will run in a background thread. That part will be added in #785 and a future PR to pass in the diagnostics thread.

@tonidero tonidero force-pushed the telemetry-anonymization branch from 9d5d87a to 39cc5b2 Compare February 9, 2023 15:58
@tonidero tonidero changed the base branch from telemetry to diagnostics-manager February 9, 2023 15:59
@tonidero tonidero marked this pull request as ready for review February 9, 2023 15:59
@tonidero tonidero requested a review from a team February 9, 2023 15:59
@tonidero tonidero force-pushed the diagnostics-manager branch from 2c34b06 to a6873d9 Compare February 10, 2023 11:25
@tonidero tonidero force-pushed the telemetry-anonymization branch from 4d73955 to 0bfa010 Compare February 10, 2023 11:27
@tonidero tonidero added refactor and removed pr:feat A new feature WIP labels Feb 10, 2023
@tonidero tonidero mentioned this pull request Feb 10, 2023
Merged
NachoSoto
NachoSoto approved these changes Feb 14, 2023
View reviewed changes
Comment thread common/src/main/java/com/revenuecat/purchases/common/Anonymizer.kt Outdated
Comment thread common/src/main/java/com/revenuecat/purchases/common/Anonymizer.kt Outdated
Comment thread common/src/main/java/com/revenuecat/purchases/common/diagnostics/DiagnosticsAnonymizer.kt Outdated

@NachoSoto NachoSoto Feb 14, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is really cool.

Comment thread common/src/test/java/com/revenuecat/purchases/common/AnonymizerTest.kt Outdated
Comment thread common/src/test/java/com/revenuecat/purchases/common/AnonymizerTest.kt Outdated

@NachoSoto NachoSoto Feb 14, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍🏻 I was gonna suggest having a relatively complex e-mail to cover the fact that you used a good regex.
It's probably not perfect, but this is probably covering 99% of e-mails.

Comment thread common/src/test/java/com/revenuecat/purchases/common/AnonymizerTest.kt Outdated

@NachoSoto NachoSoto Feb 14, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍🏻

Comment thread common/src/test/java/com/revenuecat/purchases/common/AnonymizerTest.kt Outdated

@NachoSoto NachoSoto Feb 14, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Something that this doesn't do is anonymize recursively. Can events have nested Maps?

@tonidero tonidero Feb 15, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That's a good point. We are still finishing the API but I'd vote against it, since it could get pretty complex to query... In any case, I can add that functionality just in case since it shouldn't be too hard.

Comment thread common/src/test/java/com/revenuecat/purchases/common/diagnostics/DiagnosticsAnonymizerTest.kt Outdated

@NachoSoto NachoSoto Feb 14, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

All your tests are super clear and maintainable 👏🏻

@aboedo aboedo Feb 14, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I was thinking the same, these tests are wonderful

aboedo
aboedo approved these changes Feb 14, 2023
View reviewed changes
Comment thread common/src/main/java/com/revenuecat/purchases/common/Anonymizer.kt Outdated
Comment thread common/src/test/java/com/revenuecat/purchases/common/diagnostics/DiagnosticsAnonymizerTest.kt Outdated

@aboedo aboedo Feb 14, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I was thinking the same, these tests are wonderful

@tonidero tonidero force-pushed the diagnostics-manager branch from a6873d9 to 51d8760 Compare February 15, 2023 08:39
@tonidero tonidero force-pushed the telemetry-anonymization branch from 9b1d91b to 655caea Compare February 15, 2023 09:07
Base automatically changed from diagnostics-manager to diagnostics February 16, 2023 08:58
@tonidero tonidero force-pushed the telemetry-anonymization branch from 9ba43aa to cb6a088 Compare February 16, 2023 09:00
@tonidero tonidero merged commit a3dd8e5 into diagnostics Feb 16, 2023
@tonidero tonidero deleted the telemetry-anonymization branch February 16, 2023 09:08
tonidero added a commit that referenced this pull request Feb 28, 2023
@tonidero
Diagnostics: Anonymize events (#782)
5f6e061
tonidero added a commit that referenced this pull request Feb 28, 2023
@tonidero
Diagnostics: Anonymize events (#782)
16baa29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aboedo aboedo aboedo approved these changes
+1 more reviewer
@NachoSoto NachoSoto NachoSoto approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@tonidero @NachoSoto @aboedo