prebid / Prebid.js

Setup and manage header bidding advertising partners without writing code or confusing line items. Prebid.js is open source and free.
https://docs.prebid.org
Apache License 2.0
1.33k stars 2.09k forks source link

PBJS not respecting bidLimit for video #7457

Closed spormeon closed 3 years ago

spormeon commented 3 years ago

I seem to have found some weirdness after adding a new bidder to "test", the new bidder is only using "test" params which are in the adapters git file. It seems to be pushing 2 different bidders in targeting. P5.14.0 with all the "recommended" modules included. Any idea whats gone haywire?

Google_Ad_Manager_-_Delivery_tools

spormeon commented 3 years ago

I took out the "test" bidder and its still doing it, so I think there is some weird bug, maybe around the "recommended modules" installed

spormeon commented 3 years ago

might be related to this, but i got no real clue: P5.13.0, PBjs Core (Targeting): bugfix for issue #7323 adding extra spaces (#7337)

gglas commented 3 years ago

Is the issue that the bidder name is being passed twice, in hb_bidder and hb_bidder_name ? We agree that that's a redundant kv pair and will triage separately -- but just want to make sure that's the actual problem you're seeing? What are the "two different bidders" in your screenshot? How many bidders are running in the auction? I think what we'd really need to troubleshoot would be an indicator of what precise behavior in the screenshot you're seeing that looks strange.

spormeon commented 3 years ago

hi There are 20+ bidders. Its not the same bidder being passed twice, its 2 different bidders and its random on which ones. Screenshot above is aniview & rhythmone. If @bretg is involved with this one, I can show on my test pages again ( like have shown on previous items)

bretg commented 3 years ago

We don't understand how you've posed the problem @spormeon. Yes, a test page is needed.

spormeon commented 3 years ago

