Add Constellation support for express & slow P3A metrics #20264

DJAndries · 2023-09-23T00:55:47Z

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:

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.

components/p3a/message_manager.cc

components/p3a/star_randomness_meta.cc

components/p3a/star_randomness_points.cc

browser/brave_local_state_prefs.cc

chromium_src/chrome/browser/prefs/browser_prefs.cc

build/commands/lib/config.js

browser/ntp_background/ntp_p3a_helper_impl.cc

components/p3a/p3a_config.cc

bridiver

brave_domains url changes look ok

bridiver · 2023-11-02T18:44:14Z

patches/ios-chrome-browser-shared-model-prefs-browser_prefs.mm.patch

+                      base::Time::FromNSDate(value));
+     }
+   }
+  BRAVE_MIGRATE_OBSOLETE_LOCAL_STATE_PREFS


this can be done with a chromium_src override for MigrateObsoleteLocalStatePrefs

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?

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

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

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

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

kylehickinson

iOS++ tested locally with the BraveP3AConstellation feature flag enabled

iefremov · 2023-11-07T13:14:16Z

browser/ntp_background/ntp_p3a_helper_impl.cc

+    base::Value::Dict* inflight_dict =
+        creative_dict->FindDict(kInflightDictKey);
+    DCHECK(inflight_dict);
+    if (inflight_dict == nullptr) {


this is actually an anti-pattern, see code samples in https://chromium.googlesource.com/chromium/src/+/HEAD/styleguide/c++/checks.md

…metrics

…ust build config with new upload variable

…n metric log stores

kylehickinson · 2023-11-15T22:16:45Z

If uplifted please also uplift #21000

github-actions bot assigned DJAndries Sep 23, 2023

DJAndries force-pushed the p3a-constellation-slow-express branch 2 times, most recently from 25a8ebf to fc4a171 Compare September 28, 2023 23:26

DJAndries marked this pull request as ready for review September 28, 2023 23:27

DJAndries requested review from a team and bridiver as code owners September 28, 2023 23:27

github-actions bot reviewed Sep 28, 2023

View reviewed changes

github-actions bot added the needs-security-review label Sep 28, 2023

github-actions bot assigned goodov, iefremov and thypon Sep 28, 2023

DJAndries force-pushed the p3a-constellation-slow-express branch 2 times, most recently from 6063f8f to d06573a Compare September 29, 2023 03:42

kylehickinson reviewed Oct 3, 2023

View reviewed changes

browser/brave_local_state_prefs.cc Show resolved Hide resolved

kylehickinson reviewed Oct 3, 2023

View reviewed changes

chromium_src/chrome/browser/prefs/browser_prefs.cc Show resolved Hide resolved

DJAndries force-pushed the p3a-constellation-slow-express branch from d06573a to f63003f Compare October 13, 2023 23:07

iefremov approved these changes Oct 23, 2023

View reviewed changes

bridiver reviewed Oct 23, 2023

View reviewed changes

build/commands/lib/config.js Outdated Show resolved Hide resolved

DJAndries force-pushed the p3a-constellation-slow-express branch from f63003f to b63af35 Compare October 24, 2023 00:29

bridiver reviewed Oct 26, 2023

View reviewed changes

browser/ntp_background/ntp_p3a_helper_impl.cc Outdated Show resolved Hide resolved

bridiver reviewed Oct 26, 2023

View reviewed changes

components/p3a/p3a_config.cc Outdated Show resolved Hide resolved

bridiver reviewed Oct 26, 2023

View reviewed changes

components/p3a/p3a_config.cc Show resolved Hide resolved

bridiver approved these changes Oct 26, 2023

View reviewed changes

kylehickinson requested a review from a team as a code owner November 1, 2023 13:53

DJAndries force-pushed the p3a-constellation-slow-express branch from 2aa2eec to 92947b3 Compare November 1, 2023 21:04

bridiver reviewed Nov 2, 2023

View reviewed changes

kylehickinson approved these changes Nov 3, 2023

View reviewed changes

DJAndries force-pushed the p3a-constellation-slow-express branch from 8a9fb74 to a66b052 Compare November 7, 2023 06:11

iefremov reviewed Nov 7, 2023

View reviewed changes

iefremov approved these changes Nov 7, 2023

View reviewed changes

DJAndries and others added 11 commits November 9, 2023 15:09

Add Constellation support for express & slow P3A metrics

bdd606f

Add Constellation support for NTP-SI P3A data

e08931a

Update P3A iOS switches, add tests for monthly & daily Constellation …

1132c58

…metrics

Add more P3A slow/express unit tests, fix bugs, fix iOS switches, adj…

b32b922

…ust build config with new upload variable

Use brave_domains for default P3A hosts/URLs

ecf6a92

Register obsolete P3A STAR prefs on iOS

b82b319

[iOS] Add patch for migrating obsolete local state

5282671

Address Constellation nits/feedback

6e9723d

Fix crash during Constellation rotation scheduler initialization

0d036f6

[iOS] Update patch/chromium_src override for migrating obsolete prefs

6d56b81

Ensure Constellation and JSON creative metric values are segregated i…

055b624

…n metric log stores

DJAndries force-pushed the p3a-constellation-slow-express branch from a66b052 to 055b624 Compare November 9, 2023 23:35

Misc Constellation daily/creative fixes

1eeda31

DJAndries merged commit 3f3b71b into master Nov 11, 2023
16 checks passed

DJAndries deleted the p3a-constellation-slow-express branch November 11, 2023 00:08

github-actions bot added this to the 1.62.x - Nightly milestone Nov 11, 2023

GeetaSarvadnya mentioned this pull request Dec 15, 2023

Use Constellation for express & slow P3A metrics brave/brave-browser#33217

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Constellation support for express & slow P3A metrics #20264

Add Constellation support for express & slow P3A metrics #20264

DJAndries commented Sep 23, 2023 •

edited

Loading

bridiver left a comment

bridiver Nov 2, 2023

kylehickinson Nov 2, 2023

bridiver Nov 2, 2023

kylehickinson Nov 2, 2023

bridiver Nov 2, 2023

kylehickinson Nov 3, 2023

kylehickinson left a comment

iefremov Nov 7, 2023

kylehickinson commented Nov 15, 2023

Add Constellation support for express & slow P3A metrics #20264

Add Constellation support for express & slow P3A metrics #20264

Conversation

DJAndries commented Sep 23, 2023 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

bridiver left a comment

Choose a reason for hiding this comment

bridiver Nov 2, 2023

Choose a reason for hiding this comment

kylehickinson Nov 2, 2023

Choose a reason for hiding this comment

bridiver Nov 2, 2023

Choose a reason for hiding this comment

kylehickinson Nov 2, 2023

Choose a reason for hiding this comment

bridiver Nov 2, 2023

Choose a reason for hiding this comment

kylehickinson Nov 3, 2023

Choose a reason for hiding this comment

kylehickinson left a comment

Choose a reason for hiding this comment

iefremov Nov 7, 2023

Choose a reason for hiding this comment

kylehickinson commented Nov 15, 2023

DJAndries commented Sep 23, 2023 •

edited

Loading