Spec: auctionBuyerKeys reporting #117
Conversation
Adds spec handling for the per-buyer latency and statistics reporting for Protected Audience, see https://github.com/WICG/turtledove/blob/main/FLEDGE_extended_PA_reporting.md. Some follow-up work remains and has been tracked in issues added.
|
Hey @qingxinwu, could you PTAL? Tried to put together a first go at the spec for cc @caraitto |
note there's a PR to add seller capabilities to Protected Audience's spec, and I just took over that and am working on it, so hopefully you'll not need to worry about it in your spec. |
| 1. Let |interestGroupBuyers| be |auctionConfig|'s <a spec="turtledove" | ||
| for="auction config">interest group buyers</a>. | ||
| 1. If |interestGroupBuyers| is null, set |interestGroupBuyers| to a new | ||
| [=list=]. |
There was a problem hiding this comment.
it seems these two steps are identical to check "If |config|["{{AuctionAdConfig/auctionReportBuyerKeys}}"]
[=map/exists=] and |auctionConfig|'s interest group buyers is not null:".
But the implementation seems not checking this, but just simply copying {{AuctionAdConfig/auctionReportBuyerKeys}}
There was a problem hiding this comment.
I don't think we can condense this as that would avoid validating the auctionReportBuyerKeys (unless we do that separately)
| |config|["<code>{{AuctionAdConfig/auctionReportBuyerKeys}}</code>"][|index|]. | ||
| 1. If |key|["{{PAHistogramContribution/bucket}}"] is not in the range | ||
| [0, 2<sup>128</sup>−1], [=exception/throw=] a {{TypeError}}. | ||
| 1. If |index| is equal to or greater than |interestGroupBuyers|' [=list/ |
There was a problem hiding this comment.
the implementation seems not doing this, but just simply copying all keys.
So there's a difference in implementation that the step above will continue throw errors if the condition meets for extra |index|.
There was a problem hiding this comment.
Switched this to a continue and added a note to match the validation logic. And yeah I thought it might be easier to follow in the spec if we didn't just keep these as a list. We could consider switching to more closely match the implementation structure.
| /bucket}}</code>"] is not in the range [0, 2<sup>128</sup>−1], | ||
| [=exception/throw=] a {{TypeError}}. | ||
|
|
||
| Issue: Consider validating the case where the bucket used (after |
There was a problem hiding this comment.
seems to be this todo in code? https://crsrc.org/c/third_party/blink/renderer/modules/ad_auction/navigator_auction.cc;drc=738f1250512a5e6ff8cacb6b36540cd50e8156d8;l=2065
But agreed that this needs to be validated.
There was a problem hiding this comment.
Yep, seems to be the same concept
|
Thanks for the review! Should be ready for another pass :) |
| spec="turtledove" for="interest group">owner</a> is | ||
| |buyerOrigin|. | ||
| : "`totalSignalsFetchLatency`" | ||
| :: The total time spent fetching trusted buyer signals in |
There was a problem hiding this comment.
(not something to change, just a comment)
these descriptions for values might be fine for now, but in the future (maybe when we move to the PA spec), we need to actually calculate these numbers, instead of only telling what these values mean.
There was a problem hiding this comment.
Yep, totally agree -- I tried to keep the scope of this smaller to unblock the debug mode change, but I've added an issue (number 37) to more formally spec the values here.
|
Thanks for the review! |
SHA: b09c9c4 Reason: push, by alexmturner Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Adds spec handling for the per-buyer latency and statistics reporting for Protected Audience, see
https://github.com/WICG/turtledove/blob/main/FLEDGE_extended_PA_reporting.md. Some follow-up work remains and has been tracked in issues added.
Preview | Diff