Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Spec: add some shared infra for reporting, and port forDebugOnly to it. #1296

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Spec: add some shared infra for reporting, and port forDebugOnly to it. #1296

Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
01a7463
Start the refactor on reporting...
Oct 8, 2024
ec98741
Fix score invocations.
Oct 8, 2024
9fe4956
Don't do reporting from shadow auction.
Oct 10, 2024
d63b216
Merge branch 'main' into reporting-refactor... plus start applying
Oct 14, 2024
a2bfeb8
Revert note move.
Oct 14, 2024
4397e9e
Rename field.
Oct 15, 2024
8f89dde
Apply feedback
Oct 17, 2024
d75b120
More feedback.
Oct 17, 2024
2a7fbe6
origin -> bidder origin.
Oct 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
82 changes: 43 additions & 39 deletions spec.bs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1724,27 +1724,25 @@ and a [=real time reporting contributions map=] |realTimeContributionsMap|:
1. Let |compWinnerInfo| be the result of running [=generate and score bids=] with |component|,
|auctionConfig|, |global|, |bidIgs|, |reportingContextMap|, and |realTimeContributionsMap|.
1. If [=recursively wait until configuration input promises resolve=] given |auctionConfig|
returns failure, or |compWinnerInfo| is failure, then:
1. Decrement |pendingComponentAuctions| by 1.
1. Abort these steps.
1. If |topLevelDirectFromSellerSignalsRetrieved| is false:
1. Let |topLevelDirectFromSellerSignals| be the result of running
[=get direct from seller signals=] given |seller|, |auctionConfig|'s
[=auction config/direct from seller signals header ad slot=], and |capturedAuctionHeaders|.
1. Set |topLevelDirectFromSellerSignalsForSeller| to the result of running
[=get direct from seller signals for a seller=] given |topLevelDirectFromSellerSignals|.
1. Set |topLevelDirectFromSellerSignalsRetrieved| to true.
1. If |compWinnerInfo|'s [=leading bid info/leading bid=] is not null, then run
[=score and rank a bid=] with |auctionConfig|, |reportingContextMap|[|component|],
|compWinnerInfo|'s [=leading bid info/leading bid=], |leadingBidInfo|,
|decisionLogicFetcher|, |trustedScoringSignalsBatcher|, null, "top-level-auction", null,
and |topLevelOrigin|.
1. If |compWinnerInfo|'s [=leading bid info/leading non-k-anon-enforced bid=]
is not null, then run [=score and rank a bid=] with |auctionConfig|, |reportingContextMap|[
|component|], |compWinnerInfo|'s [=leading bid info/leading non-k-anon-enforced bid=],
|leadingBidInfo|, |decisionLogicFetcher|, |trustedScoringSignalsBatcher|,
|topLevelDirectFromSellerSignalsForSeller|, null, "top-level-auction", null, |topLevelOrigin|,
and |realTimeContributionsMap|.
does not return failure, and |compWinnerInfo| is not failure, then:
1. If |topLevelDirectFromSellerSignalsRetrieved| is false:
1. Let |topLevelDirectFromSellerSignals| be the result of running
[=get direct from seller signals=] given |seller|, |auctionConfig|'s
[=auction config/direct from seller signals header ad slot=], and |capturedAuctionHeaders|.
1. Set |topLevelDirectFromSellerSignalsForSeller| to the result of running
[=get direct from seller signals for a seller=] given |topLevelDirectFromSellerSignals|.
1. Set |topLevelDirectFromSellerSignalsRetrieved| to true.
1. If |compWinnerInfo|'s [=leading bid info/leading bid=] is not null, then run
[=score and rank a bid=] with |auctionConfig|, |reportingContextMap|[|component|],
|compWinnerInfo|'s [=leading bid info/leading bid=], |leadingBidInfo|,
|decisionLogicFetcher|, |trustedScoringSignalsBatcher|, null, "top-level-auction", null,
and |topLevelOrigin|.
1. If |compWinnerInfo|'s [=leading bid info/leading non-k-anon-enforced bid=]
is not null, then run [=score and rank a bid=] with |auctionConfig|, |reportingContextMap|[
|component|], |compWinnerInfo|'s [=leading bid info/leading non-k-anon-enforced bid=],
|leadingBidInfo|, |decisionLogicFetcher|, |trustedScoringSignalsBatcher|,
|topLevelDirectFromSellerSignalsForSeller|, null, "top-level-auction", null, |topLevelOrigin|,
and |realTimeContributionsMap|.
1. Decrement |pendingComponentAuctions| by 1.
1. Wait until |pendingComponentAuctions| is 0.
1. If |auctionConfig|'s [=auction config/aborted=] is true, return failure.
Expand Down Expand Up @@ -2897,7 +2895,7 @@ a [=list=] of [=interest groups=] |bidIgs|, and a [=reporting context map=]
:: [=reporting bid source/bidding-and-auction-services=]
: [=reporting bid key/origin=]
:: |response|'s [=server auction response/interest group owner=]
: [=reporting bid key/interest group name=]
: [=reporting bid key/bid identifier=]
:: |response|'s [=server auction response/interest group name=]
:: |response|'s [=server auction response/bid=]
qingxinwu marked this conversation as resolved.
Show resolved Hide resolved
: [=generated bid/bid in seller currency=]
Expand Down Expand Up @@ -2981,10 +2979,15 @@ a [=list=] of [=interest groups=] |bidIgs|, and a [=reporting context map=]
[=interest group/owner=] is |igId|'s [=interest group/owner=] and [=interest group/name=] is |igId|'s
[=interest group/name=]. [=iteration/Continue=] if none found.
1. [=list/Append=] |ig| to |bidIgs|.
1. Insert the debug reporting URLs from |response| into |reportingContextMap| [|auctionConfig|].

Issue: TODO: Handle forDebuggingOnly reports from server auction.
(<a href="https://github.com/WICG/turtledove/issues/1254">WICG/turtledove#1254</a>)
1. [=map/For each=] |igId| → |updateIfOlderThan| of |response|'s
[=server auction response/update groups=]:
1. Let |ig| be the [=interest group=] in the [=user agent=]'s [=interest group set=] whose
[=interest group/owner=] is |igId|'s [=interest group/owner=] and [=interest group/name=] is |igId|'s
[=interest group/name=]. [=iteration/Continue=] if none found.
1. If |updateIfOlderThan| is less than 10 mintues, set it to 10 minutes.
1. If [=current wall time=] &minus; |ig|'s [=interest group/last updated=] ≥
|updateIfOlderThan|, set |ig|'s [=interest group/next update after=] to the
[=current wall time=] + |updateIfOlderThan|.
1. Return |winningBidInfo|.

</div>
Expand Down Expand Up @@ -3398,6 +3401,13 @@ A <dfn>reporting bid source</dfn> an enum with the following possible values:
</dl>

A <dfn>reporting bid key</dfn> is a [=struct=] with the following [=struct/items=]:

Note: This type exists only to uniquely identify places bids came from, avoiding confusion in
morlovich marked this conversation as resolved.
Show resolved Hide resolved
cases like bids made by the same interest group in different component auctions, or additional
bids reusing names of regular interest groups. Implementations can likely find a more efficient
way of achieving the same effect. Note that bids returned when `generateBid()` returns multiple
items all share a key, as they share reporting information, too.

<dl dfn-for="reporting bid key">
: <dfn>context</dfn>
:: The [=reporting context=] corresponding to the component (or single-level) auction the bid
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's reference equality....

Copy link
Collaborator

@qingxinwu qingxinwu Oct 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, and there will probably be more than one thing.

Expand All @@ -3406,17 +3416,11 @@ A <dfn>reporting bid key</dfn> is a [=struct=] with the following [=struct/items
:: A [=reporting bid source=] describing where the bid came from.
: <dfn>origin</dfn>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe owner?

:: The [=origin=] of the bidder.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An [=origin=]. The bidding [=interest group=]'s [=interest group/owner=]?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is not necessarily an interest group... which makes my bidding of the next field is dubious.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, but all three places (normal bid, B&A, and additional bid) set it to IG's owner?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, except for additional bid not actually having an IG, sure. Maybe bidder origin?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

: <dfn>interest group name</dfn>
:: A [=string=] uniquely identifying the interest group.

: <dfn>bid identifier</dfn>
:: A [=string=] distinguishing this source of reports from others of the same origin in the same
context.
</dl>

Note: This type exists only to uniquely identify places bid came from, avoiding confusion in
cases like bids made by the same interest group in different component auctions, or additional
bids reusing names of regular interest groups. Implementors can likely find a more efficient means
of achieving the same effect. Note that bids returned when `generateBid()` returns multiple
items all share an ID, as they share reporting information, too.

A <dfn>reporting context</dfn> is a struct with the following [=struct/items=]:
morlovich marked this conversation as resolved.
Show resolved Hide resolved
<dl dfn-for="reporting context">
: <dfn>debug reporting info</dfn>
Expand All @@ -3431,9 +3435,9 @@ auction in a multi-party auction).
To <dfn>create a reporting context map</dfn> given [=auction config=] |auctionConfig|:
1. Let |reportingContextMap| be a new [=reporting context map=].
1. If |auctionConfig|'s [=auction config/component auctions=] [=list/is empty=],
[=map/set=] |reportingContextMap| [|auctionConfig|] to a new [=reporting context=].
[=map/set=] |reportingContextMap|[|auctionConfig|] to a new [=reporting context=].
1. Otherwise, [=list/for each=] |component| in |auctionConfig|'s
[=auction config/component auctions=], [=map/set=] |reportingContextMap| [|component|] to a new
[=auction config/component auctions=], [=map/set=] |reportingContextMap|[|component|] to a new
[=reporting context=].
1. Return |reportingContextMap|.
</div>
Expand Down Expand Up @@ -3525,7 +3529,7 @@ methods for event-level <dfn>forDebuggingOnly reports</dfn> for winning and losi
: [=reporting bid key/origin=]
:: |ig|'s [=interest group/owner=]

: [=reporting bid key/interest group name=]
: [=reporting bid key/bid identifier=]
:: |ig|'s [=interest group/name=]
1. [=map/Set=] |reportingContext|'s [=reporting context/debug reporting info=] [|id|] to
|bidDebugReportInfo|.
Expand Down Expand Up @@ -4097,7 +4101,7 @@ a [=reporting context map=] |reportingContextMap|, an [=auction config=] |auctio
:: [=reporting bid source/additional-bid=]
: [=reporting bid key/origin=]
:: |ig|'s [=interest group/owner=]
: [=reporting bid key/interest group name=]
: [=reporting bid key/bid identifier=]
:: A string representation of a new globably unique identifier. This is needed since |igName|
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does impl rely on IG name? if so, we need to fix it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?)

morlovich marked this conversation as resolved.
Show resolved Hide resolved
may not be unique.
: [=generated bid/bid=]
Expand Down
Loading