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.

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

Merged
merged 11 commits into from
Nov 25, 2024

Conversation

qingxinwu
Copy link
Collaborator

@qingxinwu qingxinwu commented Nov 5, 2024

Client side support of downsampling forDebuggingOnly reports for auctions run on bidding and auction service.


Preview | Diff

@qingxinwu qingxinwu added the spec Relates to the spec label Nov 5, 2024
@qingxinwu
Copy link
Collaborator Author

@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=]:
Copy link
Collaborator

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?

Copy link
Collaborator Author

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.

spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
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=].
Copy link
Collaborator

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)

Copy link
Collaborator Author

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)

Copy link
Collaborator

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?

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 added a note. It's complicated, so a note would be very helpful.

spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated

Note: forDebuggingOnly reports from [=server auction response=] were downsampled on truested
Copy link
Collaborator

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?

Copy link
Collaborator Author

@qingxinwu qingxinwu Nov 22, 2024

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.

@qingxinwu qingxinwu changed the title [Spec] Add downsampling support for B&A response [Spec] Add downsampling forDebuggingOnly support for B&A response Nov 22, 2024
Copy link
Collaborator

@morlovich morlovich left a 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.

spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
@JensenPaul JensenPaul merged commit 35bc3e7 into WICG:main Nov 25, 2024
2 checks passed
github-actions bot added a commit that referenced this pull request Nov 25, 2024
)

SHA: 35bc3e7
Reason: push, by JensenPaul

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@qingxinwu qingxinwu deleted the fdo branch November 25, 2024 15:13
github-actions bot added a commit to qingxinwu/turtledove that referenced this pull request Nov 25, 2024
…CG#1325)

SHA: 35bc3e7
Reason: push, by qingxinwu

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spec Relates to the spec
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants