Prebid.js icon indicating copy to clipboard operation
Prebid.js copied to clipboard

price floors priority should have adunit above setConfig

Open natanavra opened this issue 2 years ago • 9 comments

Type of issue

bug

Description

Setting a price floor on adunit level is not used if there's a matching price floor rule in setConfig (including default value). The priority should be:

  1. dynamic
  2. adunit
  3. setConfig

Steps to reproduce

pbjs.setConfig({
  floors: {
    config: {
      data: {
          modelVersion: 'initial',
          currency: 'USD',
          schema: {
            fields: ['gptSlot', 'size']
          },
          default: 0.2,
          values: { '*|*': 0.2 }
      }
    }
  }
});
pbjs.requestBids({
  adUnits: [{
    ...
    floors: {
      values: { '*|*': 5 }
    }
  }]
});

Expected results

Bids below 5 cpm should be rejected.

Actual results

Bids below 0.2 cpm are rejected

Other information

https://docs.prebid.org/dev-docs/modules/floors.html#Rule-Handling:~:text=The%20module%20uses,adUnit

natanavra avatar Aug 09 '22 10:08 natanavra

@robertrmartinez - can you comment on this? the code seems to be intentionally organized to use adUnit-level floors only if no global floor config is set.

https://github.com/prebid/Prebid.js/blob/ac5f36d648bc5a2e3e1d500c979c648585d28fed/modules/priceFloors.js#L364-L370

Because only one set of floor rules is associated with each auction, it does not seem trivial to switch the priority, since bids for different adUnits would need to use different rules.

dgirardi avatar Aug 09 '22 17:08 dgirardi

Yep, when the requirements were originally written it was always adUnit floors were the fallback if setConfig or fetch level floors were not available.

So this is expected.

I am sure we can add a way to override this of course.

robertrmartinez avatar Aug 09 '22 17:08 robertrmartinez

is this a documentation issue then? the order there seems arbitrary, I would think it should go adUnit > fetch > setConfig, if adUnits should have higher priority. Is the use case really to override floors for adUnit, or are you trying to override it for an auction @natanavra? the latter would be easier to implement, but it would need some new config syntax.

dgirardi avatar Aug 09 '22 17:08 dgirardi

Sounds like maybe a documentation issue, though this line in the docs seems to describe the functionality in my opinion:

image

When we first wrote the reqs, it was defined to behave as it does.

But I can see how one would want adUnit to override it or any version to override it.

But yes, of course it can be updated to handle other cases.

robertrmartinez avatar Aug 09 '22 18:08 robertrmartinez

You are correct, the behavior matches the documentation. (I thought I saw the order in the OP documented - but now I can't find it). @natanavra, is this a feature request, or do we suggest adUnits have higher priority somewhere else?

dgirardi avatar Aug 09 '22 18:08 dgirardi

@dgirardi the documentation states this is the priority, as seen in the link and the screenshot @robertrmartinez provided albeit takes a couple of reads to understand - if the bullet list was replaced with numbers it would be more clear. That means it's not a bug as it was intended.

With that said, from my POV adunit should have a higher priority as it's a more "specific" configuration. So I definitely think we should either change the priority, or add a configuration option on the adunit for example "topPriority: boolean"

In the meantime I've solved this with a filter function before sending the auctions to the ad server, but that doesn't mark the bid as rejected.

natanavra avatar Aug 10 '22 08:08 natanavra

@natanavra have you tried using floorMin set config right before calling request bids? We have similar goals, where we want to dramatically raise the model floor at the time of request and this achieves it

patmmccann avatar Aug 10 '22 15:08 patmmccann

This should work as of #8396

patmmccann avatar Aug 10 '22 15:08 patmmccann

adunit should have a higher priority as it's a more "specific" configuration

The intention was that adunit-level config is a static fallback in case the fetch fails. Dynamic data, if available, would be considered the more freshest and highest priority.

bretg avatar Sep 01 '22 13:09 bretg

Closing as won't do; the setConfig can be arbitrarily specific, and may indeed be more specific than the adunit.

patmmccann avatar Nov 29 '22 16:11 patmmccann