Core: Local cache for video bids #12598
Conversation
src/videoCache.js
Outdated
| vastsLocalCache.set(getLocalCacheBidId(bid), dataUri); | ||
| }; | ||
|
|
||
| export async function getLocalCachedBidWithGam(adTagUrl) { |
There was a problem hiding this comment.
This should be in the dfp module.
IMO the GAM flow in general needs to change, instead of calling dfp's buildVideoUrl and passing the URL to the player it should be something like getVast and pass the XML to the player, which should work regardless of whether one is using local cache, and should also be the same thing that's called by the video module (regardless of the type of cache in use).
It might help to update / add a non-video module example to flesh out how the new API should look.
src/videoCache.js
Outdated
|
|
||
| const gamVastWrapper = await response.text(); | ||
| const bidVastDataUri = vastsLocalCache.get(`${hb_bidder}_${hb_adid}`); | ||
| const mockUrl = LOCAL_CACHE_MOCK_URL + hb_bidder; |
There was a problem hiding this comment.
the "normal" cache generates a UUID which is then passed on to the creative and echoed back in the VAST.
I think the local cache could also generate a UUID, pass it along in the same way, and this could look for it in the response contents instead of imposing a different URL structure in the creative. It'd have the advantage of working with existing integrations, and being easier to switch into / out of.
|
Tread carefully! This PR adds 1 linter error (possibly disabled through directives):
|
|
We also need to update the functionality that triggers the BID_WON event, particularly for instream video scenarios handled within the instreamTracking module. |
| export async function getBidVastWithGam(adTagUrl, cacheMap = vastsLocalCache) { | ||
| const gamAdTagUrl = new URL(adTagUrl); | ||
| const custParams = new URLSearchParams(gamAdTagUrl.searchParams.get('cust_params')); | ||
| const videoCacheKey = custParams.get('hb_uuid'); |
There was a problem hiding this comment.
Id prefer not doing the URL parsing, since it couples this to the URL structure which is also tied to the ad server. Since the local cache generates the UUIDs, it seems like the response parser could look for them without first checking the URL.
| @@ -201,13 +201,17 @@ export function VideojsProvider(providerConfig, vjs_, adState_, timeState_, call | |||
|
|
|||
| // Plugins to integrate: https://github.com/googleads/videojs-ima | |||
| function setAdTagUrl(adTagUrl, options) { | |||
There was a problem hiding this comment.
It'd be cleaner if we had a separate setAdXml rather than a setAdTagUrl that doesn't actually set the ad tag url. (same with jwplayer)
| @@ -162,6 +175,20 @@ export function getCacheUrl(id) { | |||
| return `${config.getConfig('cache.url')}?uuid=${id}`; | |||
| } | |||
|
|
|||
| export const storeLocally = (bid) => { | |||
| const vastValue = getVastXml(bid); | |||
| const strategy = config.getConfig('cache.strategy') || BLOB; | |||
There was a problem hiding this comment.
Why expose these / make them mutually exclusive? I'd always generate a blob and translate it to a string only if/when we need to. It's less for the pub to understand and it also allows using different players with the same bid.
| bid.vastUrl = bidVastUrl; | ||
| bid.videoCacheKey = generateUUID(); |
There was a problem hiding this comment.
this should be shared with the "normal" cache flow - so that they don't get out of sync if the logic changes
Type of change
Description of change
Other information