Add Constellation support for express & slow P3A metrics by DJAndries · Pull Request #20264 · brave/brave-core (original) (raw)

Resolves brave/brave-browser#33217

Submitter Checklist:

I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
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 wiki
- npm run lint, npm run presubmit wiki, 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:

The associated issue milestone is set to the smallest version that the
changes has landed on
All relevant documentation has been updated, for instance:

Test Plan:

Use MITM proxy when using browser.

Start with fresh profile.
Ensure JSON metrics are sent as usual to p3a-json.brave.com. Ensure that the cadence field in the payload contains the correct value. If the metric is in the logs object (in local state), ensure the cadence is typical. If the metric is in logs_slow, ensure the cadence is slow. If the metric is in logs_express, ensure the cadence is express. Ensure that each metric sent to the server is marked as "sent" in the local state.
Ensure requests are being made to star-randsrv.bsg.brave.com to prepare Constellation metrics. Ensure that the URL path is /instances/<cadence type>/randomness. When one request is sent for a particular cadence, check to see that metrics are being marked as "sent" as they are being sent to the server, by checking the correct "logs" object in local state. For the typical cadence, keep an eye on logs_constellation_prep. For the express cadence, keep an eye on logs_constellation_prep_express. For the slow cadence, keep an eye on logs_constellation_prep_slow.
Ensure final measurements are being sent to collector.bsg.brave.com. The URL path should be /<cadence type>. Before metrics are sent to this server, they should appear under constellation_logs, constellation_logs_express and constellation_logs_slow after being prepared (from the requests being made in step 2). As soon as the metrics are sent to the collector, the metric should disappear in the relevant local state object.

[ github-actions[bot] ](/apps/github-actions)

Choose a reason for hiding this comment

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

brave_domains url changes look ok

Choose a reason for hiding this comment

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

this can be done with a chromium_src override for MigrateObsoleteLocalStatePrefs

Choose a reason for hiding this comment

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

Since this is how we've managed the overrides for the other 3 methods can we keep this for now and then I'll open a new PR removing the patch entirely after this merges?

Choose a reason for hiding this comment

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

I think it wasn't possible in the other cases because the method name wasn't unique in the file. This is a very simple change to a chromium_src override

Choose a reason for hiding this comment

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

Yeah I just double checked, can only be done for MigrateObsoleteLocalStatePrefs and MigrateObsoleteBrowserStatePrefs. I'll update both

Choose a reason for hiding this comment

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

there's probably still another way to make it work without patches in the other cases, but for now I just want to make sure we're not adding new patches

Choose a reason for hiding this comment

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

For the time being, replaced the migrate methods over in this PR: 8a9fb74

Choose a reason for hiding this comment

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

iOS++ tested locally with the BraveP3AConstellation feature flag enabled

Choose a reason for hiding this comment

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

…ust build config with new upload variable

DJAndries deleted the p3a-constellation-slow-express branch

November 11, 2023 00:08

If uplifted please also uplift #21000

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})