-
Notifications
You must be signed in to change notification settings - Fork 900
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
Add Constellation support for express & slow P3A metrics #20264
Conversation
25a8ebf
to
fc4a171
Compare
6063f8f
to
d06573a
Compare
d06573a
to
f63003f
Compare
f63003f
to
b63af35
Compare
There was a problem hiding this comment.
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
2aa2eec
to
92947b3
Compare
base::Time::FromNSDate(value)); | ||
} | ||
} | ||
+ BRAVE_MIGRATE_OBSOLETE_LOCAL_STATE_PREFS |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
8a9fb74
to
a66b052
Compare
base::Value::Dict* inflight_dict = | ||
creative_dict->FindDict(kInflightDictKey); | ||
DCHECK(inflight_dict); | ||
if (inflight_dict == nullptr) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is actually an anti-pattern, see code samples in https://chromium.googlesource.com/chromium/src/+/HEAD/styleguide/c++/checks.md
…ust build config with new upload variable
…n metric log stores
a66b052
to
055b624
Compare
If uplifted please also uplift #21000 |
Resolves brave/brave-browser#33217
Submitter Checklist:
QA/Yes
orQA/No
;release-notes/include
orrelease-notes/exclude
;OS/...
) to the associated issuenpm run test -- brave_browser_tests
,npm run test -- brave_unit_tests
wikinpm run lint
,npm run presubmit
wiki,npm run gn_check
,npm run tslint
git rebase master
(if needed)Reviewer Checklist:
gn
After-merge Checklist:
changes has landed on
Test Plan:
Use MITM proxy when using browser.
p3a-json.brave.com
. Ensure that thecadence
field in the payload contains the correct value. If the metric is in thelogs
object (in local state), ensure the cadence istypical
. If the metric is inlogs_slow
, ensure the cadence isslow
. If the metric is inlogs_express
, ensure the cadence isexpress
. Ensure that each metric sent to the server is marked as "sent" in the local state.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 thetypical
cadence, keep an eye onlogs_constellation_prep
. For theexpress
cadence, keep an eye onlogs_constellation_prep_express
. For theslow
cadence, keep an eye onlogs_constellation_prep_slow
.collector.bsg.brave.com
. The URL path should be/<cadence type>
. Before metrics are sent to this server, they should appear underconstellation_logs
,constellation_logs_express
andconstellation_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.