Introduced FetchRate field to select source of floors from request or dynamically fetched #3380
Conversation
…uest or dynamic fetched
pm-jaydeep-mohite
changed the title
openrtb_ext/floors.go
Outdated
| @@ -88,6 +88,7 @@ type PriceFloorData struct { | |||
| ModelTimestamp int `json:"modeltimestamp,omitempty"` | |||
| ModelGroups []PriceFloorModelGroup `json:"modelgroups,omitempty"` | |||
| FloorProvider string `json:"floorprovider,omitempty"` | |||
| UseFetchDataRate *int `json:"usefetchdatarate,omitempty"` | |||
There was a problem hiding this comment.
I'm wondering if this can be named FetchDataRate or FetchRate instead? The current naming gives me the feeling like it's a boolean, but since it's an int, we could update the name to reflect that.
There was a problem hiding this comment.
Renamed to FetchRate
floors/floors.go
Outdated
| @@ -136,24 +138,35 @@ func isPriceFloorsEnabledForRequest(bidRequestWrapper *openrtb_ext.RequestWrappe | |||
| return true | |||
| } | |||
|
|
|||
| // shouldUseFetchedData will check if to use fetched data or request data | |||
| func shouldUseFetchedData(rate *int) bool { | |||
There was a problem hiding this comment.
Can the function name be just useFetchedData? I think it accomplishes the same thing.
floors/fetcher_test.go
Outdated
| @@ -396,6 +397,32 @@ func TestValidatePriceFloorRules(t *testing.T) { | |||
| }, | |||
| wantErr: true, | |||
| }, | |||
| { | |||
| name: "Invalid useFetchDataRate", | |||
There was a problem hiding this comment.
Nitpick: Can you update the name here to be Invalid FetchRate
pm-jaydeep-mohite
changed the title
| @@ -309,6 +309,10 @@ func validateRules(config config.AccountFloorFetch, priceFloors *openrtb_ext.Pri | |||
| return errors.New("skip rate should be greater than or equal to 0 and less than 100") | |||
| } | |||
|
|
|||
| if priceFloors.Data.FetchRate != nil && (*priceFloors.Data.FetchRate < dataRateMin || *priceFloors.Data.FetchRate > dataRateMax) { | |||
| return errors.New("FetchRate should be greater than or equal to 0 and less than or equal to 100") | |||
There was a problem hiding this comment.
Nitpick: In case the value of the dataRateMin and dataRateMax constants changes someday, should we use their values in the error message instead?
312 if priceFloors.Data.FetchRate != nil && (*priceFloors.Data.FetchRate < dataRateMin || *priceFloors.Data.FetchRate > dataRateMax) {
313 - return errors.New("FetchRate should be greater than or equal to 0 and less than or equal to 100")
+ return fmt.Errorf("FetchRate should be greater than or equal to %d and less than or equal to %d", dataRateMin, dataRateMax)
314 }
315
floors/fetcher.go
There was a problem hiding this comment.
Very less chance of constant getting changed as these are % based values which will have range 0 to 100 always.
| if priceFloorFetcher != nil && account.PriceFloors.UseDynamicData { | ||
| fetchResult, fetchStatus = priceFloorFetcher.Fetch(account.PriceFloors) | ||
| } | ||
|
|
||
| if fetchResult != nil && fetchStatus == openrtb_ext.FetchSuccess { | ||
| if fetchResult != nil && fetchStatus == openrtb_ext.FetchSuccess && useFetchedData(fetchResult.Data.FetchRate) { |
There was a problem hiding this comment.
Nitpick: Although the Fetch(config config.AccountPriceFloors) implementation called in line 166 returns openrtb_ext.FetchNone when fetchResult.Data is nil, making it impossible for a nil fetchResult.Data in the third conditional in line 169, can we still add a check before dereferencing?
166 fetchResult, fetchStatus = priceFloorFetcher.Fetch(account.PriceFloors)
167 }
168
169 - if fetchResult != nil && fetchStatus == openrtb_ext.FetchSuccess && useFetchedData(fetchResult.Data.FetchRate) {
+ if fetchResult != nil && fetchStatus == openrtb_ext.FetchSuccess && fetchResult.Data != nil && useFetchedData(fetchResult.Data.FetchRate) {
170 mergedFloor := mergeFloors(reqFloor, fetchResult, conversions)
171 floorRules, errList = createFloorsFrom(mergedFloor, account, fetchStatus, openrtb_ext.FetchLocation)
172 } else if reqFloor != nil {
floors/floors.go
I guess to prevent panics in case the code changes in the future. Thoughts?
There was a problem hiding this comment.
In case of fetchStatus == openrtb_ext.FetchSuccess, fetchResult.Data can not be nil [https://github.com/prebid/prebid-server/blob/498955bcd5c242866a9b05886ac4e703cc850b2f/floors/fetcher.go#L140], so no need to add redundant check.
Changes proposed in #2755 are implemented in this PR