-
Notifications
You must be signed in to change notification settings - Fork 26
Spec: Aggregate error reporting #172
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
Conversation
Adds support for aggregate error reporting. Additional changes are required to the embedding APIs' specs to support the feature.
@dmcardle, could you PTAL? Thanks! |
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.
Thanks, Alex! Just made one pass through and left some questions and nits.
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.
Thanks, Dan!
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.
Regardless of my last round of comments, this LGTM at this point :)
Uhh not sure why my recent replies aren't both showing up on this thread, but here they are: |
Gah! Another one for the gripe list! I noticed this a couple days ago, too. |
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.
Thanks, those edits look great. SLGTM!
Great, thanks for reviewing! |
SHA: e5804a7 Reason: push, by alexmturner Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Adds support for the new Private Aggregation error reporting feature to Shared Storage. See the related PAA spec change: patcg-individual-drafts/private-aggregation-api#172 Also see the explainer: https://github.com/patcg-individual-drafts/private-aggregation-api/blob/main/error_reporting.md Slightly reorganizes the PAA integration with this spec for readability.
Replaces usage of "non-internal" to "external" to better match the spec: patcg-individual-drafts/private-aggregation-api#172 Bug: 381788013 Change-Id: If139ff27eb74f6467adf2649c7bc9b0ccb7aefd9 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6335576 Reviewed-by: Dan McArdle <[email protected]> Auto-Submit: Alex Turner <[email protected]> Commit-Queue: Joe Mason <[email protected]> Reviewed-by: Joe Mason <[email protected]> Cr-Commit-Position: refs/heads/main@{#1430543}
Adds support for the new Private Aggregation error reporting feature to Shared Storage. See the related PAA spec change: patcg-individual-drafts/private-aggregation-api#172 Also see the explainer: https://github.com/patcg-individual-drafts/private-aggregation-api/blob/main/error_reporting.md Slightly reorganizes the PAA integration with this spec for readability.
Note that spec PRs for integrating with Shared Storage and Protected Audience can be found here: |
Adds support for the kPrivateAggregationApiErrorReporting feature*, including the new flow involving two calls to the interface: first, `InspectBudgetAndLock()`; and then, `ConsumeBudget()`. The old flow (when the feature is disabled) is also maintained for now. When the feature is disabled (which it is by default), this cl should have no effect. These new calls are also not yet integrated with the manager; this will be done in a follow-up cl. *This feature is described further here: https://github.com/patcg-individual-drafts/private-aggregation-api/blob/main/error_reporting.md It was specified here: patcg-individual-drafts/private-aggregation-api#172 Bug: 381788013 Change-Id: Iccd452826b887e1de39a33ab5344de2de6774978 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6304832 Reviewed-by: Charlie Harrison <[email protected]> Reviewed-by: Dan McArdle <[email protected]> Commit-Queue: Alex Turner <[email protected]> Cr-Commit-Position: refs/heads/main@{#1439738}
Adds support for aggregate error reporting. Additional changes are required to the embedding APIs' specs to support the feature.
Preview | Diff