prebid / prebid-server

Open-source solution for running real-time advertising auctions in the cloud.
https://prebid.org/product-suite/prebid-server/
Apache License 2.0
432 stars 739 forks source link

Open RTB: Add currency support #280

Closed sandraleon closed 5 years ago

sandraleon commented 6 years ago

This issue is to track adding currency support in Prebid Server.

There will be a new currency object located under bidrequest.ext:

    "currency": {
        "rates": {
            "USD": {
                "JPY": 110
            }
        }
    }

The ad server currency must be set either in bidrequest.cur (must be one ISO 4217 alpha codes string in array)

    "cur": ["USD"],

or in config file

    "ad_server_currency": "USD",

The currency specified in the bid request will take precedence.

Bidders who support multiple currencies will have to specify the currency for their bids. The currency set (in the request or config) will be provided to bidders so that they can pass that info on to their bidder servers.

Bid price gets converted based on conversion rates available. Conversion rates can be provided in the bid request as shown above. If rates are not provided there, the latest conversion rates will be fetched.

dbemiller commented 6 years ago

Overall, this plan looks great! There are two design points I'd like to make some suggestions & raise discussion about.

Default currency

If no currency is specified in the request then USD is assumed.

In the OpenRTB spec, they define bidrequest.cur to be:

Array of allowed currencies for bids on this bid request using ISO-4217 alpha codes. Recommended only if the exchange accepts multiple currencies.

Whenever there are recommendations, our endpoint tends to stay consistent with the OpenRTB spec, but also more strict than it. This gives us the flexibility to "require fewer things" in the future without breaking backwards compatibility.

In this case, I think the better behavior would be to "require bidrequest.cur to exist and be an array of size 1".

granularityMultiplier

In Prebid.js right now, I see a few ways to set price granularity:

  1. Choose one of the pre-defined buckets (low, med, high, etc)
  2. Define custom buckets explicitly (by passing min, max, and interval)
  3. Set a currency conversion + granularityMultiplier.

It looks like this is designed to port (3) into PBS... but I'm not sure that's the best way.

Once we have (2), I'm not sure that the cost of (1) or (3) is worth the value they add. This is how I see the breakdown:

Costs

Values

Unfortunately, we have to support (1) for legacy reasons. Those strings are in the DB as configs, so we can't drop support for it until some data has been migrated. But... I don't know of any pressure on us to support (3).

Is that all accurate? If so, I think we should add API support for (2) instead, and avoid (3) altogether.

dbemiller commented 6 years ago

It looks like #281 has stalled. It's not in our immediate priorities... so I'll describe the points that surfaces there for posterity, in case anyone wants to take it on.

This is really two feature requests. These could easily be implemented separately, to keep the work in small chunks.

  1. If request.cur contains some acceptable currencies, PBS should be able to auto-convert bids into one of those currencies using some sensible default rates
  2. If request.prebid.ext.currency.rates and request.cur are defined, publishers should be able to specify their own currency conversion rates. In this case, the values from (1) will be ignored.

If (2) is implemented without (1), then request.cur should be required any time request.prebid.ext.currency.rates is defined.


For (1), there are some acceptance criteria for general feature performance.

  1. Conversion rates are be fetched from GET http://currency.prebid.org/latest.json
  2. The rates must be fetched before the /status endpoint signals that PBS is ready to respond to auctions.
  3. The rates must be updated periodically to stay "fresh". This must be done in the background--not during someone's call to /auction or /openrtb2/auction

Other notes:

FlorentFlament commented 6 years ago

Hi,

I have recently been looking at how prebid-server connectors are implemented. And as far as I understand, adaptors can't specify which currency has been used by the corresponding bidder.

