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

Add an option for Price Floors module to wait for currency #9493

Open piotrj-rtbh opened 1 year ago

piotrj-rtbh commented 1 year ago

Type of issue

Bug: the Price Floors module is not able to properly use the Currency module for converting currencies in bid floors if the exchange rates are taken from an external file (eg. the default one on jsdelivr)

Description

Currency conversion should take place after the currency rate file is loaded so to safely apply the actual rates. Only in certain cases (eg. file could not be loaded or a timeout) the rates should be taken from the currency.defaultRates property. We have noticed that once the currency file is cached then the exchange rates are taken from the file. It happens to load quickly right before the bidRequests are to be processed. Without caching the file is loaded but too late and the floors are calculated with the use of defaultRates only.

Steps to reproduce

  1. Build the bundle using your bidder adapter which is able to use Price Floors module (calls bidRequest.getFloor() method somewhere) and the bidder should accept USD; add also: currency and priceFloors modules
  2. Add it as your prebid.js file
  3. Setup standard prebid environment with one ad unit and one ad size (should be enough)
  4. Add the following to your ad unit:
    floors: {
      currency: "EUR",
      schema: {
        fields: ["mediaType"]
      },
      values: {
        banner: 0.3
      }
    },
  5. Make a setConfig call with params:
    pbjs.setConfig({
    currency: {
      adServerCurrency: "EUR",
      defaultRates: { EUR: { USD: 10 } }
    },
    floors: {}
    });

Test page

Codesandbox test page: https://efo8ku.csb.app/?pbjs_debug=true

Expected results

With caching turned on it works as expected. The messages are in that order:

MESSAGE: Emitting event for: requestBids
INFO: currencyRates set to {"dataAsOf":....
...
MESSAGE: Emitting event for: beforeRequestBids
...
MESSAGE: CALLING BIDDER
MESSAGE: Emitting event for: bidRequested
INFO: getCurrencyConversion using reciprocal EUR to USD conversionRate 1.0894
MESSAGE: Adding conversionCache value 1.0894 for EUR->USD
MESSAGE: Emitting event for: beforeBidderHttp

Actual results

With caching turned off the messages are in that order:

MESSAGE: Emitting event for: requestBids
...
MESSAGE: Emitting event for: beforeRequestBids
...
MESSAGE: CALLING BIDDER
MESSAGE: Emitting event for: bidRequested
INFO: getCurrencyConversion using direct EUR to USD conversionRate 10
MESSAGE: Adding conversionCache value 10 for EUR->USD
MESSAGE: Emitting event for: beforeBidderHttp
INFO: currencyRates set to {"dataAsOf":...

Platform details

Prebid 7.34.0, Chrome 109.0.5414.120, Windows 10, Node 14.20.0, npm 6.14.17

Other information

dgirardi commented 1 year ago

I believe this is intentional, with the assumption that waiting to load conversion rates will have more of an impact on revenue than using an inaccurate rate for floors.

The floors module has a setting, auctionDelay, that currently works as a timeout only for the case when floor data is loaded from the network. We could expand this to mean "wait up to this much for currencies to be loaded" as well.

Alternatively we could introduce a similar settings for the currency module - which would be more appropriate if there are other use cases for currency conversion before the first auction, which I am not aware of.

patmmccann commented 1 year ago

It seems OP is indeed proposing a currency file timeout. I suggest a default of 0 ms.

dgirardi commented 1 year ago

Are you suggesting something like this @patmmccann?

pbjs.setConfig({
   currency: {
       auctionDelay: [...]
   }
})

AFAIK right now it's only floors that needs currencies before the first auction, and the currency module never delays it, so this setting would only make sense for this particular situation (using both currency and floors with the delay for the latter). For this reason I am not sure that this is preferable to setConfig({floors: {auctionDelay}})

patmmccann commented 1 year ago

Goog point, but should setConfig({floors: {auctionDelay}}) control two dependencies, or should they each have their own timeout, eg setConfig({floors: {auctionDelay}}) and setConfig({floors: {auctionCurrencyFileDelay}})

dgirardi commented 1 year ago

I don't see a point in having two separate controls; if you are delaying the auction anyway, why would you give up on one or the other halfway?

dgirardi commented 1 year ago

Agreed to go this route:

pbjs.setConfig({
   currency: {
       auctionDelay: [...]
   }
})