Spec: add some shared infra for reporting, and port forDebugOnly to it. #1296
Conversation
| 1. [=list/For each=] |generatedBid| of |generatedBids|: | ||
| 1. If |generatedBid|'s [=generated bid/for k-anon auction=] is true: |
There was a problem hiding this comment.
...I need to figure out how to not report the non-k-anon run properly, I think it's wrong right now.
There was a problem hiding this comment.
OK, not too hard, but I am wondering about what we should do about platform realtime contributions here --- I think in impl we don't keep them for non-k-anon scoreAd run, while it's actually annoying to make the spec say that, and I am not sure it should.
There was a problem hiding this comment.
Done'ish. Also fixed additional bids not participating in non-k-anon auction.
| : <dfn>source</dfn> | ||
| :: A [=reporting bid source=] describing where the bid came from. | ||
| : <dfn>origin</dfn> | ||
| :: The [=origin=] of the bidder. |
There was a problem hiding this comment.
An [=origin=]. The bidding [=interest group=]'s [=interest group/owner=]?
There was a problem hiding this comment.
There is not necessarily an interest group... which makes my bidding of the next field is dubious.
There was a problem hiding this comment.
hmm, but all three places (normal bid, B&A, and additional bid) set it to IG's owner?
There was a problem hiding this comment.
Well, except for additional bid not actually having an IG, sure. Maybe bidder origin?
There was a problem hiding this comment.
yeah additional bids do not have the "stored" IGs, but additional bids also have an "interestGroup" field which defines owner and name. If we don't want to mess these concepts up, yeah "bidder origin" LGTM
spec.bs
Outdated
| originated in. | ||
| : <dfn>source</dfn> | ||
| :: A [=reporting bid source=] describing where the bid came from. | ||
| : <dfn>origin</dfn> |
spec.bs
Outdated
| : <dfn>origin</dfn> | ||
| :: The [=origin=] of the bidder. | ||
| : <dfn>interest group name</dfn> | ||
| :: A [=string=] uniquely identifying the interest group. |
There was a problem hiding this comment.
A [=string=]. The bidding [=interest group=]'s [=interest group/name=]?
And it's the owner/name pair that uniquely identifies the IG, right?
spec.bs
Outdated
| : [=reporting bid key/origin=] | ||
| :: |ig|'s [=interest group/owner=] | ||
| : [=reporting bid key/interest group name=] | ||
| :: A string representation of a new globably unique identifier. This is needed since |igName| |
There was a problem hiding this comment.
is this field not the IG name? But it seems to be IG name here: https://github.com/WICG/turtledove/pull/1296/files#diff-6f5a1d8263b0b0c42e2716ba5750e3652e359532647ac934c1c70086ae3ceddaR3506
There was a problem hiding this comment.
It's interest group name for regular interest groups. For additional bids it can't be, since there is nothing saying that they have to provide a unique interest group name. (And now I am scared of something in our impl breaking because of that...).
There was a problem hiding this comment.
So renamed this. The new description is kinda vague, but it does I think clarify that one can't expect it to be an IG name.
There was a problem hiding this comment.
does impl rely on IG name? if so, we need to fix it?
There was a problem hiding this comment.
Impl generally uses BidState* as equivalent of "reporting bid key" for reporting. There are a bunch of other things that use InterestGroupKey which seem fine for what I looked, but there are too many for me to be certain none get confused with additionalBid stuff (especially since it's somehow wired into fenced frame?)
| : <dfn>source</dfn> | ||
| :: A [=reporting bid source=] describing where the bid came from. | ||
| : <dfn>origin</dfn> | ||
| :: The [=origin=] of the bidder. |
There was a problem hiding this comment.
There is not necessarily an interest group... which makes my bidding of the next field is dubious.
|
|
||
| <dl dfn-for="reporting bid key"> | ||
| : <dfn>context</dfn> | ||
| :: The [=reporting context=] corresponding to the component (or single-level) auction the bid |
There was a problem hiding this comment.
A little confused by what "context" mean here. Is this field more like a "data" or "reports" storing all kinds of reports (currently only fDO)? And why does it belong to a "reporting bid key"? I thought that the other fields made the key, and this was the reporting data for the key. Currently it reads as [=reporting bid key=] has a [=reporting context=], which is a [=map=] whose key is [=reporting bid key=], which seems to be a loop.
There was a problem hiding this comment.
Basically the particular (sub)auction this is for --- so that bids from same IGs in different component auctions can be told apart. It's not a loop because these are references, but it is a bit redundant; having everything in the key means you can determine who the winner is by comparing the reporting bid keys.
There was a problem hiding this comment.
what information of [=reporting context=] can tell bids from same IGs in different component auctions apart? We allow two component auctions from the same component seller, right? It's very unlikely for two [=reporting bid keys=] to be the same, but it's still possible for the current definition IIUC.
There was a problem hiding this comment.
It's reference equality....
There was a problem hiding this comment.
ah ok.
The current scheme is:
map (auction_config -> reporting context), where reporting context is a struct [=debug reporting info=]. Is it possible to change it to:
map (auction_config -> [=reporting info (renamed from debug report info) =])?
Or say, change [=reporting context=] from a struct to "A map from reporting bid key to bid debug reporting info", without needing the [=debug reporting info=] struct layer.
There was a problem hiding this comment.
ah ok. The current scheme is:
map (auction_config -> reporting context), where reporting context is a struct [=debug reporting info=]. Is it possible to change it to:map (auction_config -> [=reporting info (renamed from debug report info) =])?
That wouldn't work as well once Private Aggregation is added since you would need a separate map for private aggregation stuff that will have to be passed everywhere. Having a reporting context means we can stick whatever fields we need for private aggregation into the same struct (and there is a lot of stuff I expect to put there).
...As I mentioned before, we could stick real time reporting there, too, to avoid to need to pass it around, but it's not actually partionned by subauction so there would be extra work merging it (though it's like one extra loop level, so may be worth it for the uniformity). The split is mostly useful for the per-participant P.Agg. metrics I want to add, since I'll need space to stick stuff like how long the component auction's bidding script took to fetch, etc.
Or say, change [=reporting context=] from a struct to "A map from reporting bid key to bid debug reporting info", without needing the [=debug reporting info=] struct layer.
Same thing here.
There was a problem hiding this comment.
Oh I see, so PAgg map needs to be in parallel to the [=debug reporting info=] since the two maps have different keys? I was thinking maybe put all PAgg/RTR data can be put inside the [=bid debug reporting info=], but seems not working due to they are keyed by different things, probably?
There was a problem hiding this comment.
Yeah, and there will probably be more than one thing.
This is intended to be used for moving our Private Aggregation support into our spec, as it provides:
vague on how the top-level scope should work still).
...for now, forDebugOnly is what's ported to it, and while at it, things were fixed to actually pass the argument needed for reporting to scoreAd invocations, and to fix up additional bids (and B&A) for it.
It may make sense to move realTimeContributions in there as well, though it doesn't really benefit from any functionality besides maybe avoiding a parameter sometimes (and the split is kinda harmful).
Preview | Diff