The MakeBids method of the Bidder interface essentially returns an array of openrtb.Bid (https://github.com/prebid/prebid-server/blob/master/adapters/bidder.go#L70 & https://github.com/mxmCherry/openrtb/blob/master/bid.go). These objects don't have any currency field. The openrtb struct that actually contains the bid currency is the openrtb.BidResponse (https://github.com/mxmCherry/openrtb/blob/master/bid_response.go#L48).

This may be an issue, because the MakeRequests method receives an openrtb.BidRequest as parameter, which can have a list of currencies that the bidder may use (https://github.com/mxmCherry/openrtb/blob/master/bid_request.go#L145). In this case, it is impossible for the adaptor to tell prebid-server which currency has been actually used by the bidder, therefore the bid prices can't be interpreted.

With this in mind, I am wondering if the MakeBids method shouldn't return an openrtb.BidResponse instead of the []*TypedBid ? Otherwise, a quick fix to this issue would be to add an optional currency field in the TypedBid struct (even though this field will probably be the same for all the bids returned by the bidder).

I would be interested in having your thoughts about this. Regards, Florent Flament

dbemiller commented 6 years ago

hey @FlorentFlament. Yes, that's a perfect summary of the problem.

I don't think MakeBids should return a full BidResposne, though, because that object has other fields which the PBS core has no straightforward way to interpret. For example, suppose that the BidResponse contained multiple SeatBids. When PBS core builds the SeatBid for the Bidder, what would it do with all that extra info?

I suppose we could discard it, or stuff it into seatbid.ext.bidder somewhere... but I think that introduces a lot more complexity and conceptual weight than it's worth.

I like the idea of putting it inside TypedBid better, but you also make a good point here:

even though this field will probably be the same for all the bids returned by the bidder

Perhaps MakeBids could just return a new type with the info we want? For example:

type BidderResponse struct{
  Currency string
  Bids []*TypedBid
}
FlorentFlament commented 6 years ago

Hi @dbemiller ,

This struct looks good.

To be perfectly honest, I would personally prefer having the adapter return a "constrained" openrtb.BidResponse, rather than creating new custom struct, because openrtb structures already contain most fields that we need (so why not using them?). Besides, it would create a beautiful symmetry between the MakeRequests method taking an openrtb.BidRequest and the MakeBids method taking an openrtb.BidResponse.

That said, I must admit that pragmatically speaking, the structure you are proposing is much quicker to implement, and clearly does the job.

Your choice !

dbemiller commented 6 years ago

I think we share many of the same intuitions :). Symmetry is great, and reusing standard types is also great.

But... I do think it's still better to define a custom type here. If there's one place where it's important to minimize the learning curve, it's on "how to contribute a new Bidder." Streamlined types are the best way to communicate what the Bidder's responsibilities are.

Also: we've got some other reasons to move away from the openrtb library onto our own types in the future. Those Ext json.RawMessage types make us write some nasty and inefficient code to separate layers of the application. It's already going to be a chore, but I'm not too keen to add more dependency on it. Homegrown types can be moved & renamed through IDE refactoring much more easily.

FlorentFlament commented 6 years ago

Fair enough, I like the idea of "minimizing the learning curve" by using streamlined types. As for the need to write inefficient code because of the structures used by the openrtb library, I trust you since I didn't dig enough into the prebid-server code to observe it.

So I'd say let's go for the BidderResponse struct ! (A colleague of mine is really willing to do a pull request for this, so I'll let him do if that's ok for you ; )

dbemiller commented 6 years ago

That would be wonderful!

Feel free to send in smaller PRs too, if they find good places to break it up. We're trying to be pretty quick on reviews here, but small PRs are always easier than larger ones as long as they make sense as standalone changes.

benjaminch commented 6 years ago

Hello guys,

Just created the pull request introducing currency support in bid responses.

Let me know what do you think about it :)

dbemiller commented 6 years ago

hey @benjaminch @FlorentFlament ... I got asked about this a few times recently. Is criteo still working on it, or has it stalled for the time being?

benjaminch commented 6 years ago

Hey @dbemiller,

I would love to indeed ! I was thinking adding safe rejection in case bid response doesn’t have the proper currency (set in the request), I put couple todos in my last PR on this matter.

What are the nexts steps your opinion ?

Cheers,

dbemiller commented 6 years ago

Awesome :).

That sounds like a good next step. Broadly speaking, I think these are the two highest priorities:

  1. As you said, reject bids which aren't valid ISO codes or which aren't included in request.cur.

  2. In endpoints/auction.go, use ["USD"] as a default if request.cur doesn't exist. Document it as a deviation from OpenRTB in the docs.


(1) is a breaking change... so the sooner it happens, the better. We'll need to leave the PR open for a while so that Bidders have time to update their adapters/servers. (2) is important because (1) will reject all bids if empty cur arrays are allowed.

