currency module #1374

snapwich · 2017-07-12T16:12:55Z

Type of change

Feature

Description of change

Updated currency from #1209 to be in the new module format. Also replaced pbjs.setConfig with a currency configuration function; this can be updated later once we solidify on a configuration format for Prebid.js. The module should be included (gulp build --modules=currency) and then configured like so

pbjs.queue.push(function() {
  pbjs.currency({
    adServerCurrency: 'GBP',
    conversionRateFile: 'http://prebid.aws.rubiconproject.com/latest.json' // optional param, but we need to determine appropriate default
  });
  pbjs.requestBids( ... );
});

Adapters specify currency with the bid.currency property on the returned bid. The currency module decorates bidmanager.addBidResponse to defer sending through bids until the currency conversion file is loaded and their cpms can be converted. Bids that don't require currency conversion are immediately processed.

This pull-request also adds an additional optional second parameter to pbjs.setPriceGranularity called currencyMultiplier. This can be used to set the currency conversion rate from USD to the adServerCurrency that you used to generate your line items. Only needed if your ad server line items are in currency other than USD.

Other information

Thanks to Tom here at Rubicon Project for the initial implementation and @harpere for some modifications.

harpere

LGTM other than the conflicts that need to be resolved.

snapwich · 2017-07-17T17:05:12Z

resolved merge conflict

mkendall07 · 2017-07-17T18:10:00Z

modules/currency.js

+var currencyRatesLoaded = false;
+var adServerCurrency = 'USD';
+
+var oldBidResponse;


can you make this name more descriptive? I was confused about what it was until I saw line 51.

mkendall07 · 2017-07-17T18:13:50Z

modules/currency.js

+
+function wrapFunction(fn, context, params) {
+  return function() {
+    utils.logInfo('Invoking wrapped function', arguments);


not sure how helpful these info statements are as they are deep within internal implementation.

mkendall07 · 2017-07-17T18:16:14Z

modules/currency.js

+    if(bid) {
+      // execute immediately if the bid is already in the desired currency
+      if(bid.currency === adServerCurrency) {
+        return fn.apply(this, arguments);


does this have the unintended side effect of not invoking bidmanager.addBidResponse if bid === undefined ?

yes. I just modified this to call original if no bid, that way it can be responsible for all bad bids.

mkendall07 · 2017-07-17T18:16:39Z

modules/currency.js

+    utils.logInfo('Invoking addBidResponseDecorator function', arguments);
+
+    // default to USD if currency not set
+    if(bid && !bid.currency) {


can add checks here and line 91 at the top ( if(!bid) ) and exit immediately to reduce code complexity.

mkendall07 · 2017-07-17T18:20:38Z

modules/currency.js

+        }
+      } catch (e) {
+        utils.logWarn('Returning NO_BID, getCurrencyConversion threw error: ', e);
+        params[1] = bidfactory.createBid(STATUS.NO_BID);


this error condition will mess up the auction logic because it has a new bid.id not sure it's possible to fix though.

can I grab it off the original bid and pass it through to the bid factory?

mkendall07 · 2017-07-17T18:37:17Z

modules/currency.js

+    bidmanager.addBidResponse = addBidResponseDecorator(bidmanager.addBidResponse);
+  }
+
+  ajax(url, function(response) {


Should only load this once per page view.

I'll put a check so it won't load them if they're already loaded. However they can still reset the rates and then it'd have to reload them; i don't know why they'd do that, but at least they have that control.

mkendall07 · 2017-07-17T18:39:21Z

modules/currency.js

+  conversionCache = {};
+  currencySupportEnabled = true;
+
+  if(!oldBidResponse) {


nit: I think the current style is a space after if but the linter doesn't seem to be complaining about it being mixed. I would just run a fix on the file.

mkendall07 · 2017-07-17T18:42:05Z

src/prebid.js

@@ -6,7 +6,7 @@ import { videoAdUnit, hasNonVideoBidder } from './video';
 import { nativeAdUnit, nativeBidder, hasNonNativeBidder } from './native';
 import './polyfill';
 import { parse as parseURL, format as formatURL } from './url';
-import { isValidePriceConfig } from './cpmBucketManager';
+import { isValidPriceConfig } from './cpmBucketManager';


nice catch.

bretg · 2017-07-17T20:16:22Z

We shouldn't use prebid.aws.rubiconproject.com as the default conversion location.

Options:

use don7oq205h0l7.cloudfront.net - ugly, but neutral
establish a prebid.org DNS alias - e.g. currency-rates.prebid.org

My preference is for the second option, but don't know how to make that happen.

snapwich · 2017-07-24T17:40:07Z

Made updates based on feedback. Awaiting setConfig updates and DNS alias before merging.

muuki88 · 2017-07-25T14:35:15Z

Thanks @snapwich for this addition :)

If I understand the description correctly the currency module assumes that all SSPs use USD as a currency and the currency module translates to the ad server currency if not USD, right?

It doesn't handle cases where an SSP use different currencies?

snapwich · 2017-07-25T22:47:36Z

Hi @muuki88

If I understand the description correctly the currency module assumes that all SSPs use USD as a currency and the currency module translates to the ad server currency if not USD, right?

That is correct

It doesn't handle cases where an SSP use different currencies?

The bid adapters will have the ability to specify their currency by attaching bid.currency = 'GBP' etc to their bid responses. However, the bid adapters will have to be updated to support this new currency property.

As a stop-gap measure, I have just added a requested feature that allows you to specify currencies for bidders in the code configuration. It will be used something like this:

pbjs.setConfig({
  currency: {
    adServerCurrency: 'USD',
    overrides: {
      rubicon: 'GBP'
    }
  }
});

It should be noted that if the bid adapter does specify the new currency property, this override will be ignored (and log a warning).

dbemiller · 2017-08-03T13:52:11Z

modules/currency.js

+    utils.logInfo('Installing addBidResponse decorator for currency module', arguments);
+
+    originalBidResponse = bidmanager.addBidResponse;
+    bidmanager.addBidResponse = addBidResponseDecorator(bidmanager.addBidResponse);


Thinking over the issue @snapwich brought up here: I don't think we should support modules which monkey-patch functions in core.

Reason being: I've heard that we expect other people to write their own external modules. If so, then this behavior means that all of Prebid's internal APIs are now external APIs. If we remove a method or change a signature on anything we export, they could reasonably claim that it's a breaking change to the library.

I propose that we deliberately define all the "plugin points" in prebid-core, which are functions which we expect modules to call. For example, registerBidAdapter() is a plugin point today.

@snapwich @mkendall07 What do you guys think?

Off the top of my head, something like addBidPostprocessor((bid) => bid)) looks like a reasonable "plugin point" for this feature--although I'm not sure it fits with the spirit of the project. I just think that whatever it is, it should be explicit.

If we remove a method or change a signature on anything we export, they could reasonably claim that it's a breaking change to the library.

That's true but the same would be true if we created a plugin system. I do think creating a standardized way to implement a plugin to Prebid.js core would be nice though; although a big undertaking, so I wonder if we should wait until we have a few more use cases other than currency?

In the current state I'm not sure how we could do this without this decorator. I considered using the event system, but that would require mutating the bid objects, which seemed hacky; plus I don't think there would be any way using events to defer the bid response (which is required if currency file hasn't loaded yet).

If there's a better way to do the currently provided functionality I'm open to hearing it. Otherwise, maybe we could put a plugin system on the backlog for the future when we have more modules that require such features and we have a more formal insight into their needs?

It's possible the need for that plugin system is now though if we can't figure out a solution for this module and concurrent auctions.

oh yeah... I agree, a standardized way to implement plugins is way outside of the scope. I'm not even sure it's possible... but if it is, we definitely need more use-cases.

My suggestion would be something like this (not sure if you need sync or async functions... so I described both):

bidAdjustmentsRegistry.js: function newBidAdjustmentRegistry() { const adjustments = [] return { // adjuster should be a function of the type (bid, callback) => void register(adjuster) { adjustments.push(adjuster); }, adjust(bid, callback) { // execute each function in the queue, and do the callback on the result. Execute callback(bid) the input if no adjustments exist } } } export const bidAdjustments = newBidAdjustmentRegistry();

In the currency module, replace bidmanager.addBidResponse = addBidResponseDecorator(bidmanager.addBidResponse); with an equivalent bidAdjustments.register(impl). I think this is doable, but correct me if I'm wrong.

In #1421, @jaiminpanchal27 can call bidAdjustments.adjust(bid) inside the addBidResponse function. Or bidAdjustments.adjust(bid, addBidResponse) if it's async.

Does that sound like it'd work?

Hi. I'm relatively new to the project and have followed this discussion with a lot of interest.
We are currently facing the issue of converting currencies as well and this what we are planning to do,
which might be of interest as well.

Use bidderSettings

At this point using the the pbjs.bidderSettings to update our CPMs. If the currency module would simply be a library to call then integrating this would be a breeze. Our code looks something like this:

export const bidderSettings: prebidjs.IBidderSettings = { criteo: { bidCpmAdjustment: (bidCpm, bid) => { // do the currency calculation here return bidCpm; } }, appnexusAst: { bidCpmAdjustment: (bidCpm, bid) => { // do the currency calculation here return bidCpm; } } };

If the currency module can just be imported like

pbjs.currency.init(); // or even with a setting const cpmInEuro = pbjs.currency.convert(cpm, 'USD', 'EUR');

General approach for bid responses

Regarding a general plugin architecture. We have had very good experience with redux middlewares.
Basically you they are functions you can chain together. A BidResponseMiddleware would be a nice
way to abstract over all the stuff that is currently configured somewhere and applied somehow.

Also a middleware could do all the side-effects, which makes these things trivial to test, because
you can just swap them.

Example

A middleware could have the shape of

(bidResponse: BidResponse) => (bidResponse: BidResponse)

By definition a middleware applies some side effects and calls the next
middleware. A middleware can also change the bidResponse, e.g. with currency calculations
or other stuff.

// apply takes a list of middlewares and chains them const defaultMiddlewares = apply( new AdServerTargetingMiddleware(pbjs.bidderSettings), new BidCpmAdjustmentMiddleware(pbjs.bidderSettings), new MyCustomMiddleware(), );

It would also be nice to have access to the raw response from the actual SSP to work around
bugs or missing implementations. What do you think?

Thanks for the ideas @muuki88, but we've decided to move forward with the design as noted in #1089, modified slightly to use the new setConfig() approach. We think this has the advantage of preserving the bidCpmAdjustment for bidder-specific gross-vs-net conversions, separating currency which really isn't bidder-specific.

As for debugging, @snapwich has already implemented what we think is a pretty solid approach -- the bidResponse gets two new attributes: originalCpm and originalCurrency. Then cpm and currency are overwritten with the converted values. So you'll be able to access the before/after bidResponse attributes from the console.

snapwich · 2017-08-08T21:55:54Z

I've updated the pull-request with the latest changes to use setConfig.

The current configuration accepted is:

pbjs.setConfig({
  currency: {
    adServerCurrency: "GBP",  // only required field
    granularityMultiplier: 1.2,
    conversionRateFile: "localhost:3000/testCurrencyRates.json",
    bidderCurrencyDefault: {
      rubicon: "JPY"
    }
  }
});

The granularityMultiplier is part of the cpmBucketing code, but the rest is contained in the currency module.

@dbemiller I think a plugin system would be useful for Prebid.js in general if we want to start working on defining one that could be used in-place of addBidResponse decorator in this pull-request. I don't think it should hold up this request from being merged though as such a system doesn't become quite necessary until you start dealing with multiple extensions. One useful place to start looking: I am familiar with the plug-in system "Tapable" that Webpack uses (and is created by the same author). It might be a bit heavy for Prebid.js but maybe a similar API with less features would be appropriate.

bretg · 2017-08-09T02:55:37Z

@snapwich - going to need one more tweak - the default currency rate file will be https://currency.prebid.org/latest.json -- note the HTTPS rather than HTTP.

bretg · 2017-08-14T14:49:47Z

@GLStephen - we'll work on this enhancement. Unclear that currency will make Prebid 0.27 - if it does, then this won't make it. But will likely make 0.28 in either case.

# Conflicts: # src/bidmanager.js # src/prebid.js

snapwich · 2017-08-25T21:53:11Z

@bretg @GLStephen just added an option to specify the conversions manually in the config over an external file. It can be used like this:

pbjs.setConfig({
  currency: {
    adServerCurrency: 'USD',
    rates: {
      USD: {
        JPY: 100
      }
    }
  }
});

mkendall07 · 2017-08-28T18:45:32Z

modules/currency.js

+
+$$PREBID_GLOBAL$$.currency = setConfig;
+
+const DEFAULT_CURRENCY_RATE_URL = 'https://currency.prebid.org/latest.json';


you need to change this to http for now.

dbemiller · 2017-08-28T19:25:26Z

modules/currency.js

+import bidmanager from 'src/bidmanager';
+import { config } from 'src/config';
+
+$$PREBID_GLOBAL$$.currency = setConfig;


This should be JSDoc'd with types & all. It's a user-facing API.

oops. this is actually a legacy interface from before setConfig was implemented. good catch, i'll remove it and document the setConfig usage.

dbemiller · 2017-08-28T19:39:18Z

modules/currency.js

+  }
+}
+
+function resetCurrency() {


A reset() function (almost?) never makes sense. Any caller who wants to do:

var thing = newThing() ... thing.reset()

can always do:

var thing = newThing() ... thing = newThing()

instead.

They're functionally equivalent, but the latter is conceptually simpler. It's also easier to test.

To rigorously test an object with a reset(), you've effectively gotta run all your tests twice--to make sure all the state was reset properly.

To rigorously test an object with a new, you only need to run them once.

dbemiller · 2017-08-28T20:51:29Z

test/spec/modules/currency_spec.js

+describe('currency', function () {
+  describe('setConfig', () => {
+    it('results in currencySupportEnabled = false when currency not configured', () => {
+      setConfig({});


These tests pollute the global state with side-effects. Tests should be side-effect-free.

dbemiller · 2017-08-28T20:52:33Z

src/config.js

@@ -153,7 +153,7 @@ export function newConfig() {
  function getConfig(...args) {
    if (args.length <= 1 && typeof args[0] !== 'function') {
      const option = args[0];
-      return option ? config[option] : config;
+      return option ? utils.deepAccess(config, option) : config;


Update the JSDocs with the new functionality

dbemiller · 2017-08-28T20:53:00Z

src/utils.js

@@ -677,3 +677,14 @@ export function groupBy(xs, key) {
    return rv;
  }, {});
 }
+
+export function deepAccess(obj, path) {


This should have unit tests independent from the rest of the code.

* currency module * currency updates from feedback * support bidder currency override * update currency cdn to new prebid.org endpoint * update lint errors in currency * fix currency to use setConfig and change some variable names * updated currency endpiont to be secure * remove extra line in currency * change override name to default in currency * fix typo in price config * add currency.currencyMultiplier usage from setConfig * currencyMultiplier -> granularityMultiplier * allow currency.rates for conversion in currency over a file * changed https to http for now for currency endpoint * added tests for utils.deepAccess * added JSDocs for currency and utils.deepAccess * update config comment to mention deep access for getConfig

bretg · 2017-09-06T00:14:26Z

The prebid.org documentation for Currency should soon be reviewed and merged.

See prebid/prebid.github.io#323

When merged, it will be under http://prebid.org/dev-docs/modules/index.html

* currency module * currency updates from feedback * support bidder currency override * update currency cdn to new prebid.org endpoint * update lint errors in currency * fix currency to use setConfig and change some variable names * updated currency endpiont to be secure * remove extra line in currency * change override name to default in currency * fix typo in price config * add currency.currencyMultiplier usage from setConfig * currencyMultiplier -> granularityMultiplier * allow currency.rates for conversion in currency over a file * changed https to http for now for currency endpoint * added tests for utils.deepAccess * added JSDocs for currency and utils.deepAccess * update config comment to mention deep access for getConfig

* tag '0.28.0' of https://github.com/prebid/Prebid.js: (27 commits) Prebid 0.28.0 Release Revert "Upgrade sinon to 3.x (prebid#1491)" (prebid#1563) add () for correct order of operations in scaling increments for currency (prebid#1559) AppnexusAst adapter update: Added source and version to request payload (prebid#1555) remove unnecessary spread operator (prebid#1561) Adxcg adapter (prebid#1554) Upgrade sinon to 3.x (prebid#1491) Rename vastPayload to vastXml (prebid#1556) Single-size sizes array now can be taken, too (prebid#1535) Updated the istanbul-instrumenter-loader (prebid#1550) Add AerServ Adapter (prebid#1538) Fixed imports and made adform support aliasing (prebid#1518) Custom granularity fix (prebid#1546) Fix `documentation lint` issues (prebid#1544) Yieldbot adunit bidder params slot name usage fix (prebid#1394) Update serverbid adapter to use smartsync (prebid#1324) Add improvedigitalBidAdapter (prebid#1381) Fix prebid#1533 spring server typo (prebid#1542) userSync is off by default (prebid#1543) currency module (prebid#1374) ...

* currency module * currency updates from feedback * support bidder currency override * update currency cdn to new prebid.org endpoint * update lint errors in currency * fix currency to use setConfig and change some variable names * updated currency endpiont to be secure * remove extra line in currency * change override name to default in currency * fix typo in price config * add currency.currencyMultiplier usage from setConfig * currencyMultiplier -> granularityMultiplier * allow currency.rates for conversion in currency over a file * changed https to http for now for currency endpoint * added tests for utils.deepAccess * added JSDocs for currency and utils.deepAccess * update config comment to mention deep access for getConfig

mkendall07 added the in progress label Jul 12, 2017

ndhimehta assigned ndhimehta, harpere and mkendall07 and unassigned ndhimehta Jul 12, 2017

matthewlane added the needs review label Jul 12, 2017

harpere reviewed Jul 13, 2017

View reviewed changes

harpere previously approved these changes Jul 13, 2017

View reviewed changes

harpere added the LGTM label Jul 14, 2017

snapwich dismissed harpere’s stale review via 61a576b July 17, 2017 16:54

snapwich force-pushed the currency-module branch from cfffc4b to 61a576b Compare July 17, 2017 16:54

mkendall07 reviewed Jul 17, 2017

View reviewed changes

snapwich force-pushed the currency-module branch from 61a576b to a346de8 Compare July 24, 2017 17:39

snapwich mentioned this pull request Aug 1, 2017

Concurrent auctions for Prebid 1.0 #1421

Closed

dbemiller reviewed Aug 3, 2017

View reviewed changes

snapwich added 6 commits August 8, 2017 12:44

currency module

2794825

currency updates from feedback

ba0d29c

support bidder currency override

b44f994

update currency cdn to new prebid.org endpoint

bf5e92a

update lint errors in currency

d30b610

fix currency to use setConfig and change some variable names

5dfe5d9

snapwich force-pushed the currency-module branch from 94a6136 to 5dfe5d9 Compare August 8, 2017 21:44

bretg mentioned this pull request Aug 9, 2017

Currency module doc prebid/prebid.github.io#323

Merged

mkendall07 previously approved these changes Aug 16, 2017

View reviewed changes

snapwich added 2 commits August 16, 2017 15:42

remove extra line in currency

72a42ba

Merge branch 'master' into currency-module

dc82805

# Conflicts: # src/bidmanager.js # src/prebid.js

snapwich dismissed mkendall07’s stale review via dc82805 August 16, 2017 22:52

protonate self-requested a review August 17, 2017 15:06

dbemiller mentioned this pull request Aug 21, 2017

Intent to implement: Prebid Architecture #1510

Closed

snapwich added 6 commits August 23, 2017 09:49

change override name to default in currency

1c633d9

Merge branch 'master' into currency-module

74f500f

fix typo in price config

1742243

add currency.currencyMultiplier usage from setConfig

44fb5ec

currencyMultiplier -> granularityMultiplier

51dd8c3

allow currency.rates for conversion in currency over a file

296b84b

Merge remote-tracking branch 'upstream/master' into currency-module

95d5bb6

mkendall07 approved these changes Aug 28, 2017

View reviewed changes

changed https to http for now for currency endpoint

373b498

dbemiller reviewed Aug 28, 2017

View reviewed changes

snapwich added 3 commits August 28, 2017 15:33

added tests for utils.deepAccess

aa39ac7

added JSDocs for currency and utils.deepAccess

62cc571

update config comment to mention deep access for getConfig

f27e0f5

dbemiller approved these changes Aug 29, 2017

View reviewed changes

mkendall07 merged commit 1d3d26b into prebid:master Aug 29, 2017

robertrmartinez deleted the currency-module branch July 5, 2023 19:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

currency module #1374

currency module #1374

snapwich commented Jul 12, 2017

harpere left a comment

snapwich commented Jul 17, 2017

mkendall07 Jul 17, 2017

mkendall07 Jul 17, 2017

mkendall07 Jul 17, 2017

snapwich Jul 24, 2017

mkendall07 Jul 17, 2017

mkendall07 Jul 17, 2017

snapwich Jul 24, 2017

mkendall07 Jul 17, 2017

snapwich Jul 24, 2017

mkendall07 Jul 17, 2017

mkendall07 Jul 17, 2017

bretg commented Jul 17, 2017

snapwich commented Jul 24, 2017 •

edited

Loading

muuki88 commented Jul 25, 2017

snapwich commented Jul 25, 2017 •

edited

Loading

dbemiller Aug 3, 2017 •

edited

Loading

snapwich Aug 3, 2017

dbemiller Aug 4, 2017 •

edited

Loading

muuki88 Aug 8, 2017

bretg Aug 8, 2017

snapwich commented Aug 8, 2017 •

edited

Loading

bretg commented Aug 9, 2017

bretg commented Aug 14, 2017

snapwich commented Aug 25, 2017

mkendall07 Aug 28, 2017

dbemiller Aug 28, 2017

snapwich Aug 28, 2017

dbemiller Aug 28, 2017

dbemiller Aug 28, 2017

dbemiller Aug 28, 2017

dbemiller Aug 28, 2017

bretg commented Sep 6, 2017


		$$PREBID_GLOBAL$$.currency = setConfig;

		const DEFAULT_CURRENCY_RATE_URL = 'https://currency.prebid.org/latest.json';

currency module #1374

currency module #1374

Conversation

snapwich commented Jul 12, 2017

Type of change

Description of change

Other information

harpere left a comment

Choose a reason for hiding this comment

snapwich commented Jul 17, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bretg commented Jul 17, 2017

snapwich commented Jul 24, 2017 • edited Loading

muuki88 commented Jul 25, 2017

snapwich commented Jul 25, 2017 • edited Loading

dbemiller Aug 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbemiller Aug 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Use bidderSettings

General approach for bid responses

Example

Choose a reason for hiding this comment

snapwich commented Aug 8, 2017 • edited Loading

bretg commented Aug 9, 2017

bretg commented Aug 14, 2017

snapwich commented Aug 25, 2017

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

bretg commented Sep 6, 2017

snapwich commented Jul 24, 2017 •

edited

Loading

snapwich commented Jul 25, 2017 •

edited

Loading

dbemiller Aug 3, 2017 •

edited

Loading

dbemiller Aug 4, 2017 •

edited

Loading

snapwich commented Aug 8, 2017 •

edited

Loading