-
Notifications
You must be signed in to change notification settings - Fork 237
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
[Spec] Add downsampling forDebuggingOnly support for B&A response #1325
base: main
Are you sure you want to change the base?
Conversation
@morlovich Mind taking a look? Thanks! Please let me know if there's any question. |
@@ -3070,17 +3070,39 @@ a [=list=] of [=interest groups=] |bidIgs|, and a [=reporting context map=] | |||
equals |componentAd|. If there is no matching element, return failure. | |||
1. [=list/Append=] a new [=ad descriptor=] whose [=ad descriptor/url=] is | |||
|componentAd| to |winningAdComponents|. | |||
1. Let |bidDebugReportingInfo| be a new [=bid debug reporting info=]. | |||
1. [=list/For each=] |key| → |maybeDebugReportUrl| in |response|'s | |||
[=server auction response/component win debugging only reports=]: |
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.
What has the responsibility for verifying that this is actually a component auction?
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.
good question. [=server auction response/component win debugging only reports=] only comes from reports whose componentWin
(code) is set to true (which is only possible if a B&A response message is from component auction's server, for auctions whose top level auction runs on-device). Currently we trust B&A server should make sure the fields are set correctly, but would probably be good for Chrome to check that the message is from a component auction server if it has a report whose componentWin
is set to true.
1. If |debugURLs| [=list/is empty=]: | ||
1. If the result of running [=is debugging only in cooldown or lockout=] with | ||
|invokingOrigin| is false, then [=update debug report cooldown=] with |invokingOrigin|. | ||
1. [=iteration/Continue=]. |
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.
So I don't understand what's happening here, especially wrt. to the empty list check. (Or is meant to be not empty? But no, you do update cooldown when fromServer is true, so why do you need to update it if there are no reports; in case the server didn't sample any? What if there weren't any in the first place)
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.
In order to send less fDO reports back to client to reduce the response size, we decided to let the server side do the 1/1000 sampling, and only send sampled urls back to client. Client will then check origin's cooldown, and also filter sampled urls based on final auction result (if needed).
There's a difference between origin->[], and not having an origin. If there weren't any call, then there won't be a per_origin_debug_reports in the response for that origin, thus no entry for this origin in [=bid debug reporting info/server filtered debugging only reports=].
A differene case is:
For example, one losing component buyer called debugLoss API, but the URL was not picked by server side due to we're doing sampling there, then the server still needs to tell client that the origin did call the loss api to set its cooldown. In this case, per_origin_debug_reports has the origin, but its debug report URLs array can be empty. We still need to update the origin's cooldown since it did call the loss API (and it loses).
The design is a little tricky that client side needs to believe B&A server did the right thing (i.e., meeting all the assumptions client side expects, which we agreed in the design)
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.
That makes sense, but may be worth some sort of a note in the spec about server inserting a blank array to denote when it didn't sample but when we still need to update the lockout?
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.
yeah added a note. It's complicated, so a note would be very helpful.
spec.bs
Outdated
|
||
Note: forDebuggingOnly reports from [=server auction response=] were downsampled on truested |
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.
Typo, truested. So the downsampling happens on the server not just for the filtered list, but also the conditional one?
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.
yes, it samples all. Because every url is sampled independently, so we think "sampling first on server and then pick which sampled one is going to be sent on device" is the same as "picking which ones can be sampled and then sample from them" which happens on device. This way it greatly reduces the number of URLs sent back to client.
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.
Looks good modulo two minor things.
Client side support of downsampling forDebuggingOnly reports for auctions run on bidding and auction service.
Preview | Diff