benjaminch commented 6 years ago

Hello @dbemiller, just created a new PR addressing the first currency rejection support step 1. Let me know what you think !

Cheers

benjaminch commented 6 years ago

Hey @dbemiller,

Looking on this one, what's the plan then? Is the conversion rate mechanism is still on the table? If so, what's the standard use case for which a conversion needs to be applied?

The use case I can think of: Bid request comes allowing only EUR, the bidder answer with USD. As of today PBS will reject this bid, but with the rate conversion feature, PBS will be able to convert the USD to EUR so PBS can treat the bid. I am wondering if a setting on bidder's side should be available in case a given bidder doesn't want to have his bids converted (as it might include some discrepancies due to conversions rate update)?

Cheers

dbemiller commented 6 years ago

Is the conversion rate mechanism is still on the table?

Absolutely! And yes, your use case is a strong one.

The Prebid.org has some standard currency conversions which it updates daily at http://currency.prebid.org/latest.json. We should fetch those on PBS startup, store them in memory (careful of race conditions), and update it in a background process which runs every 24 hours too.

I am wondering if a setting on bidder's side should be available in case a given bidder doesn't want to have his bids converted (as it might include some discrepancies due to conversions rate update)

That's a very good question... to be honest, I don't think anyone's discussed it. Was this just a passing idea, or is Criteo planning to add an adapter and wants the option to disable it?

benjaminch commented 6 years ago

Thanks for your answer @dbemiller !

Was this just a passing idea, or is Criteo planning to add an adapter and wants the option to disable it?

Well, I didn't talked about it with other folks here, but from past experience, I think it's always better to have a safety guard just in case. If bidders are worried about this conversion rate stuff, at least they can desactivate it (especially when it occurs under the hood).

Regarding the adapter, we are working on it. I don't have any ETA for the time being though. It doesn't prevent us to try to help on the PBS backlog, so happy to help :)

dbemiller commented 6 years ago

Well, I didn't talked about it with other folks here, but from past experience, I think it's always better to have a safety guard just in case. If bidders are worried about this conversion rate stuff, at least they can desactivate it (especially when it occurs under the hood).

ahh, ok... I see where you're coming from.

I don't think we need to worry about it, though. Prebid.js has currency conversions, and Bidders can't opt-out there. Nobody has complained about it yet... so I wouldn't expect them to care here either.

Regarding the adapter, we are working on it. I don't have any ETA for the time being though. It doesn't prevent us to try to help on the PBS backlog, so happy to help :)

Thanks! ....and looking forward to it :)

benjaminch commented 6 years ago

Hello guys,

Just pushed a draft for the currency rate fetcher. This is just about introducing the mechanism to fetch rates periodically from remote source but no changes in PBS workflow just yet. I will do it in another CL for clarity.

If you can have a look to this CL and tell me what do you think about it and if you have any remarks.

Thanks ! :)

benjaminch commented 5 years ago

Hello guys,

I just added a new PR to start using the rate converter mechanism into PBS workflow. I think it needs your experts check in order to be polished.

Once everything look ok on your side, I think I will push another PR to expose rate converter config on a dedicated admin handler (fetching time, fetching URL along with actual rates values in the converter) to ease debug and so.

Thanks !

benjaminch commented 5 years ago

Hello guys,

Just added a PR to introduce a admin debug endpoint to monitor rate converter mechanics as discussed with @dbemiller earlier.

To recap, here's the plan to come to finish the dev for this feature:

Cheers !

benjaminch commented 5 years ago

Hello there !

I guess we can close this one since the last piece was merged couple weeks back. What do you guys think?

Cheers

stale[bot] commented 5 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

hhhjort commented 5 years ago

Yes I guess we can close this. :)

benjaminch commented 5 years ago

Yup ! Thanks @hhhjort ! Would be nice to know if anyone is using this feature though :)

stale[bot] commented 5 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

bjorn-lw commented 4 years ago

What is the status here? Everything seems to be merged but docs (http://prebid.org/prebid-server/developers/currency-converter.html) says the feature is disabled.

When I send in any currency other than USD in bidRequest.cur, it seems (for instance) as if Adform and Pubmatic returns bids with the correct currency, but Rubicon still always returns USD.

Since the currency is not returned in the bid, this feature (unless I do something wrong) is unusable since you can't tell if the bidder respected the currency or not.

Thanks for any insights on this.

benjaminch commented 4 years ago

Hey @bjorn-lw !

Feature is supposed to be working properly. Can you please provide couple bid requests examples you sent against Rubicon's endpoint along responses you got?

Thanks !

bjorn-lw commented 4 years ago

Hi!

See attached. Would be awesome if you spotted an issue on my end :)

Thanks! /Björn

(Attachments removed..ask if you need them..)

bjorn-lw commented 4 years ago

@benjaminch If you need more info from me, please let me know. I tested on the very latest release of Prebid Server. No config changes from default, except enabling Rubicon, changing to the Rubicon EU-endpoint + adding Rubicon credentials.

bjorn-lw commented 4 years ago

@benjaminch I'm not very fluent in GO but it seems like Rubicon resets the bid request currency so the currency is always set to the default currency (USD) before attempting currency conversion.

benjaminch commented 4 years ago

Indeed ! Question is now in such cases, PBS should prevent bidders from doing such, there is a potential mismatch between request and response currency. I’ll continue to dig into it tomorrow and let you know ;)

bjorn-lw commented 4 years ago

@benjaminch Thanks, very much appreciated. To bad there was a bug, good thing we found it.

It would be really nice to be able to use this feature, but as it stands now I can't since I don't know (without examining the code for each adapter and/or testing) which ones may exhibit the same behavior as Rubicon and thus making it a very unreliable feature.

This is not the place for feature requests, but as I'm here already writing. For our use, we don't really need the currency conversion, we just need to know the currency being sent back, it doesn't have to be the currency we requested as we can do currency conversion ourselves.

The issue we have at hand is that one of our customers is placing campaigns in their country's value and is confused why the bid coming back into the header bidding auction is not exactly the bid they placed, but off with a number of decimals. They actually thought we added a small percentage on top since the bid was slightly higher than they had expected.

If we could ask for EUR and get EUR back for the bidders that support it and USD for everyone else we would be good as long as we know who responds with what and that information is not communicated now. Regardless if PBS does currency conversion, I think it would be nice to fill in the "cur" field in the bid response.

benjaminch commented 4 years ago

Hey @bjorn-lw,

As far as I understand, it's up to the bidders to map or not currency (they might not support multi currency). PBS on its side, will forward to bidders currencies received in bid request and then check whether the bid response matches one of the request currency. If not, the response is rejected. Also, when no currency is set, it is considered as USD as described in OpenRTB.

For our use, we don't really need the currency conversion, we just need to know the currency being sent back, it doesn't have to be the currency we requested as we can do currency conversion ourselves. If no currency specified in bid response, then consider it as USD.

We can try to review adapters to map currencies, but in any case, if they don't, it means they will bid with USD (the issue being if they don't take into account the request currency).

But let's say, you have a request allowing cur:["EUR", "JPY"], even if the bidder doesn't take into account the currency, it will bid with, let's say USD, then it's bid will be converted by PBS to EUR. https://github.com/prebid/prebid-server/blob/master/exchange/bidder.go#L154 and https://github.com/prebid/prebid-server/blob/master/exchange/bidder.go#L182

Also, all use cases for multi-currencies are described in 3 tests here: https://github.com/prebid/prebid-server/blob/master/exchange/bidder_test.go#L245

Let me know how can I help you further

bjorn-lw commented 4 years ago

@benjaminch Hi!

All you say is fine and I do get it. Also, even if personally don't need the currency conversion, it's OK to have it there.

But

Having the currency specified in the response with an option to opt out currency conversion would be awesome (I guess setting the update interval to 0 would accomplish opt-out). As you said, if the adapter doesn't say anything else it defaults to USD. All well and understood.

Yes I would need more help :)

As it stands now I can't use the feature at all since Rubicon always responds with USD and who knows what more adapters including upcoming ones and there is no way to tell from the response I get back. Of course you would notice that there could be a 10x factor off or similar, but the diff between USD and EUR is not so big. I could of course make workarounds for Rubicon since I KNOW they will always respond with USD and the currency conversion is disabled for them due to them fiddling with the incoming request, but I can't tell without making assumptions, work-arounds and to some extent "hard-coding" stuff. And what happens when they fix their issue and I continue to assume they bid USD? Not a good situation. I really don't like that. And for every new adapter we add into our setup we need to test if they comply or not.

So preferably the problem should be fixed. While I would be glad to dive into the code and fix it myself, I simply don't have the time and sufficient GO-knowledge so I'm hoping for someone else to jump into this :) . In the mean time, we continue doing currency conversion on our end and also unfortunately where it looses precision.

Thanks :)

benjaminch commented 4 years ago

Hey @bjorn-lw

I do understand your issue, but not sure why it's happening, as I said, I would assume that even if Rubicon bids in USD, PBS will then do the conversion if the table is properly set (unless the currency does not exists there). I cannot really reproduce you setup (but I can if you push it on a branch somewhere :)).

Hopefully, there is an handler on the admin port to check what's in the conversion table: localhost:6060/currency/rates.

Can you check your currency is there and has an entry to USD?

bjorn-lw commented 4 years ago

@benjaminch In the particular case it's EUR (though my example was SEK, also in the list).

I think the offending line is here, but I can be mistaken.

https://github.com/prebid/prebid-server/blob/d3869bb5cccd8fb2906b50b75f02b17b46588b3a/adapters/rubicon/rubicon.go#L727

benjaminch commented 4 years ago

Yes I agree, but even if the adapter doesn't take currency into account, PBS should be able to behave properly.

Can you please link what you have hitting localhost:6060/currency/rates?

Looking at the code, it doesn't support two ways conversion, e.q. if we have an entry USD -> SEK = 9.270, but no entry SEK -> USD, then PBS won't be able to convert from SEK to USD. I will fix that. Just curious if that could fix your issue.

bjorn-lw commented 4 years ago

@benjaminch Having some issues with my machine right now, will get back to you with currency rates result,

But I saw when debugging before that all currencies were present so I think you can trust me on that, even though I will try to confirm it..

What I notice happening after calling Rubicon's adapter (and not everyone else I tested) is the following:

https://github.com/prebid/prebid-server/blob/d3869bb5cccd8fb2906b50b75f02b17b46588b3a/exchange/bidder.go#L143

request.cur is empty and the default currency (USD) is set in the line below. Nice! All is well....or is it? No it's not because the currency i asked for was EUR (or SEK or ...).

The currency converter sees that the request had USD incoming and that the response was set to default USD so no conversion is made:

https://github.com/prebid/prebid-server/blob/d3869bb5cccd8fb2906b50b75f02b17b46588b3a/exchange/bidder.go#L153

Happily returns the bid which happens to be USD and not EUR as was expected.

If the currency was reported in the response (USD) I wouldn't have any issues with this but now this is a black box from the outside and I have no idea whatsoever what currency I actually get back.

Again, GO is not my first language. I could be missing something crucial and my reasoning above makes no sense whatsoever, sorry if so :)

/Björn

bjorn-lw commented 4 years ago

if we have an entry USD -> SEK = 9.270, but no entry SEK -> USD, then PBS won't be able to convert from SEK to USD. I will fix that. Just curious if that could fix your issue.

Not in this particular scenario, but seems to be well worth fixing also.

benjaminch commented 4 years ago

Would you be able to push your current PBS version on a branch anywhere so I can test it on my side? Thanks

bjorn-lw commented 4 years ago

@benjaminch

Would you be able to push your current PBS version on a branch anywhere so I can test it on my side? Thanks

As I wrote earlier I test against the latest version of PBS (0.90.0). The only changes I have made is in the config: Enabling the Rubicon Adapter, changing the endpoint to one in EU plus adding my Rubicon Credentials.