here is another example, this one is sending 3 bidders. Sometimes it sends 1, sometimes 2, sometimes 3, its completely random. The problem it creates for us, is we build a vmap off of the back of the winning bids and if targeting is sending 3 lots aof bids in effect, it crashed out the vmap string as its to long to send and build the vmap off of. Its only started happening in P5.14.0
`{ "eventType": "setTargeting", "args": { "video10": { "hb_uuid": "cfc5e690-9851-4870-9065-89b5cfe544f8", "hb_cache_id": "cfc5e690-9851-4870-9065-89b5cfe544f8", "hb_cache_host": "prebid.adnxs.com", "hb_format": "video", "hb_size": "undefinedxundefined", "hb_pb": "0.73", "hb_adid": "185f4e0a49845a6", "hb_bidder": "videobyte", "hb_uuid_openx": "61591fe8-eb4d-4462-80c2-0ffe99799f20", "hb_size_openx": "640x480", "hb_pb_openx": "0.39", "hb_cache_path_openx": "/pbc/v1/cache", "hb_cache_path": "/pbc/v1/cache", "hb_cache_host_openx": "prebid.ams1.adnxs-simple.com", "hb_bidder_openx": "openx", "hb_uuid_rhythmone": "da1c7e34-5f13-4cd0-9fc9-58a7c98e487f", "hb_size_rhythmone": "640x480", "hb_pb_rhythmone": "0.36", "hb_cache_path_rhythm": "/pbc/v1/cache", "hb_cache_host_rhythm": "prebid.ams1.adnxs-simple.com", "hb_bidder_rhythmone": "rhythmone", "hb_source": "client", "hb_adomain": "advertiser.com" } }, "elapsedTime": 3219.8999999910593 }

i'll try and get you on slack @bretg and show you the test page etc

spormeon commented 3 years ago

I got a bit more info, I switched setup to:
debug: false, useBidCache: false, enableSendAllBids: true, targetingControls: { alwaysIncludeDeals: true }, sendBidsControl: { bidLimit: 1 }, aliasSyncEnabled: true, bidderSequence: 'random',

but targeting still randomly sets sometimes 1, sometimes 2,3 etc. Example with above bidlimit:1 set:

{ "eventType": "setTargeting", "args": { "video14": { "hb_uuid": "02134b55-88de-40d0-b639-b3abf0ba592f", "hb_cache_id": "02134b55-88de-40d0-b639-b3abf0ba592f", "hb_cache_host": "prebid.adnxs.com", "hb_format": "video", "hb_size": "1x1", "hb_pb": "2.38", "hb_adid": "47916cd3d16298d1", "hb_bidder": "rhythmone", "hb_uuid_rhythmone": "312f3f94-4f0b-4be7-8ac9-ce39ebc2962e", "hb_size_rhythmone": "1x1", "hb_pb_rhythmone": "2.65", "hb_cache_path_rhythm": "/pbc/v1/cache", "hb_cache_host_rhythm": "prebid.ams1.adnxs-simple.com", "hb_bidder_rhythmone": "rhythmone", "hb_uuid_districtm": "8e49eb4b-1e26-476e-97cd-71c076e37888", "hb_size_districtm": "1x1", "hb_pb_districtm": "3.44", "hb_cache_path_distri": "/pbc/v1/cache", "hb_cache_path": "/pbc/v1/cache", "hb_cache_host_distri": "prebid.ams1.adnxs-simple.com", "hb_bidder_districtm": "districtm", "hb_format_rhythmone": "video", "hb_adid_rhythmone": "47916cd3d16298d1", "hb_source": "s2s", "hb_adomain": "att.com" } }, "elapsedTime": 91176.20000000298 }

bretg commented 3 years ago

So would it be fair to describe the problem as "bidLimit doesn't work"? FWIW - alwaysIncludeDeals would of course break the bidLimit, but it doesn't appear that the extra bids you're getting are deals.

Maybe there's an issue with bidLimit when Prebid Server is involved.

Did you upgrade from 5.12 to 5.13 or from another version?

spormeon commented 3 years ago

i went 5.11 to 5.14. I've since also tried this and it still sends multiple: enableSendAllBids: false,

                    alwaysIncludeDeals: true,
                    allowTargetingKeys: ['BIDDER', 'AD_ID', 'PRICE_BUCKET', 'SIZE', 'ADOMAIN', 'UUID', 'CACHE_HOST', 'DEAL'],
                  sendBidsControl: {
                    bidLimit: 1
                  },

still sets target as: { "eventType": "setTargeting", "args": { "video1": { "hb_uuid": "2caaa11c-b331-4800-9253-0b79601b8bb6", "hb_cache_id": "2caaa11c-b331-4800-9253-0b79601b8bb6", "hb_cache_host": "prebid.adnxs.com", "hb_format": "video", "hb_size": "640x480", "hb_pb": "1.80", "hb_adid": "20416a6519bb84ae", "hb_bidder": "ix", "hb_uuid_districtm": "0012aca2-9190-41fb-93e1-7b3ce226750d", "hb_size_districtm": "1x1", "hb_pb_districtm": "3.81", "hb_cache_path_distri": "/pbc/v1/cache", "hb_cache_path": "/pbc/v1/cache", "hb_cache_host_distri": "prebid.ams1.adnxs-simple.com", "hb_bidder_districtm": "districtm", "hb_source": "client", "hb_adomain": "burgerking.co.uk" } }, "elapsedTime": 2544.7000000029802 } IX is client side, districtm is S2S but its random, could be 2 clients, could be 2 s2s etc

i'm on slack now, if you want to look at the test page?

bretg commented 3 years ago

Stepped through the test page and as a result, was reminded that dfpAdServerVideo module simply doesn't support any targeting controls:

It never has. Here's the code. Notice that someone (sounds like Patrick :-) added comments recognizing that there's an issue here. https://github.com/prebid/Prebid.js/blob/0d56358e107c8a14ef802fea271b75c34e4ff412/modules/dfpAdServerVideo.js#L261

  const prebidTargetingSet = Object.assign({},
    // Why are we adding standard keys here ? Refer https://github.com/prebid/Prebid.js/issues/3664
    { hb_uuid: bid && bid.videoCacheKey },
    // hb_cache_id became optional in prebid 5.0 after 4.x enabled the concept of optional keys. Discussion led to reversing the prior expectation of deprecating hb_uuid
    { hb_cache_id: bid && bid.videoCacheKey },
    allTargetingData,
    adserverTargeting,
  );

So, @spormeon - I can't explain why you don't see the issue in other PBJS versions because nothing's changed in video since at least 5.0. But here's a potential workaround: don't use the dfpAdServerVideo if you want to control how many adserver targeting variables are added, instead, use https://docs.prebid.org/dev-docs/publisher-api-reference/getAdserverTargetingForAdUnitCode.html, which does support the controls.

The main targeting.js file had changes:

Also, please note that your config for allowTargetingKeys and alwaysIncludeDeals isn't quite right -- they are attributes of the targetingControls object:

targetingControls: {
    alwaysIncludeDeals: true,
    allowTargetingKeys: ['BIDDER', 'AD_ID', 'PRICE_BUCKET',...]
  }

For the record, I'm not seeing the odd mixing of targeting as noted in the examples above, where the winning bid is listed as 'hb_bidder=ix' for instance yet no hb_pb_ix exists. Not saying you didn't see it, but I'm not familiar with the bidfilter chrome extension -- perhaps it's only showing some of the values? (could be wishful thinking)

@gglas - please consider adding this as a topic to the next PBJS prioritization meeting. Another potential bug I noticed is that getAllTargeting isn't returning hb_adid. This is a pretty important value. It's quite odd that we're emitting hb_pb_BIDDER, but not hb_adid_BIDDER in this scenario.

I think we need to have a team enhance the test cases for targeting.

spormeon commented 3 years ago

ok, so maybe this has just become more prevalent recently due to more bids and more bidders having been added etc, so its trying to send more KY's (its currently not a problem in production , just testing)

so basically dfpAdServerVideo module needs to take account of the targetingControls ? and then everything can be done from/ with the following?
targetingControls: { alwaysIncludeDeals: true, allowTargetingKeys: ['BIDDER', 'AD_ID', 'PRICE_BUCKET',...] }

I did try all this but obviously it didn't have an effect

@bretg Thanks for looking at the test page, hopefully its found something useful

bretg commented 3 years ago

Stepped through this again with @r-schweitzer . We found the logic in getAllTargeting isn't working as expected:

    var targeting = getWinningBidTargeting(adUnitCodes, bidsReceived)
      .concat(getCustomBidTargeting(adUnitCodes, bidsReceived))
      .concat(config.getConfig('enableSendAllBids') ? getBidLandscapeTargeting(adUnitCodes, bidsReceived) : getDealBids(adUnitCodes, bidsReceived))
      .concat(getAdUnitTargeting(adUnitCodes));

In words: 1) start with getWinningBidTargeting 2) add on getCustomBidTargeting 3) if enableSendAllBids then add getBidLandscapeTargeting otherwise add getDealBids 4) add on getAdUnitTargeting

Odd things:

patmmccann commented 3 years ago

why does dfp video module call get all targeting instead of getAdserverTargetingForAdUnitCode(adunitCode);

bretg commented 3 years ago

@mmoschovas pointed out some interesting things in slack:

1. bidLimit only applies to sendAllBids which is disabled in this case - meaning this is currently set to send highest (which is not working because of the next point)

2. adServerTargeting for a few bidders has both standard and bidder specific keys. There is a targeting concat that runs through a few steps : winning, customkeys, landscape (sendAllBids enabled)/deal (sendAllBids disabled), and any adUnitTargeting. On this test page, highest/winning is returned for the first, the bidders that have bidder specific keys for the second, and nothing for the third and fourth. So right now the reason there are extra bids is due to the custom key piece. From what I can tell, it's only PBS bidders that are causing this behavior as they seem to be the only bidders with the extra targeting due to adServer targeting being applied based on the bidder seatBid ext.prebid.targeting. I think a fix here would be to either updated the s2s adapter to exclude bidder specific versions of the standard keys, or another option may be to update the getCustomKeys function within targeting.js to filter the bidder specific versions of the standard keys

This points to a possible workaround @spormeon : in your s2sConfig.extPrebid, try adding the following values:

extPrebid: {
   targeting: {
      includebidderkeys: false,
      includewinners: false
   }
}

PBJS doesn't need PBS to supply the targeting. So if @mmoschovas is right, this will stop PBS targeting from interfering. And since PBS isn't doing the targeting, s2sConfig.extPrebid.targeting.pricegranularity can be removed.

There are default values in the pbsBidAdapter for these, but the code currently overwrites ext.prebid.targeting with whatever's in s2sConfig.extPrebid.targeting. The code is doing a shallow copy rather than a deep copy:

Object.assign(request.ext.prebid, s2sConfig.extPrebid);

Perhaps this ought to be changed to deepMerge?

spormeon commented 3 years ago

that "workaround" is there now ( on the test page) but throws: Invalid request: request.ext is invalid: ext.prebid.targeting: At least one of includewinners or includebidderkeys must be enabled to enable targeting support

i think i've got it right? prebidVideo4_29_0_js_—_Prebid_Publishers__Workspace_

spormeon commented 3 years ago

includewinners: true, seems to work. Its only sending the 1 bid now at least by the looks of it

bretg commented 3 years ago

We've opened PBS enhancement to make targeting optional -- https://github.com/prebid/prebid-server/issues/2030

To drive this issue home, I'd like to suggest two other followups:

1) @mmoschovas - does it make sense to update the pbsBidAdapter to do a deep-merge of ext.prebid.targeting from extPrebid rather than an object.assign? I think it does.

2) Issue #7538 tracks enhanced definition and testing around targeting

mmoschovas commented 3 years ago

@bretg yep makes sense to me

bretg commented 3 years ago

Followup items opened.