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

Introduced FetchRate field to select source of floors from request or dynamically fetched #3380

Merged
merged 4 commits into from
Jan 29, 2024
Merged

Introduced FetchRate field to select source of floors from request or dynamically fetched #3380

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7ac2216
Introduced UseFetchDataRate field to select source of floors from req…
pm-jaydeep-mohite Jan 2, 2024
161b0a8
Added UT
pm-jaydeep-mohite Jan 2, 2024
e0593e3
Addressed review comments for variable and function rename
pm-jaydeep-mohite Jan 9, 2024
a394f69
Updated UT name
pm-jaydeep-mohite Jan 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions floors/fetcher.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very less chance of constant getting changed as these are % based values which will have range 0 to 100 always.

}

for _, modelGroup := range priceFloors.Data.ModelGroups {
if len(modelGroup.Values) == 0 || len(modelGroup.Values) > config.MaxRules {
return errors.New("invalid number of floor rules, floor rules should be greater than zero and less than MaxRules specified in account config")
Expand Down
27 changes: 27 additions & 0 deletions floors/fetcher_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import (
"github.com/prebid/prebid-server/v2/metrics"
metricsConf "github.com/prebid/prebid-server/v2/metrics/config"
"github.com/prebid/prebid-server/v2/openrtb_ext"
"github.com/prebid/prebid-server/v2/util/ptrutil"
"github.com/prebid/prebid-server/v2/util/timeutil"
"github.com/stretchr/testify/assert"
)
Expand Down Expand Up @@ -396,6 +397,32 @@ func TestValidatePriceFloorRules(t *testing.T) {
},
wantErr: true,
},
{
name: "Invalid useFetchDataRate",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: Can you update the name here to be Invalid FetchRate

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

args: args{
configs: config.AccountFloorFetch{
Enabled: true,
URL: testURL,
Timeout: 5,
MaxFileSizeKB: 20,
MaxRules: 1,
MaxAge: 20,
Period: 10,
},
priceFloors: &openrtb_ext.PriceFloorRules{
Data: &openrtb_ext.PriceFloorData{
SkipRate: 10,
ModelGroups: []openrtb_ext.PriceFloorModelGroup{{
Values: map[string]float64{
"*|*|www.website.com": 15.01,
},
}},
FetchRate: ptrutil.ToPtr(-11),
},
},
},
wantErr: true,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
Expand Down
23 changes: 18 additions & 5 deletions floors/floors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ const (
enforceRateMin int = 0
enforceRateMax int = 100
floorPrecision float64 = 0.01
dataRateMin int = 0
dataRateMax int = 100
)

// EnrichWithPriceFloors checks for floors enabled in account and request and selects floors data from dynamic fetched if present
Expand Down Expand Up @@ -136,24 +138,35 @@ func isPriceFloorsEnabledForRequest(bidRequestWrapper *openrtb_ext.RequestWrappe
return true
}

// useFetchedData will check if to use fetched data or request data
func useFetchedData(rate *int) bool {
if rate == nil {
return true
}
randomNumber := rand.Intn(dataRateMax)
return randomNumber < *rate
}

// resolveFloors does selection of floors fields from request data and dynamic fetched data if dynamic fetch is enabled
func resolveFloors(account config.Account, bidRequestWrapper *openrtb_ext.RequestWrapper, conversions currency.Conversions, priceFloorFetcher FloorFetcher) (*openrtb_ext.PriceFloorRules, []error) {
var errList []error
var floorRules *openrtb_ext.PriceFloorRules
var (
errList []error
floorRules *openrtb_ext.PriceFloorRules
fetchResult *openrtb_ext.PriceFloorRules
fetchStatus string
)

reqFloor := extractFloorsFromRequest(bidRequestWrapper)
if reqFloor != nil && reqFloor.Location != nil && len(reqFloor.Location.URL) > 0 {
account.PriceFloors.Fetcher.URL = reqFloor.Location.URL
}
account.PriceFloors.Fetcher.AccountID = account.ID

var fetchResult *openrtb_ext.PriceFloorRules
var fetchStatus string
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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

mergedFloor := mergeFloors(reqFloor, fetchResult, conversions)
floorRules, errList = createFloorsFrom(mergedFloor, account, fetchStatus, openrtb_ext.FetchLocation)
} else if reqFloor != nil {
Expand Down
278 changes: 278 additions & 0 deletions floors/floors_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"github.com/prebid/prebid-server/v2/currency"
"github.com/prebid/prebid-server/v2/openrtb_ext"
"github.com/prebid/prebid-server/v2/util/jsonutil"
"github.com/prebid/prebid-server/v2/util/ptrutil"
"github.com/stretchr/testify/assert"
)

Expand Down Expand Up @@ -760,6 +761,283 @@ func TestResolveFloors(t *testing.T) {
}
}

type MockFetchDataRate0 struct{}

func (m *MockFetchDataRate0) Fetch(configs config.AccountPriceFloors) (*openrtb_ext.PriceFloorRules, string) {

if !configs.UseDynamicData {
return nil, openrtb_ext.FetchNone
}
priceFloors := openrtb_ext.PriceFloorRules{
Enabled: getTrue(),
PriceFloorLocation: openrtb_ext.RequestLocation,
Enforcement: &openrtb_ext.PriceFloorEnforcement{
EnforcePBS: getTrue(),
EnforceRate: 100,
FloorDeals: getTrue(),
},
Data: &openrtb_ext.PriceFloorData{
Currency: "USD",
ModelGroups: []openrtb_ext.PriceFloorModelGroup{
{
ModelVersion: "model from fetched",
Currency: "USD",
Values: map[string]float64{
"banner|300x600|www.website5.com": 15,
"*|*|*": 25,
},
Schema: openrtb_ext.PriceFloorSchema{
Fields: []string{"mediaType", "size", "domain"},
},
},
},
FetchRate: ptrutil.ToPtr(0),
},
}
return &priceFloors, openrtb_ext.FetchSuccess
}

func (m *MockFetchDataRate0) Stop() {

}

type MockFetchDataRate100 struct{}

func (m *MockFetchDataRate100) Fetch(configs config.AccountPriceFloors) (*openrtb_ext.PriceFloorRules, string) {

if !configs.UseDynamicData {
return nil, openrtb_ext.FetchNone
}
priceFloors := openrtb_ext.PriceFloorRules{
Enabled: getTrue(),
PriceFloorLocation: openrtb_ext.RequestLocation,
Enforcement: &openrtb_ext.PriceFloorEnforcement{
EnforcePBS: getTrue(),
EnforceRate: 100,
FloorDeals: getTrue(),
},
Data: &openrtb_ext.PriceFloorData{
Currency: "USD",
ModelGroups: []openrtb_ext.PriceFloorModelGroup{
{
ModelVersion: "model from fetched",
Currency: "USD",
Values: map[string]float64{
"banner|300x600|www.website5.com": 15,
"*|*|*": 25,
},
Schema: openrtb_ext.PriceFloorSchema{
Fields: []string{"mediaType", "size", "domain"},
},
},
},
FetchRate: ptrutil.ToPtr(100),
},
}
return &priceFloors, openrtb_ext.FetchSuccess
}

func (m *MockFetchDataRate100) Stop() {

}

type MockFetchDataRateNotProvided struct{}

func (m *MockFetchDataRateNotProvided) Fetch(configs config.AccountPriceFloors) (*openrtb_ext.PriceFloorRules, string) {

if !configs.UseDynamicData {
return nil, openrtb_ext.FetchNone
}
priceFloors := openrtb_ext.PriceFloorRules{
Enabled: getTrue(),
PriceFloorLocation: openrtb_ext.RequestLocation,
Enforcement: &openrtb_ext.PriceFloorEnforcement{
EnforcePBS: getTrue(),
EnforceRate: 100,
FloorDeals: getTrue(),
},
Data: &openrtb_ext.PriceFloorData{
Currency: "USD",
ModelGroups: []openrtb_ext.PriceFloorModelGroup{
{
ModelVersion: "model from fetched",
Currency: "USD",
Values: map[string]float64{
"banner|300x600|www.website5.com": 5,
"*|*|*": 15,
},
Schema: openrtb_ext.PriceFloorSchema{
Fields: []string{"mediaType", "size", "domain"},
},
},
},
},
}
return &priceFloors, openrtb_ext.FetchSuccess
}

func (m *MockFetchDataRateNotProvided) Stop() {

}

func TestResolveFloorsWithUseDataRate(t *testing.T) {
rates := map[string]map[string]float64{}

testCases := []struct {
name string
bidRequestWrapper *openrtb_ext.RequestWrapper
account config.Account
conversions currency.Conversions
expErr []error
expFloors *openrtb_ext.PriceFloorRules
fetcher FloorFetcher
}{
{
name: "Dynamic fetch enabled, floors from request selected as data rate 0",
fetcher: &MockFetchDataRate0{},
bidRequestWrapper: &openrtb_ext.RequestWrapper{
BidRequest: &openrtb2.BidRequest{
Site: &openrtb2.Site{
Publisher: &openrtb2.Publisher{Domain: "www.website.com"},
},
Imp: []openrtb2.Imp{{ID: "1234", Banner: &openrtb2.Banner{Format: []openrtb2.Format{{W: 300, H: 250}}}}},
Ext: json.RawMessage(`{"prebid":{"floors":{"data":{"currency":"USD","modelgroups":[{"modelversion":"model 1 from req","currency":"USD","values":{"banner|300x600|www.website5.com":5,"*|*|*":7},"schema":{"fields":["mediaType","size","domain"],"delimiter":"|"}}]},"enabled":true,"enforcement":{"enforcepbs":true,"floordeals":true,"enforcerate":100}}}}`),
},
},
account: config.Account{
PriceFloors: config.AccountPriceFloors{
Enabled: true,
UseDynamicData: true,
},
},
expFloors: &openrtb_ext.PriceFloorRules{
Enabled: getTrue(),
FetchStatus: openrtb_ext.FetchNone,
PriceFloorLocation: openrtb_ext.RequestLocation,
Enforcement: &openrtb_ext.PriceFloorEnforcement{
EnforcePBS: getTrue(),
FloorDeals: getTrue(),
EnforceRate: 100,
},
Data: &openrtb_ext.PriceFloorData{
Currency: "USD",
ModelGroups: []openrtb_ext.PriceFloorModelGroup{
{
ModelVersion: "model 1 from req",
Currency: "USD",
Values: map[string]float64{
"banner|300x600|www.website5.com": 5,
"*|*|*": 7,
},
Schema: openrtb_ext.PriceFloorSchema{
Fields: []string{"mediaType", "size", "domain"},
Delimiter: "|",
},
},
},
},
},
},
{
name: "Dynamic fetch enabled, floors from fetched selected as data rate is 100",
fetcher: &MockFetchDataRate100{},
bidRequestWrapper: &openrtb_ext.RequestWrapper{
BidRequest: &openrtb2.BidRequest{
Site: &openrtb2.Site{
Publisher: &openrtb2.Publisher{Domain: "www.website.com"},
},
Imp: []openrtb2.Imp{{ID: "1234", Banner: &openrtb2.Banner{Format: []openrtb2.Format{{W: 300, H: 250}}}}},
},
},
account: config.Account{
PriceFloors: config.AccountPriceFloors{
Enabled: true,
UseDynamicData: true,
},
},
expFloors: &openrtb_ext.PriceFloorRules{
Enabled: getTrue(),
FetchStatus: openrtb_ext.FetchSuccess,
PriceFloorLocation: openrtb_ext.FetchLocation,
Enforcement: &openrtb_ext.PriceFloorEnforcement{
EnforcePBS: getTrue(),
FloorDeals: getTrue(),
EnforceRate: 100,
},
Data: &openrtb_ext.PriceFloorData{
Currency: "USD",
ModelGroups: []openrtb_ext.PriceFloorModelGroup{
{
ModelVersion: "model from fetched",
Currency: "USD",
Values: map[string]float64{
"banner|300x600|www.website5.com": 15,
"*|*|*": 25,
},
Schema: openrtb_ext.PriceFloorSchema{
Fields: []string{"mediaType", "size", "domain"},
},
},
},
FetchRate: ptrutil.ToPtr(100),
},
},
},
{
name: "Dynamic fetch enabled, floors from fetched selected as data rate not provided as default value = 100",
fetcher: &MockFetchDataRateNotProvided{},
bidRequestWrapper: &openrtb_ext.RequestWrapper{
BidRequest: &openrtb2.BidRequest{
Site: &openrtb2.Site{
Publisher: &openrtb2.Publisher{Domain: "www.website.com"},
},
Imp: []openrtb2.Imp{{ID: "1234", Banner: &openrtb2.Banner{Format: []openrtb2.Format{{W: 300, H: 250}}}}},
Ext: json.RawMessage(`{"prebid":{"floors":{"data":{"currency":"USD","modelgroups":[{"modelversion":"model 1 from req","currency":"USD","values":{"banner|300x600|www.website5.com":5,"*|*|*":7},"schema":{"fields":["mediaType","size","domain"],"delimiter":"|"}}]},"enabled":true,"enforcement":{"enforcepbs":true,"floordeals":true,"enforcerate":100}}}}`),
},
},
account: config.Account{
PriceFloors: config.AccountPriceFloors{
Enabled: true,
UseDynamicData: true,
},
},
expFloors: &openrtb_ext.PriceFloorRules{
Enabled: getTrue(),
FetchStatus: openrtb_ext.FetchSuccess,
PriceFloorLocation: openrtb_ext.FetchLocation,
Enforcement: &openrtb_ext.PriceFloorEnforcement{
EnforcePBS: getTrue(),
FloorDeals: getTrue(),
EnforceRate: 100,
},
Data: &openrtb_ext.PriceFloorData{
Currency: "USD",
ModelGroups: []openrtb_ext.PriceFloorModelGroup{
{
ModelVersion: "model from fetched",
Currency: "USD",
Values: map[string]float64{
"banner|300x600|www.website5.com": 5,
"*|*|*": 15,
},
Schema: openrtb_ext.PriceFloorSchema{
Fields: []string{"mediaType", "size", "domain"},
},
},
},
},
},
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
resolvedFloors, _ := resolveFloors(tc.account, tc.bidRequestWrapper, getCurrencyRates(rates), tc.fetcher)
assert.Equal(t, resolvedFloors, tc.expFloors, tc.name)
})
}
}

func printFloors(floors *openrtb_ext.PriceFloorRules) string {
fbytes, _ := jsonutil.Marshal(floors)
return string(fbytes)
Expand Down
Loading
Loading