Obviously you need to do the same, but I guess you know that (the Rubicon adapter is disabled by default since you can't use it without having API credentials).

Did you see my step-by-step description of what I belive is the problem?

Thanks again, Björn

bjorn-lw commented 4 years ago

@benjaminch See attached currency list. EUR and SEK is present under USD.

Do you agree with my analysis above or do I misinterpret the code?

Thanks for all your help so far, I very much appreciate it! /Björn rates.json.txt

benjaminch commented 4 years ago

I don't have any Rubicon credentials unfortunately ... To me, even if Rubicon doesn't flag the currency in bid request, worth case scenario their bid will be rejected but it shouldn't lead to bad currency being used.

Let's try to illustrate:

cur: ["SEK"] => PBS => Rubicon cur: => PBS cur: ["USD"] (PBS will try to convert USD to SEK, if not able, then reject Rubicon bid).

Is that what you observe? Would it be possible to get the original bid request to PBS along with request sent to Rubicon and Response received from Rubicon so I can try to test?

bjorn-lw commented 4 years ago

@benjaminch

I don't have any Rubicon credentials unfortunately ... To me, even if Rubicon doesn't flag the currency in bid request, worth case scenario their bid will be rejected but it shouldn't lead to bad currency being used.

OK, that's very unfortunate. If you could get in touch with me outside of here I could perhaps help you with this. But I pointed out the exact lines of codes. I don't know what else I can do. If the request is passed by reference into each adapter, they can modify the request at will and thus resetting the incoming CUR just as Rubicon does.

Not sure how I can explain it clearer, no offense.

Let's try to illustrate:

cur: ["SEK"] => PBS => Rubicon cur: => PBS cur: ["USD"] (PBS will try to convert USD to SEK, if not able, then reject Rubicon bid).

No. This is not at all what is happening. Here's what I observe:

  1. Prebid Server gets a request with cur set to ['EUR']
  2. Prebid server passes request by reference (from what I understand) into Rubicon
  3. Rubicon sets request.cur = Nil (https://github.com/prebid/prebid-server/blob/d3869bb5cccd8fb2906b50b75f02b17b46588b3a/adapters/rubicon/rubicon.go#L727)
  4. Response from Rubicon gets handled by Prebid Server which will do currency conversion. By now request.cur is empty due to what happened in 3). https://github.com/prebid/prebid-server/blob/d3869bb5cccd8fb2906b50b75f02b17b46588b3a/exchange/bidder.go#L143
  5. Now that request.cur is suddenly reset, PBS know nothing about the requested currency and assumes it is USD.
  6. When it's time for converting the bid into requested currency value, the response is assumed to be USD (as Rubicon doesn't tell which currency it's bidding in). Also, for all that PBS knows, the requested currency was USD so no conversion is being made: https://github.com/prebid/prebid-server/blob/d3869bb5cccd8fb2906b50b75f02b17b46588b3a/exchange/bidder.go#L153

Not sure I can make it clearer than that. It only builds upon observation. I actually don't know if the request is passed by reference, but as Rubicon seems to be able to reset the CUR field it seems to be. Anyways, at 4) the request.cur is absolutely without doubt empty and it wasn't on entry. Also it's correctly set after calling other adapters such as Adform who doesn't touch this field.

/Björn

benjaminch commented 4 years ago

Oh ! Ok I see, I missed the passed by ref thing and was thinking it changed the request for Rubicon only. Indeed, request is passed by reference, I'll check if it can have any side effect, I'll post my findings here.

bjorn-lw commented 4 years ago

@benjaminch Yes :) I was beginning to have serious doubts in my own understanding of this :) Perhaps you also see that this problem would have been much easier to detect if the bidder's currency was sent back in the response (according to the CUR field in the OpenRTB standard).

Thanks once again :)

benjaminch commented 4 years ago

Hey @hhhjort @SyntaxNode, what do you think about this behavior? MakeRequest do pass bid request as reference allowing side effects if any adapter happen to change one request value. Shouldn't all data passed to adapters be value instead of references?

hhhjort commented 4 years ago

The req and req.Imp objects are unique to adapters (as part of the adapter cleaning, removing references to other adapters), they are just shallow copies of the original request. So any references within them will point to the original objects. So Rubicon changing req.Cur should only affect them. There is an original request object floating around somewhere, so we should hopefully be able to grab that for the currency conversions rather than depending on the adapter copy. That should solve this issue. There may still be an issue where adapters don't report their bid currency at all, in which case we will have to assume USD, but that should be fairly safe as that is the default that seems to work.

bjorn-lw commented 4 years ago

@hhhjort @benjaminch If this can be fixed would be nice since this feature as of now can't be used due to this issue.

mlapeyre3 commented 4 years ago

Hello guys,

As of today, do you confirm no currency module is implemented for Prebid Server and all bids are considered as USD?

Thanks