Introduce currency support in adapters bid responses as described in #280. #491

benjaminch · 2018-05-03T14:47:41Z

This CL introduces currency declaration support in adapters bid responses as described in #280.

This change covers:

Introducing a new struct BidderResponse carrying the list of bids TypedBid and a currency as string
Change the existing adapters using the new API MakeBids so it start using the new BidderResponse struct

This change doesn't cover:

Doesn't change the former API using Call method
Any check done on Pre-bid server side to discard any bid received with an invalid currency
(such as unknown, or a one that wasn't authorized in a first place via trhe bid request)
Any mechanisms on tranlating currency rates

As a nice next step, we should implement the mechanism to filter bids having invalid currencies on not allowed ones.
It will allow to protect both parties from currencies mismatch / discrepancies.

dbemiller

Looks good overall! A few little things did jump out at me... see below.

dbemiller · 2018-05-03T18:34:59Z

adapters/bidder.go

+// to "USD".
+//
+// By default, currency is USD but this behavior might be subject to change.
+func NewBidderResponse() *BidderResponse {


Should we allow an initial capacity here?

It seems like many of the adapters were using this and lost it. For example:

- bids := make([]*adapters.TypedBid, 0, len(adformBids)) +func toOpenRtbBidResponse(adformBids []*adformBid, r *openrtb.BidRequest) *adapters.BidderResponse { + bidResponse := adapters.NewBidderResponse()

Fair point !
I let a default constructor NewBidderResponse() and also added a NewBidderResponseWithBidsCapacity(bidsCapacity int) and use the later in the adapters leveraging bids capacity earlier.

dbemiller · 2018-05-03T18:50:18Z

adapters/adapterstest/test_json.go

 		bidsErrs = append(bidsErrs, theseErrs...)
+		if thisBidResponse != nil {
+			// The currency is the same accross all bids


hmm... I'm a little puzzled by this. I know everything is USD now, but how do you plan to handle this in the live code?

As a Bidder, I would expect to be able to return different currencies for different calls to MakeBids. Since HTTP calls are made in parallel, the behavior might not even be deterministic; HTTP Responses with different currencies might come back in different orders for different auctions.

I feel like this would be much less confusing as:

"expectedBidResponses": [ { "currency": "USD", "bids: [...] }, { "currency": "USD", "bids": [...] } ]

Fair point, I agree with you, I make sure that currency is populated everytime:

if thisBidResponse != nil { bidResponse.Currency = thisBidResponse.Currency bidResponse.Bids = append(bidResponse.Bids, thisBidResponse.Bids...) }

wait... I'm a little bit confused by what this accomplishes.

I might be missing something... but it looks like the currency from the last call to MakeBids takes effect now, rather than the first one.

This behavior still seems like it'd be confusing to Bidders, and still seems to have the same problem with determinism from the parallel calls in live code?

Oh sorry, I get what you meant, just updated the change, let me know what you think.

perfect :). Thanks!

dbemiller · 2018-05-03T19:01:20Z

exchange/bidder.go

-					bids[i].Bid.Price = bids[i].Bid.Price * bidAdjustment
+			if bidResponse != nil {
+				for i := 0; i < len(bidResponse.Bids); i++ {
+					if bidResponse.Bids[i].Bid != nil {


Could you stick a // TODO #280: Convert the bid price comment here? Just so we don't lose track of it.

A TODO #280 would also be useful in the `func (brw *bidResponseWrapper) validateBids().

Yes good point, just done

cirla

This is nice and clean plumbing and well limited in scope. The BidderResponse container will make it easier in the future for adapters to pass additional information back to the exchange.

…nses as described in prebid#280. This change covers: * Introducing a new struct `BidderResponse` carrying the list of bids `TypedBid` and a currency as string * Change the existing adapters using the new API `MakeBids` so it start using the new `BidderResponse` struct This change doesn't cover: * Doesn't change the former API using `Call` method * Any check done on Pre-bid server side to discard any bid received with an invalid currency (such as unknown, or a one that wasn't authorized in a first place via trhe bid request) * Any mechanisms on tranlating currency rates As a nice next step, we should implement the mechanism to filter bids having invalid currencies on not allowed ones. It will allow to protect both parties from currencies mismatch / discrepancies.

dbemiller

looks great!

benjaminch · 2018-05-04T16:38:53Z

Thanks guys :)

bretg · 2018-05-08T17:53:53Z

@dbemiller - would like to discuss the design here because PBS-Java is returning currency in a different way, so this is now a difference in how bid adapters would need to set currency in Go-vs-Java.

in PBS-Go, the adapter would set BidResponse.currency, which is outside of each TypedBid.
in PBS-Java, it would set TypedBid[N].currency

It isn't a requirement that each bid have a separate currency, but approach #2 has the nice property that the currency stays associated with each bid, so it's cached and analytics adapters may have easier access.

Is it true that in the design #1 the currency doesn't survive the call to Prebid Cache? Unclear that's a near-term requirement, but seems like something that could be important should anyone want to look at the CPM again.

benjaminch · 2018-05-08T18:05:53Z

Hey @bretg

I wasn't aware of this behavior difference between Go / Java.
To me, it looks cleaner to have the currency at root level without having to repeat it within each TypedBid since the currency should be the same accros for all bids of the same bid response.

Happy to try another solution if you guys want. But the BidderResponse is a nice add to me so it will allow to share more information shared between each bids down the road.

bretg · 2018-05-08T18:29:32Z

@benjaminch - it's good encapsulation for sure. My concern is about what's cached -- if that common info isn't stored in PBC, we won't have it at render time.

So of course an alternate solution is to change how such common info is cached.

dbemiller · 2018-05-08T18:30:18Z

@bretg That idea was actually pitched here:

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).

We settled on BidderResponse because it works better for Bidders who put all their responses in the same currency, which is (today) all of them.

I'm very strongly opposed to asking a community member rewrite code based on differences between Go/Java. Forking projets has a very high maintenance cost, which is why Prebid.org isn't officially supporting the Java version.

I'm a little confused by the more substantive objections, though. The Bidder API changes here don't affect the Analytics adapter APIs or what we cache. We can easily change both regardless of how Bidders return the info

To me, it seems like the root question is:

How often will Bidders need to respond with bids of different currencies in the same HTTP response?

If it's common, it should be inside TypedBid.
If it's impossible, it should be in BidderResponse.
If it's unusual, it should be in both, and the TypedBid value should override the BidderResponse value if it's defined.

The only way these PR changes are "bad" is if it's a common case. If we need to transition from impossible => unusual in the future, it's a backwards-compatible change.

Also worth noting: OpenRTB defines cur on the BidResponse... so it seemed quite safe to assume that Bidders rarely, if ever, return bids in different currencies in the same response.

bretg · 2018-05-08T19:04:43Z

I wasn't asking for a change, I was asking to discuss the design. Hence the wording: "would like to discuss the design here". Sorry for not catching the design discussion in the other thread.

How often will Bidders need to respond with bids of different currencies in the same HTTP response

Never - this wasn't the issue.

The Bidder API changes here don't affect the Analytics adapter APIs or what we cache. We can easily change both regardless of how Bidders return the info

Very well. I accept this. We'll change the Java version at some point.

…nses as described in prebid#280. (prebid#491) This change covers: * Introducing a new struct `BidderResponse` carrying the list of bids `TypedBid` and a currency as string * Change the existing adapters using the new API `MakeBids` so it start using the new `BidderResponse` struct This change doesn't cover: * Doesn't change the former API using `Call` method * Any check done on Pre-bid server side to discard any bid received with an invalid currency (such as unknown, or a one that wasn't authorized in a first place via trhe bid request) * Any mechanisms on tranlating currency rates As a nice next step, we should implement the mechanism to filter bids having invalid currencies on not allowed ones. It will allow to protect both parties from currencies mismatch / discrepancies.

prebid#491)

* OTT-1058: record number of deal responses for given pub-prof-partner (prebid#485) (prebid#487) Co-authored-by: Pubmatic-Dhruv-Sonone <[email protected]> * OTT-1060: Added IncDealBidCount in stats server for selected publishe… (prebid#491) --------- Co-authored-by: ashishshinde-pubm <[email protected]> Co-authored-by: Pubmatic-Dhruv-Sonone <[email protected]> Co-authored-by: ShriprasadM <[email protected]>

benjaminch force-pushed the exposing-currency branch from 2d0daec to 276ee4a Compare May 3, 2018 14:56

dbemiller requested a review from cirla May 3, 2018 18:30

dbemiller assigned cirla May 3, 2018

dbemiller self-requested a review May 3, 2018 18:30

dbemiller assigned cirla and dbemiller and unassigned cirla May 3, 2018

dbemiller suggested changes May 3, 2018

View reviewed changes

benjaminch force-pushed the exposing-currency branch 4 times, most recently from 071e2c6 to f7347dc Compare May 4, 2018 09:23

cirla previously approved these changes May 4, 2018

View reviewed changes

benjaminch dismissed cirla’s stale review via 0ad0d1b May 4, 2018 16:29

benjaminch force-pushed the exposing-currency branch from f7347dc to 0ad0d1b Compare May 4, 2018 16:29

dbemiller approved these changes May 4, 2018

View reviewed changes

cirla approved these changes May 4, 2018

View reviewed changes

dbemiller merged commit dce2245 into prebid:master May 4, 2018

benjaminch mentioned this pull request Mar 8, 2019

Open RTB: Add currency support #280

Closed

pm-nilesh-chate pushed a commit to pm-nilesh-chate/prebid-server that referenced this pull request Jun 21, 2023

OTT-1060: Added IncDealBidCount in stats server for selected publishe… (

3332bd3

prebid#491)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce currency support in adapters bid responses as described in #280. #491

Introduce currency support in adapters bid responses as described in #280. #491

benjaminch commented May 3, 2018

dbemiller left a comment

dbemiller May 3, 2018

benjaminch May 4, 2018

dbemiller May 3, 2018

benjaminch May 4, 2018 •

edited

Loading

dbemiller May 4, 2018 •

edited

Loading

benjaminch May 4, 2018

dbemiller May 4, 2018

dbemiller May 3, 2018

benjaminch May 4, 2018

cirla left a comment

dbemiller left a comment

benjaminch commented May 4, 2018

bretg commented May 8, 2018

benjaminch commented May 8, 2018

bretg commented May 8, 2018

dbemiller commented May 8, 2018 •

edited

Loading

bretg commented May 8, 2018

Introduce currency support in adapters bid responses as described in #280. #491

Introduce currency support in adapters bid responses as described in #280. #491

Conversation

benjaminch commented May 3, 2018

dbemiller left a comment

Choose a reason for hiding this comment

dbemiller May 3, 2018

Choose a reason for hiding this comment

benjaminch May 4, 2018

Choose a reason for hiding this comment

dbemiller May 3, 2018

Choose a reason for hiding this comment

benjaminch May 4, 2018 • edited Loading

Choose a reason for hiding this comment

dbemiller May 4, 2018 • edited Loading

Choose a reason for hiding this comment

benjaminch May 4, 2018

Choose a reason for hiding this comment

dbemiller May 4, 2018

Choose a reason for hiding this comment

dbemiller May 3, 2018

Choose a reason for hiding this comment

benjaminch May 4, 2018

Choose a reason for hiding this comment

cirla left a comment

Choose a reason for hiding this comment

dbemiller left a comment

Choose a reason for hiding this comment

benjaminch commented May 4, 2018

bretg commented May 8, 2018

benjaminch commented May 8, 2018

bretg commented May 8, 2018

dbemiller commented May 8, 2018 • edited Loading

bretg commented May 8, 2018

benjaminch May 4, 2018 •

edited

Loading

dbemiller May 4, 2018 •

edited

Loading

dbemiller commented May 8, 2018 •

edited

Loading