Adpod module #3511

jsnellbaker · 2019-02-04T20:30:33Z

Type of change

Feature
Does this change affect user-facing APIs or examples documented on http://prebid.org?

Description of change

This PR adds adpod module to prebid. It creates several hooked functions that are used to handle/evaluate adpod bids/adunits.

In terms of usage, this module would be ideally activated through another module (eg freeWheelAdserverVideo (see #3489)) by calling the initAdpodHooks() function somewhere in the file directly. Through this approach, the module would not need to be directly specified in the build process; it would automatically be pulled in when the ...AdserverVideo module is included.

Other information

Docs PR here

mkendall07 · 2019-02-04T20:40:36Z

This pull request introduces 2 alerts when merging c661a7a into 29dd87c - view on LGTM.com

new alerts:

2 for Invocation of non-function

Comment posted by LGTM.com

mkendall07 · 2019-02-05T14:07:33Z

This pull request introduces 2 alerts when merging 9c7d7e0 into 90afc1a - view on LGTM.com

new alerts:

2 for Invocation of non-function

Comment posted by LGTM.com

mkendall07 · 2019-02-05T15:02:41Z

This pull request introduces 2 alerts when merging 1a6cd39 into 90afc1a - view on LGTM.com

new alerts:

2 for Invocation of non-function

Comment posted by LGTM.com

mkendall07 · 2019-02-05T16:29:07Z

This pull request introduces 2 alerts when merging 2b7158f into 90afc1a - view on LGTM.com

new alerts:

2 for Invocation of non-function

Comment posted by LGTM.com

mkendall07 · 2019-02-05T19:07:22Z

This pull request introduces 2 alerts when merging 44eb4ec into 90afc1a - view on LGTM.com

new alerts:

2 for Invocation of non-function

Comment posted by LGTM.com

mkendall07 · 2019-02-05T21:03:11Z

This pull request introduces 2 alerts when merging 7380f08 into fbe766e - view on LGTM.com

new alerts:

2 for Invocation of non-function

Comment posted by LGTM.com

mkendall07 · 2019-02-06T15:43:26Z

This pull request introduces 2 alerts when merging db14fc6 into 16ae069 - view on LGTM.com

new alerts:

2 for Invocation of non-function

Comment posted by LGTM.com

mkendall07 · 2019-02-06T15:56:40Z

This pull request introduces 2 alerts when merging 69bc093 into 16ae069 - view on LGTM.com

new alerts:

2 for Invocation of non-function

Comment posted by LGTM.com

mkendall07 · 2019-02-06T16:24:58Z

This pull request introduces 2 alerts when merging 9860a56 into c2ef82b - view on LGTM.com

new alerts:

2 for Invocation of non-function

Comment posted by LGTM.com

jaiminpanchal27

Overall LGTM.
Few minor things

jaiminpanchal27 · 2019-02-06T15:43:07Z

modules/adpod.js

+ */
+export function initAdpodHooks() {
+  function setupHookFnOnce(hookId, hookFn, priority = 15) {
+    let result = hooks[hookId].getHooks({hook: hookFn});


What if some other module has added hook on checkAdUnitSetup and it is added first. Will these hooks be added since your are comparing the length to 0 ?

So they would be added to the hooks['checkAdUnitSetup'] object, but they wouldn't show when I execute the getHooks() call because I'm effectively filtering that get to only look for the function that's inside this module.

I tested this out locally and even when I name the hook function the same (ie checkAdUnitSetupHook) - the hooked function won't return with this type of call because the function signature is different.

jaiminpanchal27 · 2019-02-06T15:48:11Z

modules/adpod.js

+ */
+export function checkAdUnitSetupHook(fn, adUnits) {
+  let goodAdUnits = adUnits.filter(adUnit => {
+    let mediaTypes = adUnit.mediaTypes;


Suggestion: utils.deepAccess can be used here. deepAccess(adUnit, 'mediaTypes.video.context')

jaiminpanchal27 · 2019-02-06T16:15:18Z

modules/adpod.js

+ * @param {Object} bidderRequest copy of bid's associated bidderRequest object
+ */
+export function callPrebidCacheHook(fn, auctionInstance, bidResponse, afterBidAdded, bidderRequest) {
+  let videoConfig = bidderRequest.mediaTypes && bidderRequest.mediaTypes.video;


Suggestion: utils.deepAccess can be used here. deepAccess(bidderRequest, 'mediaTypes.video.context')

jaiminpanchal27 · 2019-02-06T17:46:42Z

modules/adpod.js

+  // note to reviewers - was doing this to ensure the variable wouldn't be malformed by the additional incoming bids
+  // while the current list is being processed by PBC and the store callback
+  // ...but is this copy really needed?
+  let bidListCopy = bidList.slice();


Can be removed. No need to copy

jaiminpanchal27 · 2019-02-06T17:49:37Z

modules/adpod.js

+    } else {
+      for (let i = 0; i < cacheIds.length; i++) {
+        // when uuid in response is empty string then the key already existed, so this bid wasn't cached
+        // TODO? - should we throw warning here?  should we call the afterBidAdded() anyway to decrement the internal bid counter?


We should call afterBidAdded to decrement the counter because there can be a case where auction is not timedout and one of the bid is back with empty uuid. In this case we would like to close the auction and not wait for timer to expire

To confirm on this point - should we throw any type of message that the bid wasn't cached b/c it had a duplicate key? Or just let it go silently?

yep logMessage is good

jaiminpanchal27 · 2019-02-06T17:52:28Z

modules/adpod.js

+      for (let i = 0; i < cacheIds.length; i++) {
+        // when uuid in response is empty string then the key already existed, so this bid wasn't cached
+        // TODO? - should we throw warning here?  should we call the afterBidAdded() anyway to decrement the internal bid counter?
+        // TODO? - verify the cacheKey is one of the expected values (eg in case PBC returns generic UUID)?


We can ignore this check

jsnellbaker · 2019-02-07T14:16:20Z

@jaiminpanchal27 I have made several updates in lieu of your feedback; please take another look when you have the chance.

idettman

LGTM

mkendall07

LGTM. One requested change on the bid duration rounding behavior

mkendall07 · 2019-02-20T18:32:02Z

modules/adpod.js

+import find from 'core-js/library/fn/array/find';
+const from = require('core-js/library/fn/array/from');
+
+export const TARGETING_KEY_PB_CAT_DUR = 'hb_pb_cat_dur';


kind of late but are we happy with this name? :)

mkendall07 · 2019-02-20T18:32:23Z

modules/adpod.js

+export const TARGETING_KEY_PB_CAT_DUR = 'hb_pb_cat_dur';
+export const TARGETING_KEY_CACHE_ID = 'hb_cache_id'
+
+let queueTimeDelay = 50;


any experimentation with different default values here?

I did test using different values (went to 500) and with the timers I setup at different points in this module - it was executing the call to Prebid Cache at the set time value when there was a sufficient gap in bids.

mkendall07 · 2019-02-20T18:36:15Z

modules/adpod.js

+      // create parent level object based on auction ID (in case there are concurrent auctions running) to store objects for that auction
+      if (!registry[bid.auctionId]) {
+        registry[bid.auctionId] = {};
+        registry[bid.auctionId].bidStorage = new Set();


nice use of a new data type!

mkendall07 · 2019-02-20T18:42:12Z

modules/adpod.js

+ *  - only bids that exactly match those listed values are accepted (don't round at all).
+ *  - populate the `bid.video.durationBucket` field with the matching duration value
+ * when adUnit.mediaTypes.video.requireExactDuration is false
+ *  - round the duration to the next highest specified duration value based on adunit (eg if range was [5, 15, 30] -> 3s is rounded to 5s; 16s is rounded to 30s)


we wanted to add a 2 second buffer here. That is if a bid is 16s it should be rounded down to the the nearest bucket (15s in this case) because it is within 2s of a bucket.

I'll make an update here to incorporate this logic. Should this buffer apply to the highest range? ie if the ranges were [15, 30, 45, 60] and the bid was 61 - we would round it down to 60 and accept it?

yeah I think we can apply it to the highest value as well.

I updated the logic to account for this buffer; please take another look when you have the chance.

…egory field

jsnellbaker added 6 commits January 30, 2019 15:52

initial version of adpod module

2896f90

fix hooks, clean-up and uncomment unit tests

01369b5

correct issues in unit tests

99fdaae

add missing custom key fields in fake bids for unit tests

0d7d2af

several updates to module and unit tests

dcd4056

update logic to only add hooks once

c661a7a

jsnellbaker added the on hold label Feb 4, 2019

jsnellbaker requested review from matthewlane, mkendall07 and jaiminpanchal27 February 4, 2019 20:30

update targeting keys to new name

9c7d7e0

add module description

1a6cd39

fix typo

2b7158f

jaiminpanchal27 added the video label Feb 5, 2019

add adunit check in case of multi-format

44eb4ec

jaiminpanchal27 added adpod and removed video labels Feb 5, 2019

add exports for adpod targeting keys

7380f08

move ADPOD constant to mediaTypes.js

db14fc6

undo mediaTypes change

69bc093

Merge branch 'master' into adpod_module

9860a56

jaiminpanchal27 requested changes Feb 6, 2019

View reviewed changes

jsnellbaker added 3 commits February 6, 2019 14:09

Merge branch 'master' into adpod_module

9131554

fix unit tests

f655c5c

updates based on initial feedback

92823cc

matthewlane approved these changes Feb 6, 2019

View reviewed changes

add msg when PBC rejects bid due to duplicate key

b74d7a3

jsnellbaker removed the on hold label Feb 7, 2019

jaiminpanchal27 approved these changes Feb 8, 2019

View reviewed changes

jaiminpanchal27 added the LGTM label Feb 8, 2019

fix issue in videoBidCheck hook

de58a2b

jsnellbaker requested review from robertrmartinez and idettman February 14, 2019 15:30

idettman approved these changes Feb 19, 2019

View reviewed changes

mkendall07 approved these changes Feb 20, 2019

View reviewed changes

jsnellbaker added 3 commits February 20, 2019 16:12

add buffer logic when rounding bidDuration

ad1c678

add logic for deferCaching

1e1e49b

update logic around brandCategoryExclusion

b60cea7

jaiminpanchal27 approved these changes Feb 26, 2019

View reviewed changes

jsnellbaker added 2 commits February 27, 2019 11:46

update support to new hook api and update logic when checking iab cat…

9728f2f

…egory field

merge branch 'master' to branch adpod_module + resolve conflicts

6dfc923

jaiminpanchal27 merged commit e96d58e into master Feb 27, 2019

jsnellbaker deleted the adpod_module branch March 18, 2019 13:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adpod module #3511

Adpod module #3511

jsnellbaker commented Feb 4, 2019

mkendall07 commented Feb 4, 2019

mkendall07 commented Feb 5, 2019

mkendall07 commented Feb 5, 2019

mkendall07 commented Feb 5, 2019

mkendall07 commented Feb 5, 2019

mkendall07 commented Feb 5, 2019

mkendall07 commented Feb 6, 2019

mkendall07 commented Feb 6, 2019

mkendall07 commented Feb 6, 2019

jaiminpanchal27 left a comment

jaiminpanchal27 Feb 6, 2019

jsnellbaker Feb 6, 2019 •

edited

Loading

jaiminpanchal27 Feb 6, 2019

jaiminpanchal27 Feb 6, 2019

jaiminpanchal27 Feb 6, 2019

jaiminpanchal27 Feb 6, 2019

jsnellbaker Feb 6, 2019

jaiminpanchal27 Feb 6, 2019

jaiminpanchal27 Feb 6, 2019

jsnellbaker commented Feb 7, 2019

idettman left a comment

mkendall07 left a comment

mkendall07 Feb 20, 2019

mkendall07 Feb 20, 2019

jsnellbaker Feb 20, 2019

mkendall07 Feb 20, 2019

mkendall07 Feb 20, 2019

jsnellbaker Feb 20, 2019

mkendall07 Feb 20, 2019

jsnellbaker Feb 20, 2019

Adpod module #3511

Adpod module #3511

Conversation

jsnellbaker commented Feb 4, 2019

Type of change

Description of change

Other information

mkendall07 commented Feb 4, 2019

mkendall07 commented Feb 5, 2019

mkendall07 commented Feb 5, 2019

mkendall07 commented Feb 5, 2019

mkendall07 commented Feb 5, 2019

mkendall07 commented Feb 5, 2019

mkendall07 commented Feb 6, 2019

mkendall07 commented Feb 6, 2019

mkendall07 commented Feb 6, 2019

jaiminpanchal27 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsnellbaker Feb 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsnellbaker commented Feb 7, 2019

idettman left a comment

Choose a reason for hiding this comment

mkendall07 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsnellbaker Feb 6, 2019 •

edited

Loading