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.

Sign up for GitHub

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

Jump to bottom

Price in AUCTION_PRICE macro is not converted anymore #8319

Closed
acsbendi opened this issue Apr 26, 2022 · 6 comments
Closed

Price in AUCTION_PRICE macro is not converted anymore #8319

acsbendi opened this issue Apr 26, 2022 · 6 comments
Assignees
@patmmccann
Labels
bug

Comments

@acsbendi
Copy link
Contributor

Type of issue

Bug.

Description

#8171 introduced a significant breaking change which caused a serious bug in our systems (Kobler adapter) and possibly in other bidders as well:

  • Previously, AUCTION_PRICE macro was replaced by the final price in CPM, not the original price that the bidder sent. This meant that the price was in the currency of the publisher (see https://docs.prebid.org/dev-docs/modules/currency.html).
  • After the mentioned change, AUCTION_PRICE is replaced by the original price that the bidder sends, which may be in a different currency than what the publisher uses. An example for this is provided below. Also, since the price is exactly the same as the one sent by the bidder, this macro doesn't make sense anymore, the bidder could just include its price in the ad tag/ad tag URL itself without going through Prebid.

The previous functionality was much better as it avoided currency-conversion-related problems (we could record the exact same price that the publisher will charge us). However, the biggest problem here is that this macro, or more generally, prices in Prebid.js don't have an associated currency (e.g. there is no standard AUCTION_PRICE_CURRENCY macro to contain the currency of AUCTION_PRICE) so it's very easy to end up with bugs related to currency conversion.

Steps to reproduce

To verify it was working correctly before:

  • set Prebid's version to 6.15.0 (git checkout tags/6.15.0)
  • make the following changes in the repo (necessary to use Kobler adapter on localhost, see documentation)
  • run gulp build
  • run the test page provided below after replacing the source of script with the correct path of the build output
  • the price sent to https://dev-atag.essrtb.com/serve/prebid_ad_tag is in the correct currency, as shown in this screenshot:
    old

To reproduce the current erroneous behavior:

  • set Prebid's version to 6.16.0 (git checkout tags/6.16.0)
  • make the following changes in the repo (necessary to use Kobler adapter on localhost, see documentation)
  • run gulp build
  • run the test page provided below after replacing the source of script with the correct path of the build output
  • the price sent to https://dev-atag.essrtb.com/serve/prebid_ad_tag is NOT in the correct currency, as shown in this screenshot:
    new

Test page

<html>

<head>
    <link rel="icon" type="image/png" href="/favicon.png">
    <script async src="https://www.googletagservices.com/tag/js/gpt.js"></script>
    <script src="file:///C:/Users/Bendi/Documents/Kobler/Prebid.js/build/dist/prebid.js"></script>
    <script>
        var div_1_sizes = [
            [300, 250],
            [320, 250],
            [300, 600]
        ];
        var PREBID_TIMEOUT = 1000;
        var FAILSAFE_TIMEOUT = 3000;

        var adUnits = [
            {
                code: '/19968336/header-bid-tag-0',
                mediaTypes: {
                    banner: {
                        sizes: div_1_sizes
                    }
                },
                bids: [{
                    bidder: 'kobler',
                    params: {
                        placementId: 'k5H7et3R0'
                    }
                }]
            }
        ];

        // ======== DO NOT EDIT BELOW THIS LINE =========== //
        var googletag = googletag || {};
        googletag.cmd = googletag.cmd || [];
        googletag.cmd.push(function() {
            googletag.pubads().disableInitialLoad();
        });

        var pbjs = pbjs || {};
        pbjs.que = pbjs.que || [];

        pbjs.que.push(function() {
            pbjs.addAdUnits(adUnits);
            pbjs.setConfig({
                "currency": {
                    "adServerCurrency": "NOK"
                }
            });
            pbjs.requestBids({
                bidsBackHandler: initAdserver,
                timeout: PREBID_TIMEOUT
            });
        });

        function initAdserver() {
            if (pbjs.initAdserverSet) return;
            pbjs.initAdserverSet = true;
            googletag.cmd.push(function() {
                pbjs.que.push(function() {
                    pbjs.setTargetingForGPTAsync();
                    googletag.pubads().refresh();
                });
            });
        }
        // in case PBJS doesn't load
        setTimeout(function() {
            initAdserver();
        }, FAILSAFE_TIMEOUT);

        googletag.cmd.push(function() {
            googletag.defineSlot('/19968336/header-bid-tag-0', div_1_sizes, 'div-1').addService(googletag.pubads());
            googletag.pubads().enableSingleRequest();
            googletag.enableServices();
        });

    </script>

</head>

<body>
<h2>Basic Prebid.js Example</h2>
<h5>Div-1</h5>
<div id='div-1'>
    <script type='text/javascript'>
        googletag.cmd.push(function() {
            googletag.display('div-1');
        });

    </script>
</div>

</body>

</html>

Expected results

The price is in the currency used by the publisher (set as adServerCurrency), just like before.

Actual results

The price is in the currency that the bidder used for bidding.

Platform details

All Prebid versions are affected after and including 6.16.0.

Other information

Related PR: #8171
@acsbendi acsbendi changed the title AUCTION_PRICE macro is not converted anymore Price in AUCTION_PRICE macro is not converted anymore Apr 26, 2022
@patmmccann
Copy link
Collaborator

patmmccann commented Apr 26, 2022 •
edited
Loading

This is very interesting; the concern addressed in 8171 is that publishers should be able to adjust your bids however they want to compete in the auction without it raising or lowering what you pay them. In your previous code, a publisher could simply multiply all your bids by two and get paid double. If they cut them in half, you would pay them half. This matching is undesirable and reduced publisher flexibility.

How do you propose we fix that bug without causing this bug? It seems your win notification url should just accept a second macro?

@patmmccann patmmccann added the bug label Apr 26, 2022
@patmmccann patmmccann self-assigned this Apr 26, 2022
@acsbendi
Copy link
Contributor Author

acsbendi commented Apr 27, 2022 •
edited
Loading

@patmmccann I've been looking around the codebase, and it seems to me that there are three different prices, each for its own purpose:

  • bid.originalCpm: used to record the price the bidder responded with. Its currency is provided in bid.currency.
  • bid.cpm: the price the bidder will be charged. It may be different from bid.originalCpm, both in amount and currency.
  • adserverTargeting.hb_pb: ad server price (bucket), used to rank the bids and decide which of them wins the auction.

If my understanding is correct, the publishers are supposed to change adserverTargeting.hb_pb instead of bid.cpm if they want to give higher priority to some bidders while not charging them a higher price. Please correct me if I'm wrong.

There are two additional related issues:

  • the last two prices in the list above, and most importantly the second one, have no associated currency.
  • the names of these prices are not precise enough, for example, CPM is just a measurement. Also, there is not enough documentation on them.

The following changes could be made to address these:

  • instead of bid.originalCpm, we could have bid.originalPrice (or bid.originalPriceInCpm)
  • instead of bid.cpm, we could have bid.priceChargedToBidder (or bid.priceChargedToBidderInCpm). This would be the value of the standard AUCTION_PRICE macro, or alternatively a new macro could be introduced (e.g. CHARGED_PRICE).
  • we could introduce separate currencies for these prices: bid.originalPriceCurrency and bid.priceChargedToBidderCurrency
  • we could introduce standard macros for the currency too (AUCTION_PRICE_CURRENCY and possibly CHARGED_PRICE_CURRENCY).

I know changes like that are very hard to make since it would affect all the adapters, but I think it would be worth it in the long term to avoid bugs related to money, which are arguably the most serious ones.

@patmmccann
Copy link
Collaborator

patmmccann commented Apr 27, 2022 •
edited
Loading

There's a lot to unpack here.

First is, prebid is open source software, subject to edits by publishers before deployment, and by paying publishers based on what prebid decides you open your company up to many attacks in which your payables might be greatly inflated or otherwise manipulated. For example, a malware provider could buy a ton of ads on your exchange and then not pay for any of them by changing the win price to zero.

Second, re 'bid.cpm: the price the bidder will be charged.' That is not the meaning of this field. It is meant to be the price the publisher adjusts a bid to so it is comparable to other bids in the auction, potentially by unifying the currency or by multiplying each bid by an average discrepency for that bidder. It is a mistake to pay yourself this amount, as the publisher may choose to cut it in half to account for credit risk, or to double it because they wish to prefer a bidder. In this scenario, it seems you would simply double the price you pay the publisher. The more likely scenario is a publisher may wish to adjust all your bids downward by 2-5%, as they do for each ssp, in order to account for billing discrepencies. This adjustment would then deflate your payments, necessitating a further downward adjustment, in a neverending cycle.

Third, the Kobler bid adapter is resolving the macros in the kobler win notification. The other pull request was to address the point just above, where it seemed a bug was implemented in that the publisher adjusted price was considered the win price. There is no reason the kobler bid adapter could not resolve a currency macro eg

const winNotificationUrl = replaceAuctionCurrency(replaceAuctionPrice(bid.nurl, bid.originalCpm || cpm))

where replace auction currency is a function you define in your adapter. There would be no need for a currency macro convention, you could call it Beezlebub and it would work fine as long as your adapter function expected that.

However, publishers do not hold second price auctions, the existence of the price macro seems to be a holdover from days of lore when ssps did. You shouldn't open yourself up to malfeasance so easily; you should just fill this macro in, and likely encrypt it, before you transmit your win notification along, there is no good reason to have it filled in client side.

@acsbendi
Copy link
Contributor Author

Thank you for the thorough explanation! Based on it, the following is how I understand the whole picture now. The general recommendation is not to rely on Prebid.js for billing in any way. Instead, we just have to base our billing on the fact that Prebid is first-price always, and if we win, we have to pay exactly the price we bid. Please let me know if any of this is not correct or imprecise.

Assuming my understanding is correct, I have 2 remaining questions/suggestions:

  • Why do we still have AUCTION_PRICE as a macro? It's confusing because it makes it look like the bidder has to use it for billing. Replacing it with the original bidding price does not really make sense because it could just be done by the bidder itself without using a macro. So maybe it would be a good idea to deprecate it? If that's not possible, at least updating the documentation would help a lot too.
  • If we want to record the prices in the currency that the publisher will charge us in, is it correct that we have to do the currency conversion ourselves and not expect Prebid.js to help with that? Doesn't this cause unwanted differences due to using different currency rates?

@patmmccann
Copy link
Collaborator

patmmccann commented Apr 29, 2022 •
edited
Loading

I agree Auction Price should be deprecated, we're just exposing ourselves to vulnerabilities. I suppose it is to support the remaining ecosystem players that haven't taken action yet. I changed the doc as you suggested prebid/prebid.github.io#3743

Why would you allow the publisher to choose the exchange rate? Aren't publishers going to choose rates extremely favorable to themselves? This seems like another area you would open yourselves to vulnerability. You may bid in euros, and I may decide that euros are on parity with yen and pay you back in yen? This is obviously an extreme scenario, but it seems unwise for you to allow. If you are going to send back the price and currency the publisher is choosing, I certainly recommend preserving the original in another key value pair.

In fact, you could change your adapter back to have this behavior if you so choose, as long as you also pass the original cpm. We didn't intend to throw a wrench in your process, we thought we were benevolently correcting an oversight in your adapter code. You should not pay publishers on the adjusted prices for reasons expressed elsewhere in the thread, but if you want to record them, there isn't a rule against it I am aware of.

The concept of an ssp receiving a bill from a publisher with the publishers chosen currency and adjustments to any bid is quite unusual. Publishers typically receive a check from an ssp based on the ssp's calculations and do discounting so that the estimated payments match the actuals.

@acsbendi
Copy link
Contributor Author

acsbendi commented May 3, 2022

I think I understand everything now, thanks again.

The changed behavior that caused a problem for us was not actually in our adapter, it was the change in what's being substituted in the actual ad tag before serving (see here). We only use the win notifications for some extra logging, the billing is done based on the ad tag request.

The real solution in our case is just to bid in the publisher's currency so that no conversion will be necessary and of course, to only trust the price received in our own encrypted payload. And if conversion is necessary for any reason, we will also record the original price along with the converted one.

Repository owner moved this from Triage to Done in Prebid.js Tactical Issues table May 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@patmmccann patmmccann

Labels
bug
Projects
Prebid.js Tactical Issues table
Status: Done
Milestone
No milestone
Development

No branches or pull requests
2 participants
@patmmccann @acsbendi