Add Nebula support for P3A/Constellation #22522
Conversation
DJAndries
force-pushed
the
nebula-p3a
branch
2 times, most recently
from
ba21e34
to
da4b4b4
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| #include "services/network/public/cpp/shared_url_loader_factory.h" | ||
| #include "services/network/public/cpp/simple_url_loader.h" | ||
|
|
||
| namespace p3a { | ||
|
|
||
| namespace { | ||
|
|
||
| constexpr std::size_t kP3AConstellationCurrentThreshold = 50; | ||
| constexpr double kNebulaParticipationRate = 0.105; |
There was a problem hiding this comment.
It would be good to describe what these two parameters do.
There was a problem hiding this comment.
Is the ParticipationRate meant to be 1.0 in the future (after the test) or will we always keep both rates?
There was a problem hiding this comment.
The participation rate is derived from the threshold, privacy budget ε and expected population size δ. It isn't ever expected to become one, but we could of course adjust the trade-offs in the future.
| @@ -25,6 +25,9 @@ class SharedURLLoaderFactory; | |||
|
|
|||
| namespace p3a { | |||
|
|
|||
| inline constexpr size_t kConstellationDefaultThreshold = 50; | |||
There was a problem hiding this comment.
Same here, an explanation would be nice.
I think they are both the number of reports that need to be identical (i.e. k-anonymity), but one is for people who are enrolled into Nebula and the other is the current system?
There was a problem hiding this comment.
That's right. @AliShahin determined we could lower the threshold number of identical reports needed for recording under differential-privacy sampling. So this is the value used for the current system, but questions for which we turn on Nebula will use the lower threshold.
| @@ -21,6 +21,9 @@ BASE_FEATURE(kTypicalJSONDeprecation, | |||
| BASE_FEATURE(kOtherJSONDeprecation, | |||
| "BraveP3AOtherJSONDeprecation", | |||
| base::FEATURE_DISABLED_BY_DEFAULT); | |||
| BASE_FEATURE(kNebula, | |||
There was a problem hiding this comment.
this needs short explanation and / or link to a doc
There was a problem hiding this comment.
I added some doc comments in b89ce25.
| // mark request as successful to avoid transmission. | ||
| constellation_prep_log_stores_[log_type]->DiscardStagedLog(); | ||
| constellation_prep_schedulers_[log_type]->UploadFinished(true); | ||
| delegate_->OnMetricCycled(log_key, true); |
There was a problem hiding this comment.
shouldn't we return here?
There was a problem hiding this comment.
yes, good catch. thanks
There was a problem hiding this comment.
I added a return in 5da6c5f.
| constexpr double kNebulaParticipationRate = 0.105; | ||
| constexpr double kNebulaScramblingRate = 0.05; | ||
|
|
||
| bool CheckParticipationAndScrambleForNebula(std::vector<std::string>* layers) { |
There was a problem hiding this comment.
perhaps this is "MaybeScrambleForNebula" ? it feels like "check & scramble" implies both actions are done no matter what
There was a problem hiding this comment.
Changed in 1289c83 with a comment, since the function still does two different things.
| MetricLogType log_type, | ||
| uint8_t epoch, | ||
| StarRandomnessMeta* randomness_meta, | ||
| ::rust::Box<constellation::RandomnessRequestStateWrapper> |
There was a problem hiding this comment.
perhaps we can move lib.rs.h include to .cc now
There was a problem hiding this comment.
Unfortunately not yet. This file still uses types from rust cxx reflection in the public definition of StarRandomnessPoints.
|
also we could make randomness parameters configurable via Griffin (as feature parameters) |
NB this makes the most sense for |
|
Rebased to work around the unrelated noplatform ci failure. |
Describe where the new constants come from and link to the original issue which describes the protocol design.
Use `const` annotations for local variables that don't need multiple assignment to make the flow contract more obvious. Addresses a review comment.
Halt with an assert violation instead of dereferencing a null. Addressses a review comment.
This really does two things, which I've tried to make more clear with a comment, but `MaybeFoo` is a more common idiom in our code, and it avoids the misleading idea that scrambling happens most of the time.
Return early after discarding Histograms which won't be submitted so we don't waste time trying to process them further.
|
[puLL-Merge] - brave/brave-core@22522 DescriptionThis PR implements the Nebula differential privacy protocol for P3A (Privacy-Preserving Product Analytics) in Brave. The changes introduce sampling and randomization techniques to enhance privacy protection for certain metrics while maintaining the ability to collect aggregate data. ChangesChanges
Possible Issues
Security Hotspots
|
|
Verification PASSED on And
Test PlanTest plan verification_PASSED
Regression test
Step 1_PASSED
randomness metrics at
Step 2_PASSED
46 randomness requests are sent to this endpoint
46 typical logs metrics are shown under
46 metrics are shown under
Step 3_PASSED
metrics prepared
constellation logs
|
|
@DJAndries: All the requests are not sent to |
|
cc: @kjozwiak @LaurenWags |
Resolves brave/brave-browser#35841
Submitter Checklist:
QA/YesorQA/No;release-notes/includeorrelease-notes/exclude;OS/...) to the associated issuenpm run test -- brave_browser_tests,npm run test -- brave_unit_testswikinpm run presubmitwiki,npm run gn_check,npm run tslintgit rebase master(if needed)Reviewer Checklist:
gnAfter-merge Checklist:
changes has landed on
Test Plan:
Ensure requests are being sent to
star-randsrv.bsg.brave.comandcollector.bsg.brave.comas usual. Each collector request should have theBrave-P3A-Constellation-Thresholdheader set with a value of 20 or 50. Ensure the metrics get marked as sent in local state as usual.