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

Remove refcode field from P3A messages #12478

Merged
merged 2 commits into from
Mar 7, 2022
Merged

Remove refcode field from P3A messages #12478

merged 2 commits into from
Mar 7, 2022

Conversation

rillian
Copy link
Contributor

@rillian rillian commented Mar 3, 2022

Stop reporting refcode with P3A messages since we don't need it for analysis

  • Report none in the protobuf metadata string so there's something for the backend to parse
  • Don't report the field at all in json-format messages.

Resolves brave/brave-browser#21460

Submitter Checklist:

  • I confirm that no security/privacy review is needed, or that I have requested one
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

Examine P3A network traffic from the browser

  • Messages to the p3a.brave.com endpoint should always list none in the refcode field, which is the second-to-last comma-separated value at the end of the base64-coded measurement string.
  • Messages to the p3a-json.brave.com endpoint should not contain a refcode field at all.

To test a fresh profile:

  • Start the client
  • Wait ~10 minutes until a number of P3A reports are sent.

To test an established profile

  • Set up a test profile with referral code
  • Set the clock forward past midnight on a future Sunday evening to trigger a new P3A reporting period.

rillian added 2 commits March 3, 2022 13:13
Always rewrite the `refcode` metadata field attached to P3A reports
to "none". We no longer need this analysis column for P3A questions
so we shouldn't collect it.

We continue to report a value as part of the metadata string to
avoid imposing a format change on the analysis back-end code.
We no longer need to report this field, so remove it from the
dict submitted in json format entirely.
@rillian rillian requested review from DJAndries and porteron March 3, 2022 21:25
@rillian rillian requested a review from iefremov as a code owner March 3, 2022 21:25
@rillian rillian self-assigned this Mar 3, 2022
Copy link
Member

@porteron porteron left a comment

Choose a reason for hiding this comment

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

👌

const std::string& country = meta->country_code;
constexpr char kRefcodeNone[] = "none";
constexpr char kCountryOther[] = "other";
constexpr char kRefcodeOther[] = "other";

static base::flat_set<std::string> const kLinuxCountries(
{"US", "FR", "DE", "GB", "IN", "BR", "PL", "NL", "ES", "CA", "IT", "AU",
Copy link
Contributor

@iefremov iefremov Mar 7, 2022

Choose a reason for hiding this comment

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

we probably could revise the other lists as well, most probably things changed since 2018 :)

@rillian rillian merged commit 54dd861 into master Mar 7, 2022
@rillian rillian deleted the p3a-no-refcode branch March 7, 2022 19:17
@rillian rillian added this to the 1.38.x - Nightly milestone Mar 7, 2022
rillian added a commit that referenced this pull request Mar 7, 2022
When the refcode field was removed, special-case logic which allowed
the reporting the "US" country code as part of P3A message metadata
was unintentionally removed.

Since the US has many users, it's useful to segment usage data by
that region, and there is no privacy advantage to stripping that
country code.

Follow up to #12478
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.

Remove refcode from P3A reports
4 participants