Request Provided Currency Rates

[guscarreon]

This pull request implements feature described in: #1140

Until now, Prebid Server uses currency conversion rates fetched periodically from a service configured in package currency. This pull request allows a given bid request to use its own bid-request-level defined currency conversion rates. These bid-level currency rates come inside a newly created bidRequest.ext.prebid.currency field that looks like this:

"ext": {
  "prebid": {
    "currency": {
      "rates": {
        "USD": { "UAH": 24.47, "ETB": 32.04 },
        "GBP": { "USD": 1.21, "EUR": 1.1 }
      },
      "usepbsrates": false
    }
  }
}

request.ext.prebid.currency contains two fields: rates that is a map of currency conversion rates, and usePbsRates that determines whether or not the custom rates defined in rates complement those of Prebid Server's currency rate service (default behavior) or substitutes them entirely.

[guscarreon]

This pull request also implements the option to define a mock bid within the JSON test when needed

[guscarreon]

mock Prebid Server currency rate service values added to test configuration

[guscarreon]

Newly implemented mock bidder makes a bid of $1.00 dollar

[guscarreon]

This test case describes the functionality added to this PR in a nutshell.

[mansinahar]

Typo: were discarded

[mansinahar]

Looks like the second description here is possibly a copy-paste leftover?

[guscarreon]

It is. Sorry. Removed

[mansinahar]

I think you meant the description to end with field is not found

[guscarreon]

yep, reworded.

[mansinahar]

Any reason why we are doing the default conversions first and then discarding them if custom conversion rates are present? I think it would make more sense and be more efficient to apply the custom conversion rates first if they exist and fallback to default only if custom conversion rates don't exist or don't apply

[guscarreon]

Good point. Also refactored the code a bit and provided a couple more test cases.

[SyntaxNode]

Should we throw errors on invalid currency codes instead of silently removing them? .. or perhaps throw a warning?

[guscarreon]

This PR is throwing a fatal error only if all of the custom currency codes were found to be wrong. The reason this approach made more sense to me is that we ignore malformed or inaccurate 3-digit currency codes inside requestBid() anyways.

197                 var conversionRate float64
198                 var err error
199                 for _, bidReqCur := range request.Cur {
200                     if conversionRate, err = conversions.GetRate(bidResponse.Currency, bidReqCur); err == nil {
201                         seatBid.currency = bidReqCur
202                         break
203                     }
204                 }
exchange/bidder.go

But, I'm open to ideas here.

[SyntaxNode]

Let's seek out @hhhjort @mansinahar @bsardo for additional thoughts here.

[mansinahar]

I think I agree with @SyntaxNode here that we should at least append an error/warning about the bad currency codes so that the user knows what exactly the problem was because right now we just remove them which might eventually lead to a currency not found error and that doesn't indicate the exact issue.

Consider for example, someone wanted to provide and override for EUR to USD and by mistake they spelled USD as UST in their request, we'd remove that entry from the currency rates and then if we do get a bid in USD we won't be able to find the currency rate and return a currency not found error. This is kinda misleading, so I'd suggest we either return an error right here in the validating request step and mark the request invalid or we at least add a warning message in the response that points out the invalid currency code

[guscarreon]

Modified this function to throw fatal bad input error at the first invalid or malformed currency code found in the map

[SyntaxNode]

Might I suggest a different wording, to key on that this is a required field only because UsePBSRates is set to true in their request and that possibly not all currency names are invalid. Something along the lines of:

ext.prebid.currency.usePbsRates is set to false and all ext.prebid,currency.rates are invalid. invalid ISO-4217 code: %s 

[guscarreon]

But to reach this branch bidReqCurrencyRates.ConversionRates must be empty which means either it was empty from the beginning, which leads to returning the error in line 484, or it got emptied after finding all of the 3-letter currency codes to be wrong that resolves in returning error in 482. Do you agree with this approach? We could still reword the error message or even refactor this part if needed.

[SyntaxNode]

I don't think that's necessarily true. As the code is currently, a ConversionRates entry will be removed if the base currency is wrong even if the mapped currencies are correct. For example:

ConversionRates: map[string]map[string]float64{
    "FOO": {
        "GBP": 1.2,
        "MXN": 0.05,
        "JPY": 0.95,
    },
}

If I'm reading the code correctly, this will cause the error message to be reached when UsePBSRates is true. While the base currency of FOO is invalid, the mapped currencies of GBP, MXN, and JPY are valid.

[guscarreon]

This part of the code has been removed because we are now immediately returning a bad input error when the first non-valid code is found.

[SyntaxNode]

Why is the time needed for constant rates from the request?

[guscarreon]

Although these custom rates are constant values that will not change, in terms of the logic inside our currency package, they are not considered ConstantRates or at least they are not named that. This return value is meant to return the currency.Conversions implementation found in currency/rates.go. While "constant" rates are implemented in currency/constant_rates.go. They widely differ and in my opinion ConstantRates should be renamed to something else. Probably SameCurrencyRates or UnitaryRates or something. Kept that part out of the scope of this PR but I could always modify it if you disagree.

Because line 924 returns all-custom currency conversions object that doesn't go into a RateConverter dating them with time.Now() as the time they were fetched seemed more accurate but you raise a good point. Given that we don't actually do anything with that time stamp from here on I'll change to a less expensive time.Time{} instead.

[SyntaxNode]

Ideally, I think we should remove the reference here to time entirely. It really has no meaning.

It seems this field only exists to fully parse the auto generated PBS currency file. We don't actually do anything with that timestamp though. Perhaps we can remove it entirely.

Thoughts? @guscarreon @bsardo

[guscarreon]

I didn't remove it in today's PR update but I can do so if you want to. @bsardo @SyntaxNode please let me know.

[mansinahar]

Once we've recognized the current fromCurrency as an invalid currency and removed from the list, is there any reason to still go through the toCurrencies for it? I believe we should just continue to the next entry in the map after deleting this entry.

[guscarreon]

Good catch. Added a continue to skip to next entry

[mansinahar]

I think I agree with @SyntaxNode here that we should at least append an error/warning about the bad currency codes so that the user knows what exactly the problem was because right now we just remove them which might eventually lead to a currency not found error and that doesn't indicate the exact issue.

Consider for example, someone wanted to provide and override for EUR to USD and by mistake they spelled USD as UST in their request, we'd remove that entry from the currency rates and then if we do get a bid in USD we won't be able to find the currency rate and return a currency not found error. This is kinda misleading, so I'd suggest we either return an error right here in the validating request step and mark the request invalid or we at least add a warning message in the response that points out the invalid currency code

[mansinahar]

In case when only custom rates are in play, I believe we don't need to store the inverse rate conversions as well. The GetRate function in rates.go should take care of it when it does the currency conversion between two currencies

[guscarreon]

True. Good catch, corrected.

[mansinahar]

Wouldn't it be easier here if we just looped through all the custom rates and override the values in PBS rates (or a copy of PBS rates) that need to be overridden and then return the updated rate conversions? Something like:

// Go through all the custom rates
for fromCur, rates := range customRates {
	for toCur, val := range rates {
		// If the rate conversion exists in PBS rates then override the value
		if _, ok := pbsRates[fromCur]; ok {
			pbsRates[fromCur][toCur] = val
		} else if _, ok := pbsRates[toCur]; ok {
			pbsRates[toCur][fromCur] = val
		} else {
			// If it doesn't exist then, add the conversion
			pbsRates[fromCur][toCur] = val
		}
	}
}

(This code snippet is just to give you an idea of what I was thinking. May or may not compile as is)

[guscarreon]

I considered that option at first but discarded in favor of the new code found inside the newly created currency/rate_engines.go. Let me know your take on this.

[SyntaxNode]

Nitpick: Comment should start with NewAggregateConversions.

[guscarreon]

Done

[SyntaxNode]

Nitpick: Please remove the empty line.

[SyntaxNode]

The prefix check of the error is not ideal. If we need to make a choice based on a specific kind of error, please use a custom error and change this is to a type check.

[guscarreon]

done

[SyntaxNode]

Nitpick: Please follow the Go naming convention of starting with the function name. Also, there's still a reference to RateEngines .

Consider:
// GetRates is not implemented for AggregateConversions . There is no need to call GetRates for this scenario.

[SyntaxNode]

This is hard to read. Please add line breaks.

[guscarreon]

JSON doesn't allow breaking lines for readability so I shortened the description instead

[SyntaxNode]

Nitpick: Please update rateEngines variable name.

[SyntaxNode]

Nitpick: Please update RateEngines name usage in comment to AggregateConversions.

[SyntaxNode]

Where does the price of 2 come from? Is it hardcoded in the test framework?

[guscarreon]

The expected price of $2.00 is the converted amount of $20.00 MXN to US dollars. The mockBidder placed a $20 mexican pesos bid and the custom conversion rate of $10.00 MXN per USD takes precedence over PBS' currency rate or $8.00 MXN per USD.

[mansinahar]

please consider changing the directory name from custom-currency to currency-conversion as these tests are generally testing out currency conversions i.e. we also have tests in this directory where we use just PBS rates and not custom currency, right?

[guscarreon]

All test files in this directory are about bid requests that come with a requet.ext.prebid.currency field that defines custom currency conversion rates. I believe the name custom-currency is better in here, don't you?

[mansinahar]

Ah! I missed that. In that case, can we please add some test cases where the custom currency rates are not defined i.e.ext.prebid.currency doesn't exits but PBS rates are defined and we're using those to do the conversions? I'd have assumed we already had these tests but from a quick look I didn't find them in the sample-requests JSON tests. And we can have all these currency related tests then under a currency-conversion directory

[guscarreon]

Hadn't seen this comment before Mansi. Done. Thanks for your careful review

[SyntaxNode]

This test is failing due to a subtly where the iteration order for a map is indeterminate. That doesn't affect production, but does cause this test to sometimes fail because you can't be certain if "FOO" or "BAR" will fail first. You can solve this with having just one invalid currency.

Please review other tests to ensure they don't have the same problem.

[guscarreon]

Yep, this was also a problem in the JSON tests. Let me know if you agree with the correction

[SyntaxNode]

I don't see this change either. The commit may have gotten lost?

[SyntaxNode]

Please choose a severity of either Fatal (the auction is stopped when the error is encountered) or Warning (the auction continues and a warning is written to the debug section of the response). A vanilla Go error is considered fatal, so that might make the most sense here - especially since you've defined the code as an error code as opposed to a warning code.

[guscarreon]

The error gets ignored in exchange/bidder.go so the severity doesn't really matter:

213                 var err error
214                 for _, bidReqCur := range request.Cur {
215                     if conversionRate, err = conversions.GetRate(bidResponse.Currency, bidReqCur); err == nil {
216                         seatBid.currency = bidReqCur
217                         break
218                     }
219                 }
exchange/bidder.go

What matters is the type so we can type-assert here:

 24 func (re *AggregateConversions) GetRate(from string, to string) (float64, error) {
 25     rate, err := re.customRates.GetRate(from, to)
 26     if err == nil {
 27         return rate, nil
 28     } else if _, isMissingRateErr := err.(*errortypes.NoConversionRate); !isMissingRateErr {
 29         // other error, return the error
 30         return 0, err
 31     }
 32
 33     // because the custom rates' GetRate() call returned an error other than "conversion
 34     // rate not found", there's nothing wrong with the 3 letter currency code so let's
 35     // try the PBS rates instead
 36     return re.serverRates.GetRate(from, to)
 37 }
currency/aggregate_conversions.go

So, because we don't interrupt execution, a SeverityWarning makes the most sense, right?