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

Add 'hb_cache_host' targeting for video bids when cache is set #3654

Merged

Conversation

idettman
Copy link
Contributor

@idettman idettman commented Mar 19, 2019

Type of change

  • Other

Motivation

DFP imposes a limit on publishers entering in a video VAST tag URL... the host cannot be a variable.
Given that macros are possible, supplying the host and path on the ad request would solve this, but this is impossible in DFP, as %% macros cannot be supplied in the host name.

The solution we chose is for Prebid.org, was to host a cache redirector service. The video creative would point to this service, which then redirects the player to where the cached asset actually resides, and the publisher would be able to enter a constant VAST tag URL like:
https://cache.prebid.org/redir?uuid=%%PATTERN:hb_uuid%%&host=%%PATTERN:hb_cache_host%%

The Cache Redirector Service at cache.prebid.org looks at the hb_cache_host variable and redirects the video player to the appropriate place, e.g. prebid.adnxs.com/pbc/v1/cache or prebid-server.rubiconproject.com/cache.

To support separate line items for each bidder, the VAST tag URL could contain the bidder-specific key-value-pairs:
https://cache.prebid.org/redir?uuid=%%PATTERN:hb_uuid_rubicon%%&host=%%PATTERN:hb_cache_host_rubicon%%

Description of change

If mediaType is video, and the config has cache.url defined, every bidResponse object will now contain hb_cache_host in the adServerTargeting.
If an adapter had previously specified hb_cache_host in it's bidResponseObject, that value is used, instead of overriding with the client side cache.url.
This addressees the scenario where some bidders use a server side cache and others use client side cache.

Handling invalid cache url values
hb_cache_host is parsed from the config property cache.url (if it's defined). If the cache.url value is not a valid url, both hb_cache_id and hb_uuid will be undefined, and PBJS tries to cache on the random string and fails, logging the following warning:
WARNING: Failed to save to the video cache: Error: Error storing video ad in the cache: : {}. Video bid must be discarded.

Example

config.setConfig({
  cache: {
    url: 'https://prebid.adnxs.com/pbc/v1/cache'
  }
});

Related

@idettman idettman added needs review pinned won't be closed by stalebot needs 2nd review Core module updates require two approvals from the core team labels Mar 19, 2019
Copy link
Collaborator

@jsnellbaker jsnellbaker left a comment

Choose a reason for hiding this comment

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

As a question - is the hb_cache_path currently in-use in the redirector service? Or is this an anticipated future need?

I looked through the other repo and the examples in the main README just refer to the hb_uuid and hb_cache_host keys.

@bretg
Copy link
Collaborator

bretg commented Mar 20, 2019

hb_cache_path isn't needed right now because it's built into the redirector config.

we added it to be orthogonal, but it can be removed.

@jsnellbaker
Copy link
Collaborator

@bretg Thanks for the clarification; that's fine. The overall changes here LGTM.

Is there anything we'd need to update in the prebid docs in lieu of this change? It seems like it'd be handled by any docs focused on the redirector service, but I wasn't sure if there would be anything else to note base-line. I was potentially thinking around the bidderSettings.standard.adServerTargeting table - as anyone who's using this feature would need to populate these new keys if they wanted to use the service as well (otherwise they won't be generated in the bid).

@bretg
Copy link
Collaborator

bretg commented Mar 21, 2019

Good call @jsnellbaker. However, I'd like these targeting values to be set even if the user sets bidderSettings.standard.adServerTargeting. Therefore, I think the new blocks of code should be moved out of this block:

if (!bidderSettings[CONSTANTS.JSON_MAPPING.BD_SETTING_STANDARD][CONSTANTS.JSON_MAPPING.ADSERVER_TARGETING]) {

Will open review comments on that.

BTW - @mkendall07 and I are starting to discuss whether the system should be setting the standard targeting variables even if the page supplies bidderSettings.standard.adServerTargeting -- I think it should.

});

Copy link
Collaborator

Choose a reason for hiding this comment

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

I suggest moving this block out of the check for bidderSettings.standard.adServerTargeting so that if a user does set that value, hb_cache_host is set anyhow. Probably the right approach is to check:

  • when getConfig('cache.url') is set
    -- if hb_cache_host is not already set by the user, set it

And while we're at it, let's drop hb_cache_path. I should be consistent in my desire to minimize our targeting footprint, so if we don't need hb_cache_path, let's not set it.

@bretg bretg changed the title Add 'hb_cache_host' and 'hb_cache_path' targeting for video bids when cache is set Add 'hb_cache_host' targeting for video bids when cache is set Mar 21, 2019
@jsnellbaker jsnellbaker self-requested a review March 21, 2019 14:46
@idettman
Copy link
Contributor Author

Ready for re-review, hb_cache_host is now defined even if adserverTargeting was defined before setting default targeting, also removed hb_cache_path from targeting.

@idettman idettman requested a review from bretg March 23, 2019 05:48
@bretg
Copy link
Collaborator

bretg commented Mar 25, 2019

Ok, so what this code says is:

  1. adserverTargeting = user-specified bidderSettngs.standard.adserverTargeting
  2. if the mediaType is video and user did not specify hb_uuid, hb_cache_id, or hb_cache_host, add them

I like it. But I can see how others might not like the added complexity of what was formerly a simple rule. However, in talking to @mkendall07, I believe this is the direction we should be going -- other attributes will follow these in future releases.

Matt, if you approve this, I'll write up a note on the Pub API page.

@mkendall07
Copy link
Member

@bretg

Thanks for the note. I do think it's the right direction, however, I think we need a way for a pub to opt out of all these additional keys. For example, no one is using hb_cache_host now, I don't think we should force them to have it sent. We are seeing concerns about the total length of all the keys sent to DFP (in the worst case, this truncates the DFP call). So I'll approve if there is a way to not force the additional keys.

@bretg
Copy link
Collaborator

bretg commented Mar 26, 2019

@mkendall07 - good point. We've already documented bidderSettings.sendStandardTargeting. It defaults to true.

So this code needs to be updated to not add the variables if bidderSettings.sendStandardTargeting is false. Sorry for the back and forth here @idettman.

Copy link
Member

@mkendall07 mkendall07 left a comment

Choose a reason for hiding this comment

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

just putting a block on this to prevent accidental merges until it can be updated.

@idettman
Copy link
Contributor Author

Will add logic to not add the variables if bidderSettings.sendStandardTargeting is false.

@idettman
Copy link
Contributor Author

Added logic to not add hb_cache_host if bidderSettings[bidderCode].sendStandardTargeting = false.

…ve add-video-cache-targeting-host-path value
@bretg
Copy link
Collaborator

bretg commented Apr 1, 2019

Docs PR prebid/prebid.github.io#1238

@bretg bretg removed the needs docs label Apr 1, 2019
Copy link
Member

@mkendall07 mkendall07 left a comment

Choose a reason for hiding this comment

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

LGTM

@bretg bretg removed the request for review from robertrmartinez April 2, 2019 16:33
@bretg bretg removed needs review pinned won't be closed by stalebot labels Apr 2, 2019
@bretg
Copy link
Collaborator

bretg commented Apr 2, 2019

Confirmed with Kendall that we're good to go here. Merging.

@bretg bretg merged commit c060a5c into prebid:master Apr 2, 2019
@idettman idettman added LGTM and removed needs 2nd review Core module updates require two approvals from the core team labels Apr 4, 2019
@idettman idettman deleted the add-video-cache-targeting-host-path branch June 5, 2019 23:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants