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

PBJS not respecting bidLimit for video #7457

Closed
spormeon opened this issue Sep 23, 2021 · 19 comments
Closed

PBJS not respecting bidLimit for video #7457

spormeon opened this issue Sep 23, 2021 · 19 comments
Assignees
Labels

Comments

@spormeon
Copy link

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
Copy link
Author

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
Copy link
Author

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)

@ChrisHuie ChrisHuie self-assigned this Sep 23, 2021
@gglas
Copy link

gglas commented Sep 27, 2021

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
Copy link
Author

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
Copy link
Collaborator

bretg commented Sep 27, 2021

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

@spormeon
Copy link
Author

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
Copy link
Author

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
Copy link
Collaborator

bretg commented Sep 28, 2021

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
Copy link
Author

spormeon commented Sep 28, 2021

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
Copy link
Collaborator

bretg commented Sep 29, 2021

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

  • bidLimit
  • enableSendAllBids
  • allowTargetingKeys
  • alwaysIncludeDeals

It never has. Here's the code. Notice that someone (sounds like Patrick :-) added comments recognizing that there's an issue here.

function getCustParams(bid, options) {

  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:

  • 4.43 allowSendAllBidsTargetingKeys (you don't seem to be using this)
  • 5.9 allow non-string (eg. numeric) targeting segments
  • 5.13 remove extra spaces

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.

@bretg bretg changed the title 2 different bidders being pushed to dfp PBJS not respecting bidLimit for video Sep 29, 2021
@spormeon
Copy link
Author

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

@gglas gglas added the bug label Sep 29, 2021
@bretg
Copy link
Collaborator

bretg commented Sep 29, 2021

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:

  • What we see on the test page is that the bidder in the output of step 1 is different than the bidder in the output of step 4.
  • why is this calling both getWinningBidTargeting and getBidLandscapeTargeting? Does the latter avoid the winning bid?
  • why are deals getting added without checking alwaysIncludeDeals?
  • what even is getAdUnitTargeting? There aren't any comments

@patmmccann
Copy link
Collaborator

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

@bretg
Copy link
Collaborator

bretg commented Sep 30, 2021

@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
Copy link
Author

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
Copy link
Author

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

@bretg
Copy link
Collaborator

bretg commented Oct 5, 2021

We've opened PBS enhancement to make targeting optional -- prebid/prebid-server#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 Enhance targeting unit tests #7538 tracks enhanced definition and testing around targeting

@mmoschovas
Copy link
Contributor

@bretg yep makes sense to me

@bretg
Copy link
Collaborator

bretg commented Oct 15, 2021

Followup items opened.

@bretg bretg closed this as completed Oct 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants