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

Header-based DirectFromSellerSignals #695

Merged
merged 9 commits into from
Sep 29, 2023
Merged

Conversation

caraitto
Copy link
Collaborator

@caraitto caraitto commented Jul 6, 2023

Addresses #119

FLEDGE.md Outdated Show resolved Hide resolved
FLEDGE.md Outdated
]
```

When invoking `navigator.runAdAuction()`, `directFromSellerSignalsHeaderAdSlot` is used to lookup the signals intended for that auction. `directFromSellerSignalsHeaderAdSlot` is a string that should match the `adSlot` value contained in some `Ad-Auction-Signals` response served from the origin of that auction's seller. Note that for multi-seller or component auctions, each component auction can specify its own `directFromSellerSignalsHeaderAdSlot`, and the response should be served from that component auction's seller. Different sellers may safely use the same `adSlot` names without conflict. If `directFromSellerSignalsHeaderAdSlot` matches multiple `adSlot`s from header responses, one of those `adSlot` responses will be chosen arbitrarily.
Copy link
Contributor

Choose a reason for hiding this comment

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

If directFromSellerSignalsHeaderAdSlot matches multiple adSlots from header responses, one of those adSlot responses will be chosen arbitrarily.

Why not LIFO versus arbitrary?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I could look into switching to LIFO if there's interest -- the current WIP MVP implementation stores an unordered set of responses, but it could possibly be switched to a queue, maybe even in a later version (switching to a non-arbitrary order from an arbitrary order shouldn't be a breaking change).

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to switching to a LIFO data structure. Ad techs may choose to define the adSlot as an element on the page containing an ad that may refresh the ad over time. It seems unlikely to want the same exact set of signals when a newer response with the same adSlot is available on the page.

Similarly, have you considered how resource management should work in this flow? Once the ad auction is completed, is there any way to dispose the stale signals, or is this an unavoidable source of memory leaks? We plan on iterating on the amount of data transferable within this header and starting small; do you anticipate any issues when approaching O(kilobytes) of JSON?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK, I think we can use a LIFO order [0], where a fetch for adSlot "foo" for fetch / seller origin seller.com would overwrite any previous stored signals for the (seller.com, "foo") key tuple.

In the initial version [1], all server responses are stored until the top-level page is unloaded, and there is no way to otherwise remove old responses from memory; each new fetch() with {adAuctionHeaders: true} that captures new headers will result in additional memory consumption, and we also parse potentially all of those responses in arbitrary order, only stopping if a matching adSlot is found.

Currently, the entire set of responses is re-parsed and searched for each top-level, component auction, or non-component auction on the page using the new directFromSellerSignals. So, if there are many responses, or they are large, this will add to the auction latency.

With the new LIFO order, the signals would only be parsed once, and further lookups would just be map lookups (unless more signals have arrived).

Individual responses greater than 1000 bytes are currently discarded [2]. We could consider increasing this, but headers are generally small, and several server implementations and CDNs impose their own limits. Also, the signals get copied from the browser process to worklet processes.

Do folks think these proposed changes would address all your concerns?

[0] Internally in the Chromium implementation, it would technically be a FIFO queue of the JSON string responses -- we'd process all of them during auctions, overwriting the value signals for each (fetch / seller origin, adSlot string) key tuple as we process the list, then empty the processed FIFO -- this would mean that more recent responses overwrite older responses, or LIFO semantics. The spec already works in a similar manner (except that JSON parsing is synchronous in spec language, so the parsing happens in the FETCH spec patch). The reason for processing the JSON during auctions is to avoid complexity around potential races / waiting in auctions until processing has completed if started in the network interceptor.

Note, this means that the memory for old signals wouldn't be freed until an auction is run in this new proposal.

[1] The initial version was cherry-picked into Chrome Beta 117.0.5938.35, but we'll need approval on an I2S to be able to flip the controlling flag on (currently working on getting approval, which requires we complete the spec).

[2] https://source.chromium.org/chromium/chromium/src/+/main:content/browser/interest_group/ad_auction_url_loader_interceptor.cc;l=166;drc=6f4c64436342c818aa41e6a5c55034e74ec9c6b6

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@JacobGo What do you think of the above proposal? I'll probably make the change in a separate PR.

Copy link
Contributor

@JacobGo JacobGo Sep 19, 2023

Choose a reason for hiding this comment

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

Thanks @caraitto, the LIFO order optimization sounds great!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here's a PR to change this for the explainer (the spec already matches this behavior): #890

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

FYI: the LIFO change will be available in 121.0.6121.0 and newer: https://chromiumdash.appspot.com/commit/b26d4257d640b9abdef6a75eaf7d0eb1a103367d

@JensenPaul JensenPaul added the Non-breaking Feature Request Feature request for functionality unlikely to break backwards compatibility label Jul 19, 2023
@JacobGo
Copy link
Contributor

JacobGo commented Jul 25, 2023

Could you please elaborate on the rationale for introducing directFromSellerSignalsHeaderAdSlot as opposed to reusing directFromSellerSignals? What happens if both are provided? Would it possible to instead infer which one is in use by examining the Content-Type or looking for the appropriate response headers?

@caraitto
Copy link
Collaborator Author

Could you please elaborate on the rationale for introducing directFromSellerSignalsHeaderAdSlot as opposed to reusing directFromSellerSignals? What happens if both are provided? Would it possible to instead infer which one is in use by examining the Content-Type or looking for the appropriate response headers?

Sure -- both directFromSellerSignalsHeaderAdSlot and directFromSellerSignals take a string as a parameter -- an ad-slot for the former, and a prefix for constructing subresource bundle subresource URLs in the latter case.

While it would be possible to try to to parse directFromSellerSignals as a URL, using the subresource bundle version if the string is a valid URL, and treating it as a header ad-slot otherwise, this seems confusing and fragile. Instead, it's easier to just have separate fields.

Changing the type of the directFromSellerSignals auction config parameter (to take an object specifying which type to use in addition to the prefix / ad-slot) would also be a breaking change.

As mentioned in the PR, It is not valid to provide values for both directFromSellerSignalsHeaderAdSlot and directFromSellerSignals in the same component, top-level, or non-component auction -- in this case, runAdAuction() will throw an error explaining the problem, and no auction will run.

Furthermore, different component / top-level auctions belonging to the same top-level may use different forms of directFromSellerSignals, and it's also not required that every seller in overall auction use a form directFromSellerSignals if some of them do.

FLEDGE.md Outdated Show resolved Hide resolved
FLEDGE.md Outdated Show resolved Hide resolved
aarongable pushed a commit to chromium/chromium that referenced this pull request Sep 25, 2023
UMA metrics for the response sizes will be added after a refactor to
support the LIFO semantics requested in [0].

[0] WICG/turtledove#695 (comment)

Bug: 1462720
Change-Id: Ie93ccd5a9e155eb2fb1ef7d2eda7130d668a4b81
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4889564
Commit-Queue: Caleb Raitto <caraitto@chromium.org>
Reviewed-by: Steven Bingler <bingler@chromium.org>
Reviewed-by: Russ Hamilton <behamilton@google.com>
Cr-Commit-Position: refs/heads/main@{#1201169}
@JensenPaul JensenPaul merged commit be00bd7 into WICG:main Sep 29, 2023
2 checks passed
github-actions bot added a commit that referenced this pull request Sep 29, 2023
SHA: be00bd7
Reason: push, by JensenPaul

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@caraitto caraitto deleted the patch-2 branch September 29, 2023 19:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Non-breaking Feature Request Feature request for functionality unlikely to break backwards compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants