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

Implementing prebid-cache to support Video ads #1277

Merged
merged 27 commits into from Jul 11, 2017

Conversation

Projects
None yet
3 participants
@dbemiller
Contributor

dbemiller commented Jun 7, 2017

PR which leverages prebid-server as a video ad cache. The goal here is to give publishers an ID which they can send to the ad server as a query param.

Per the feedback, I'm not exposing the callback to users anymore. The bidmanager simply waits to add the bid until the call to the cache succeeds.

@dbemiller dbemiller added the video label Jun 7, 2017

@protonate protonate self-requested a review Jun 7, 2017

@protonate protonate assigned protonate and dbemiller and unassigned protonate Jun 7, 2017

* cache ID has been returned.
*/
function prepVideoBid(bid) {
// Queue callbacks which arrive before the cache has returned.

This comment has been minimized.

@protonate

protonate Jun 8, 2017

Collaborator

We use queues of functions that are to be called when some async operation completes, including pbjs.cmd, events queue and the in-progress renderers PR for callbacks to fire once the renderer is loaded. It would be good to generalize this with a common module.

This comment has been minimized.

@dbemiller

dbemiller Jun 19, 2017

Contributor

This got deleted anyway... it's not necessary if we don't expose the callback to the user.

@protonate

This is looking good, I've a bunch of notes, many focused on the heavy lift of what we name things, and some callouts for later bikeshedding, but the big note would be the object.adServer and API fluctuations if more than one ad server.

* through the properties and functions on `pbjs.adserver`.
*
* If publishers bundle with more than one Ad Server, they'll need to interact with them through
* `pbjs.adservers.{adServerCode}`, where the adServerCode is defined by the ad server itself.

This comment has been minimized.

@protonate

protonate Jun 27, 2017

Collaborator

It would be good to keep the API consistent if the ad server to use could be determined through config or from the Bid object, Ad Unit, etc.

This comment has been minimized.

@dbemiller

dbemiller Jun 27, 2017

Contributor

Can it be? I'm all for deriving properties and avoiding data redundancies.

Which properties on the Bid or AdUnit could be used to derive it?

Also beware: that would prevent ad server functionality from being a module, since we wouldn't know which ad server was needed until runtime. Do you think that's a worthwhile tradeoff?

This comment has been minimized.

@mkendall07

mkendall07 Jun 27, 2017

Collaborator

We need to introduce a new config object I think.

This comment has been minimized.

@protonate

protonate Jun 27, 2017

Collaborator

I'd imagine the pub would be configured with an Ad Server definition (including "none") and an AdUnit could be configured with an override to use, if present.

This comment has been minimized.

@dbemiller

dbemiller Jun 27, 2017

Contributor

Chatted about this outside of the PR. I removed pbjs.adserver, so the API will always be pbjs.adServers.dfp.buildVideoUrl(options).

/**
* @typedef {Object} VideoSupport
*
* @property {string} code The identifying code for this adserver. Note that this contributes to

This comment has been minimized.

@protonate

protonate Jun 27, 2017

Collaborator

I know this is used throughout Prebid.js but would love to find another word to use in place of code. name, perhaps, and start migrating to that.

This comment has been minimized.

@dbemiller

dbemiller Jun 27, 2017

Contributor

Done. I do like name better than code... and this is a pretty good place to start using it, since the API is pretty internal.

* If this is the only code we've seen, attach functionality to $$PREBID_GLOBAL$$.adServer.
* If we've seen any other codes, attach functionality to $$PREBID_GLOBAL$$.adServers[code].
*
* @param {object} object The object where this function should manage namespaces.

This comment has been minimized.

@protonate

protonate Jun 27, 2017

Collaborator

This could be called adServers by getting rid of object.adServer default, thereby promoting object.adServers to just adServers. Then the API can be generic functions and use a strategy pattern to determine which ad server-specific functions to use, based on adUnit or page level config probably.

* @param {string} code The code for this ad server.
* @return {object} An object where functions for dealing with this ad server can be added.
*/
function prepareNamespaceIn(object, code) {

This comment has been minimized.

@protonate

protonate Jun 27, 2017

Collaborator

A strategy pattern would remove the need for much of the "default state management" code here.

I'm also unsure of the meaning of prepareNamespaceIn -- do you mean "in prebid with this code", or "in bound", or ... ?

This comment has been minimized.

@dbemiller

dbemiller Jun 27, 2017

Contributor

Not relevant anymore because I deleted it, but... it was supposed to read like "prepare a namespace in object for {code}".

I'm generally trying to name functions so that the code which calls them reads as closely to english as possible.

* the user-facing API. For example, if the adserver code is 'dfp', and the Prebid bundle contains two
* or more ad server modules, then publishers will access its functions through 'pbjs.adservers.dfp'.
*
* @method {VideoAdUrlBuilder} buildVideoAdUrl

This comment has been minimized.

@protonate

protonate Jun 27, 2017

Collaborator

I'd prefer @function rather than the alias/synonym, and consistency at the least.

This comment has been minimized.

@dbemiller

dbemiller Jun 27, 2017

Contributor

yeah that's fair. Actually I agree, cause in my mind, method seems to signify something that shouldn't be ripped off of the object, because it's relying on this. function signifies something that can be passed around freely.

I'll change it.

/**
* Takes the user-defined callback, and wraps it into our ajax function's API.
*
* This also needs to work around the fact that the cache server returns a 200 on "data not found"...

This comment has been minimized.

@protonate

protonate Jun 27, 2017

Collaborator

We should change this behavior server-side?

This comment has been minimized.

@dbemiller

dbemiller Jun 27, 2017

Contributor

The semantics are debatable...

As a server: someone just told you to return the data stored in the cache. If you looked in the cache and successfully found no value, did you complete the request successfully? What if someone told you to store "nothing", in one of their earlier put requests?

I answered that with a definite ¯_(ツ)_/¯, and decided to work with Sweeny's implementation.

This comment has been minimized.

@protonate

protonate Jun 27, 2017

Collaborator

A 204 would provide a definite "successful request with no content response".

This comment has been minimized.

@protonate

protonate Jun 27, 2017

Collaborator

I think my broader point is we also control the server and don't have to work around anything that doesn't make sense to us.

This comment has been minimized.

@dbemiller

dbemiller Jun 27, 2017

Contributor

I'd buy that. So trying to be clear, my understanding is that you think the cache should:

  1. If it doesn't recognize the key, return a 404.
  2. If it recognizes the key and stores "nothing", return a 204.
  3. If it recognizes the key and stores "something", return a 200 with the stored data.

Correct?

This comment has been minimized.

@dbemiller

dbemiller Jun 27, 2017

Contributor

er... and one other judgment call which needs agreement:

  1. If the videoCache can't find the video, it should be an error.

(I didn't see any discussion on this point... but it's an important one. If you agree, then a prebid-cache 204 "success" would still be considered an "error" from this file's point of view, and all this code would still be necessary anyway)

* Fetch the VAST XML associated with cacheId, and execute the callback function with it.
*
* @param {string} cacheId The ID of the video whose content should be retrieved.
* @param {videoCacheRetrieveCallback} callback A callback function which should be run once the

This comment has been minimized.

@protonate

protonate Jun 27, 2017

Collaborator

We've been conforming to calling this a done callback.

This comment has been minimized.

@dbemiller

dbemiller Jun 27, 2017

Contributor

Didn't know that... I'll update it

* Make a server call for the given cacheId, and execute the callback function when they get back to us.
*
* @param {string} cacheId The ID of the video whose content should be retrieved.
* @param {videoCacheRetrieveCallback} callback A callback function which should be run once the

This comment has been minimized.

@protonate

protonate Jun 27, 2017

Collaborator

Call this done?

This comment has been minimized.

@dbemiller

dbemiller Jun 27, 2017

Contributor

Sure

* [cacheId, cacheValue] pairs locally so that we don't go to the network again if
* the publisher requests the same VAST content more than once.
*/
function memoize(retrievalFunction) {

This comment has been minimized.

@protonate

protonate Jun 27, 2017

Collaborator

Great, can you add some info on when this is used? For example, I think it would only be when a bid is returned to an auction from the (pending) BidPool. Is my understanding accurate?

This comment has been minimized.

@dbemiller

dbemiller Jun 27, 2017

Contributor

It's used right below it, in the exported function :).

Per the docs, the goal here is to prevent multiple server calls.

If we successfully fetched a thing from the server, we can store it in page memory for the next time.

If the server fetch failed, we want to retry the call (might have just been a network problem).

It might have some more general uses, but... better private than public and only used once, imo. If refactoring, it's much easier to make more visible rather than less.

This comment has been minimized.

@mkendall07

mkendall07 Jun 28, 2017

Collaborator

I don't think we need this functionality at all. We don't actually invoke the read from the cache, the video player does. Am I missing something?

This comment has been minimized.

@dbemiller

dbemiller Jun 30, 2017

Contributor

You know... you're totally right.

Whenever I write something which interacts with an HTTP API, my first instinct is always "let's wrap that API in one from {my language} so that it's contained."

So.... I wrote it. Then wrote the video player integration. Didn't even notice that it was never called by real code.

Is deleted now.

* Initialize the global state so that the auction-space looks like we want it to.
*
* @param {Array} adUnits The array of ad units which should appear in this auction.
* @param {function} bidRequestTweaker A function which accepts a basic bidRequest, and

This comment has been minimized.

@protonate

protonate Jun 27, 2017

Collaborator

Best name yet!

const derivedParams = {
correlator: Date.now(),
description_url: encodeURIComponent(bid.descriptionUrl),

This comment has been minimized.

@mkendall07

mkendall07 Jun 28, 2017

Collaborator

Do we still need to send description_url? Should we be getting bid.vastUrl?

This comment has been minimized.

@dbemiller

dbemiller Jun 30, 2017

Contributor

Per the discussion in chat, I removed this. Publishers can still send it in themselves.

I don't think the vastUrl is related... but it's not needed by this point in the code anyway. The URL has been sent to the cache, so it's the { hb_uuid: bid.videoCacheKey } line which adds that to the DFP URL.

}
if (bidsBackAll()) {
exports.executeCallback();
if (isValid()) {

This comment has been minimized.

@mkendall07

mkendall07 Jun 28, 2017

Collaborator

I'd prefer to see this code block at the beginning of the addBidResponse method as it's easier to read the code that way, vs scanning and trying to see what block is not wrapped inside a function.

This comment has been minimized.

@dbemiller

dbemiller Jun 30, 2017

Contributor

Makes sense. Is fixed (after the next push)

const PUT_URL = 'https://prebid.adnxs.com/pbc/v1/put'
const GET_URL = 'https://prebid.adnxs.com/pbc/v1/get?';
const EMPTY_VAST_RESPONSE = '<VAST version="3.0"></VAST>'

This comment has been minimized.

@dbemiller

dbemiller Jul 5, 2017

Contributor

Looks like this disappears anyway. It was only being used in in store, which turned out to be unnecessary.

// Technically, this is vulnerable to cross-script injection by sketchy vastUrl bids.
// We could make sure it's a valid URI... but since we're loading VAST XML from the
// URL they provide anyway, that's probably not a big deal.
return '<VAST version="2.0"><Ad id=""><Wrapper><AdSystem>prebid.org wrapper</AdSystem><VASTAdTagURI><![CDATA[' +

This comment has been minimized.

@mkendall07

mkendall07 Jun 28, 2017

Collaborator

Are we supporting VAST 2 or VAST3?

This comment has been minimized.

@dbemiller

dbemiller Jul 5, 2017

Contributor

Per the chat, it doesn't sound like it matters. I'll bump it up to 3.0, since they're more likely to maintain backwards compatibility with that version if the standard ever moves forward any more.

// Technically, this is vulnerable to cross-script injection by sketchy vastUrl bids.
// We could make sure it's a valid URI... but since we're loading VAST XML from the
// URL they provide anyway, that's probably not a big deal.
return '<VAST version="2.0"><Ad id=""><Wrapper><AdSystem>prebid.org wrapper</AdSystem><VASTAdTagURI><![CDATA[' +

This comment has been minimized.

@mkendall07

mkendall07 Jun 28, 2017

Collaborator

Prefer string templates instead of concatenation.

This comment has been minimized.

@dbemiller

dbemiller Jun 30, 2017

Contributor

Makes sense

done(null, ids);
},
error: function(statusText, responseBody) {
done(new Error('Error storing video ad in the cache: ' + statusText + ': ' + JSON.stringify(responseBody)), []);

This comment has been minimized.

@mkendall07

mkendall07 Jun 28, 2017

Collaborator

string templates again.

This comment has been minimized.

@dbemiller

dbemiller Jun 30, 2017

Contributor

Fixed

* @return {Function} A callback which interprets the cache server's responses, and makes up the right
* arguments for our callback.
*/
function shimStorageCallback(done) {

This comment has been minimized.

@mkendall07

mkendall07 Jun 28, 2017

Collaborator

Don't love the idea of calling a function a shim

This comment has been minimized.

@dbemiller

dbemiller Jun 30, 2017

Contributor

I agree it's not the clearest, but... is there anything you like better? transform? wrap? Other?

I haven't come up with anything I really liked.

* [cacheId, cacheValue] pairs locally so that we don't go to the network again if
* the publisher requests the same VAST content more than once.
*/
function memoize(retrievalFunction) {

This comment has been minimized.

@mkendall07

mkendall07 Jun 28, 2017

Collaborator

I don't think we need this functionality at all. We don't actually invoke the read from the cache, the video player does. Am I missing something?

dbemiller added some commits Jun 27, 2017

Removed the shifty API. Updated unit tests. Renamed a property becaus…
…e Ive messed it up multiple times myself already during testing, expecting it to have a different name.
@@ -0,0 +1,132 @@

This comment has been minimized.

@protonate

protonate Jul 10, 2017

Collaborator

This can be moved to ./test as it isn't really an integration example.

// Video bids may fail if the cache is down, or there's trouble on the network.
function tryAddVideoBid(bid) {
store([bid], function(error, cacheIds) {
doCallbacksIfNeeded();

This comment has been minimized.

@mkendall07

mkendall07 Jul 10, 2017

Collaborator

Shouldn't we move this line after the bid is added to the auction? I realize that this is not consistent with the previous behavior, but at this point we should have a bid cached, so why not include it? @protonate @dbemiller

This comment has been minimized.

@protonate

protonate Jul 11, 2017

Collaborator

👍

@mkendall07 mkendall07 dismissed stale reviews from protonate and themself via da637df Jul 11, 2017

@mkendall07 mkendall07 merged commit 56bdd27 into master Jul 11, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@mkendall07 mkendall07 deleted the feature/video-cache branch Jul 11, 2017

jbAdyoulike added a commit to jbAdyoulike/Prebid.js that referenced this pull request Sep 21, 2017

Implementing prebid-cache to support Video ads (prebid#1277)
* Added code to cache video content, and made the cache key accessible through the bid.

* Renamed the method we add to the bid.

* Removed an extraneous function.

* Removed some accidental console.log messages

* Added unit tests for the video cache, and fixed a bug or two.

* Refactored the bidmanager a bit to help wrap my head around it, and changed video support so that only bids which have been cached are valid.

* Implemented video end-to-end. Waiting on the modules PR so that it can be transformed into a module.

* Better fix to the utils bug. Modernized the imports.

* Updated documentation.

* Added a few unit tests, and added fixtures for shared data structures.

* Added more thorough tests, and refactored a bit for better code reuse.

* Renamed a function.

* Added tests for adding video bids when the cache fails.

* Removed an unused import.

* Added tests for the adServerManager, and renamed some files for consistency with the normal conventions.

* Renamed the dfpVideo module, and added unit tests for it.

* Moved dfpAdServerVideo into the modules directory.

* Fixed a bug, and added a regression test to catch it in the future.

* Added a bare-bones example page.

* Made some cosmetic changes to names and documentation.

* Removed the shifty API. Updated unit tests. Renamed a property because Ive messed it up multiple times myself already during testing, expecting it to have a different name.

* Most code review comments. Still need to look into the details of VAST.

* Deleted some unused code, and upped the vast version to use 3.0 everywhere.

* Made the cache use the new endpoints

* Fixed style.

* Moved the test page into the tests directory.

* include the bid that ended the auction

dluxemburg added a commit to Genius/Prebid.js that referenced this pull request Jul 17, 2018

Implementing prebid-cache to support Video ads (prebid#1277)
* Added code to cache video content, and made the cache key accessible through the bid.

* Renamed the method we add to the bid.

* Removed an extraneous function.

* Removed some accidental console.log messages

* Added unit tests for the video cache, and fixed a bug or two.

* Refactored the bidmanager a bit to help wrap my head around it, and changed video support so that only bids which have been cached are valid.

* Implemented video end-to-end. Waiting on the modules PR so that it can be transformed into a module.

* Better fix to the utils bug. Modernized the imports.

* Updated documentation.

* Added a few unit tests, and added fixtures for shared data structures.

* Added more thorough tests, and refactored a bit for better code reuse.

* Renamed a function.

* Added tests for adding video bids when the cache fails.

* Removed an unused import.

* Added tests for the adServerManager, and renamed some files for consistency with the normal conventions.

* Renamed the dfpVideo module, and added unit tests for it.

* Moved dfpAdServerVideo into the modules directory.

* Fixed a bug, and added a regression test to catch it in the future.

* Added a bare-bones example page.

* Made some cosmetic changes to names and documentation.

* Removed the shifty API. Updated unit tests. Renamed a property because Ive messed it up multiple times myself already during testing, expecting it to have a different name.

* Most code review comments. Still need to look into the details of VAST.

* Deleted some unused code, and upped the vast version to use 3.0 everywhere.

* Made the cache use the new endpoints

* Fixed style.

* Moved the test page into the tests directory.

* include the bid that ended the auction
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment