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

Media type price granularity #2721

Merged
merged 18 commits into from
Jun 5, 2023
Merged

Media type price granularity #2721

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
a96265f
Initial working implementation
VeronikaSolovei9 Apr 25, 2023
61783cf
Code clean up
VeronikaSolovei9 Apr 26, 2023
082ce54
Fix for existing unit tests
VeronikaSolovei9 Apr 26, 2023
0340ae9
Added unit tests
VeronikaSolovei9 Apr 26, 2023
59459f7
Added json tests
VeronikaSolovei9 Apr 26, 2023
833547a
Code review clean up
VeronikaSolovei9 May 2, 2023
f19e70d
Added support for native bid type
VeronikaSolovei9 May 3, 2023
c28cf36
Code clean up
VeronikaSolovei9 May 11, 2023
69dca07
Error handling clean up
VeronikaSolovei9 May 15, 2023
cd013d8
Clean up type description
VeronikaSolovei9 May 17, 2023
f949e12
Clean up unit tests
VeronikaSolovei9 May 18, 2023
c0a3217
Removed redundant integration tests
VeronikaSolovei9 May 22, 2023
74af7d1
Refactored Unmarshal error handling
VeronikaSolovei9 May 23, 2023
6183f53
Changed pointer to no pointer
VeronikaSolovei9 May 24, 2023
ad16925
Merge branch 'master' into mediaTypePriceGranularity
VeronikaSolovei9 May 24, 2023
4ca20a4
Minor code clean up
VeronikaSolovei9 May 30, 2023
23feb90
Merge branch 'master' into mediaTypePriceGranularity
VeronikaSolovei9 May 31, 2023
1cb8277
Code and unit tests cleanup
VeronikaSolovei9 Jun 1, 2023
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
54 changes: 38 additions & 16 deletions endpoints/openrtb2/auction.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1563,29 +1563,51 @@ func validateTargeting(t *openrtb_ext.ExtRequestTargeting) error {
}

if t.PriceGranularity != nil {
pg := t.PriceGranularity

if pg.Precision == nil {
return errors.New("Price granularity error: precision is required")
} else if *pg.Precision < 0 {
return errors.New("Price granularity error: precision must be non-negative")
} else if *pg.Precision > openrtb_ext.MaxDecimalFigures {
return fmt.Errorf("Price granularity error: precision of more than %d significant figures is not supported", openrtb_ext.MaxDecimalFigures)
if err := validatePriceGranularity(t.PriceGranularity); err != nil {
return err
}
}

var prevMax float64 = 0
for _, gr := range pg.Ranges {
if gr.Max <= prevMax {
return errors.New(`Price granularity error: range list must be ordered with increasing "max"`)
if t.MediaTypePriceGranularity != nil {
if t.MediaTypePriceGranularity.Video != nil {
if err := validatePriceGranularity(t.MediaTypePriceGranularity.Video); err != nil {
return err
}

if gr.Increment <= 0.0 {
return errors.New("Price granularity error: increment must be a nonzero positive number")
}
if t.MediaTypePriceGranularity.Banner != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

For my own understanding, why do we not need to have checks for other media types like Native and Audio?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is a question I wanted to ask too.
Here is a link to documentation: https://docs.prebid.org/prebid-server/endpoints/openrtb2/pbs-endpoint-auction.html#targeting
In the table of attributes I only found mediatypepricegranularity.banner and mediatypepricegranularity.video. I'll confirm this with the team.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bretg Bret, can you please confirm we only want to support banner and video for media type price granularity.

Copy link
Contributor

Choose a reason for hiding this comment

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

native should be supported as defined in the original issue #857. Doc updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

if err := validatePriceGranularity(t.MediaTypePriceGranularity.Banner); err != nil {
return err
}
}
if t.MediaTypePriceGranularity.Native != nil {
if err := validatePriceGranularity(t.MediaTypePriceGranularity.Native); err != nil {
return err
}
prevMax = gr.Max
}
}
return nil
}

func validatePriceGranularity(pg *openrtb_ext.PriceGranularity) error {
if pg.Precision == nil {
return errors.New("Price granularity error: precision is required")
Copy link
Contributor

Choose a reason for hiding this comment

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

Will we ever get to a situation where this will be nil? Validate is called after SetDefaults and as part of set defaults we ensure that precision is set to the default value if not specified. See here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Discussed offline. No changes needed.

} else if *pg.Precision < 0 {
return errors.New("Price granularity error: precision must be non-negative")
} else if *pg.Precision > openrtb_ext.MaxDecimalFigures {
return fmt.Errorf("Price granularity error: precision of more than %d significant figures is not supported", openrtb_ext.MaxDecimalFigures)
}

var prevMax float64 = 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a Go preference for how this should be initialized?

e.g: prevMax := 0.0 vs. var prevMax float64 = 0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This code was moved without modifications from func validateTargeting(t *openrtb_ext.ExtRequestTargeting) error to it's own func validatePriceGranularity(pg *openrtb_ext.PriceGranularity) error.
For this case I think type was declared explicitly like this to show this is a price comparison.
We can discuss it with the team.

for _, gr := range pg.Ranges {
if gr.Max <= prevMax {
return errors.New(`Price granularity error: range list must be ordered with increasing "max"`)
}

if gr.Increment <= 0.0 {
return errors.New("Price granularity error: increment must be a nonzero positive number")
}
prevMax = gr.Max
}
return nil
}

Expand Down
174 changes: 174 additions & 0 deletions endpoints/openrtb2/auction_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1890,6 +1890,180 @@ func TestValidateTargeting(t *testing.T) {
},
expectedError: errors.New("Price granularity error: increment must be a nonzero positive number"),
},
{
name: "media type price granularity video correct",
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that we have separated out price granularity validation in a separate method, you think we can add another unit test specifically for logic in validatePriceGranularity function and move some of the test cases pertaining to that logic from this test to the new test?

This allows detailed test cases to be separated out from higher level function test cases. Plus, the test cases in this test have become too large and this will help keep them to a manageable number as well.

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! Some tests are gone due to duplicated tests with validatePriceGranularity. I left some of previously added tests where we test more than one price granularities where all are incorrect/correct/or some correct and some incorrect. I'm open to remove some of them if you think they are redundant

givenTargeting: &openrtb_ext.ExtRequestTargeting{
IncludeWinners: ptrutil.ToPtr(true),
MediaTypePriceGranularity: &openrtb_ext.MediaTypePriceGranularity{
Video: &openrtb_ext.PriceGranularity{
Precision: ptrutil.ToPtr(2),
Ranges: []openrtb_ext.GranularityRange{
{Min: 0.0, Max: 10.0, Increment: 1},
},
},
},
},
expectedError: nil,
},
{
name: "media type price granularity banner correct",
givenTargeting: &openrtb_ext.ExtRequestTargeting{
IncludeWinners: ptrutil.ToPtr(true),
MediaTypePriceGranularity: &openrtb_ext.MediaTypePriceGranularity{
Banner: &openrtb_ext.PriceGranularity{
Precision: ptrutil.ToPtr(2),
Ranges: []openrtb_ext.GranularityRange{
{Min: 0.0, Max: 10.0, Increment: 1},
},
},
},
},
expectedError: nil,
},
{
name: "media type price granularity native correct",
givenTargeting: &openrtb_ext.ExtRequestTargeting{
IncludeWinners: ptrutil.ToPtr(true),
MediaTypePriceGranularity: &openrtb_ext.MediaTypePriceGranularity{
Native: &openrtb_ext.PriceGranularity{
Precision: ptrutil.ToPtr(2),
Ranges: []openrtb_ext.GranularityRange{
{Min: 0.0, Max: 20.0, Increment: 1},
},
},
},
},
expectedError: nil,
},
{
name: "media type price granularity video and banner correct",
givenTargeting: &openrtb_ext.ExtRequestTargeting{
IncludeWinners: ptrutil.ToPtr(true),
MediaTypePriceGranularity: &openrtb_ext.MediaTypePriceGranularity{
Banner: &openrtb_ext.PriceGranularity{
Precision: ptrutil.ToPtr(2),
Ranges: []openrtb_ext.GranularityRange{
{Min: 0.0, Max: 10.0, Increment: 1},
},
},
Video: &openrtb_ext.PriceGranularity{
Precision: ptrutil.ToPtr(2),
Ranges: []openrtb_ext.GranularityRange{
{Min: 0.0, Max: 10.0, Increment: 1},
},
},
},
},
expectedError: nil,
},
{
name: "media type price granularity video and native correct",
givenTargeting: &openrtb_ext.ExtRequestTargeting{
IncludeWinners: ptrutil.ToPtr(true),
MediaTypePriceGranularity: &openrtb_ext.MediaTypePriceGranularity{
Native: &openrtb_ext.PriceGranularity{
Precision: ptrutil.ToPtr(2),
Ranges: []openrtb_ext.GranularityRange{
{Min: 0.0, Max: 30.0, Increment: 1},
},
},
Video: &openrtb_ext.PriceGranularity{
Precision: ptrutil.ToPtr(2),
Ranges: []openrtb_ext.GranularityRange{
{Min: 0.0, Max: 10.0, Increment: 1},
},
},
},
},
expectedError: nil,
},
{
name: "media type price granularity video incorrect",
givenTargeting: &openrtb_ext.ExtRequestTargeting{
IncludeWinners: ptrutil.ToPtr(true),
MediaTypePriceGranularity: &openrtb_ext.MediaTypePriceGranularity{
Video: &openrtb_ext.PriceGranularity{
Precision: ptrutil.ToPtr(2),
Ranges: []openrtb_ext.GranularityRange{
{Min: 0.0, Max: 10.0, Increment: -1},
},
},
},
},
expectedError: errors.New("Price granularity error: increment must be a nonzero positive number"),
},
{
name: "media type price granularity banner incorrect",
givenTargeting: &openrtb_ext.ExtRequestTargeting{
IncludeWinners: ptrutil.ToPtr(true),
MediaTypePriceGranularity: &openrtb_ext.MediaTypePriceGranularity{
Banner: &openrtb_ext.PriceGranularity{
Precision: ptrutil.ToPtr(2),
Ranges: []openrtb_ext.GranularityRange{
{Min: 0.0, Max: 0.0, Increment: 1},
},
},
},
},
expectedError: errors.New("Price granularity error: range list must be ordered with increasing \"max\""),
},
{
name: "media type price granularity native incorrect",
givenTargeting: &openrtb_ext.ExtRequestTargeting{
IncludeWinners: ptrutil.ToPtr(true),
MediaTypePriceGranularity: &openrtb_ext.MediaTypePriceGranularity{
Native: &openrtb_ext.PriceGranularity{
Precision: ptrutil.ToPtr(2),
Ranges: []openrtb_ext.GranularityRange{
{Min: 0.0, Max: 0.0, Increment: 1},
},
},
},
},
expectedError: errors.New("Price granularity error: range list must be ordered with increasing \"max\""),
},
{
name: "media type price granularity video correct and banner incorrect",
givenTargeting: &openrtb_ext.ExtRequestTargeting{
IncludeWinners: ptrutil.ToPtr(true),
MediaTypePriceGranularity: &openrtb_ext.MediaTypePriceGranularity{
Banner: &openrtb_ext.PriceGranularity{
Precision: ptrutil.ToPtr(2),
Ranges: []openrtb_ext.GranularityRange{
{Min: 0.0, Max: 10.0, Increment: -1},
},
},
Video: &openrtb_ext.PriceGranularity{
Precision: ptrutil.ToPtr(2),
Ranges: []openrtb_ext.GranularityRange{
{Min: 0.0, Max: 0.0, Increment: 1},
},
},
},
},
expectedError: errors.New("Price granularity error: range list must be ordered with increasing \"max\""),
},
{
name: "media type price granularity native incorrect and banner correct",
givenTargeting: &openrtb_ext.ExtRequestTargeting{
IncludeWinners: ptrutil.ToPtr(true),
MediaTypePriceGranularity: &openrtb_ext.MediaTypePriceGranularity{
Native: &openrtb_ext.PriceGranularity{
Precision: ptrutil.ToPtr(2),
Ranges: []openrtb_ext.GranularityRange{
{Min: 0.0, Max: 10.0, Increment: -1},
},
},
Video: &openrtb_ext.PriceGranularity{
Precision: ptrutil.ToPtr(2),
Ranges: []openrtb_ext.GranularityRange{
{Min: 0.0, Max: 0.0, Increment: 1},
},
},
},
},
expectedError: errors.New("Price granularity error: range list must be ordered with increasing \"max\""),
},
}

for _, tc := range testCases {
Expand Down
4 changes: 2 additions & 2 deletions exchange/auction.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,12 +185,12 @@ func (a *auction) validateAndUpdateMultiBid(adapterBids map[openrtb_ext.BidderNa
}
}

func (a *auction) setRoundedPrices(priceGranularity openrtb_ext.PriceGranularity) {
func (a *auction) setRoundedPrices(targetingData *targetData) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can make the targetData argument to be a non-pointer here and in the GetPriceBucket method. The pointer in the signature tells me that one of these functions might be mutating the object which isn't the case. Also, see how the openrtb_ext.PriceGranularity object previously was also a non-pointer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed

roundedPrices := make(map[*entities.PbsOrtbBid]string, 5*len(a.winningBids))
for _, topBidsPerImp := range a.winningBidsByBidder {
for _, topBidsPerBidder := range topBidsPerImp {
for _, topBid := range topBidsPerBidder {
roundedPrices[topBid] = GetPriceBucket(topBid.Bid.Price, priceGranularity)
roundedPrices[topBid] = GetPriceBucket(*topBid.Bid, targetingData)
}
}
}
Expand Down
54 changes: 11 additions & 43 deletions exchange/exchange.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -390,7 +390,7 @@ func (e *exchange) HoldAuction(ctx context.Context, r AuctionRequest, debugLog *
// A non-nil auction is only needed if targeting is active. (It is used below this block to extract cache keys)
auc = newAuction(adapterBids, len(r.BidRequestWrapper.Imp), targData.preferDeals)
auc.validateAndUpdateMultiBid(adapterBids, targData.preferDeals, r.Account.DefaultBidLimit)
auc.setRoundedPrices(targData.priceGranularity)
auc.setRoundedPrices(targData)

if requestExtPrebid.SupportDeals {
dealErrs := applyDealSupport(r.BidRequestWrapper.BidRequest, auc, bidCategory, multiBidMap)
Expand Down Expand Up @@ -908,7 +908,7 @@ func applyCategoryMapping(ctx context.Context, targeting openrtb_ext.ExtRequestT
bidID := bid.Bid.ID
var duration int
var category string
var pb string
var priceBucket string

if bid.BidVideo != nil {
duration = bid.BidVideo.Duration
Expand Down Expand Up @@ -942,7 +942,7 @@ func applyCategoryMapping(ctx context.Context, targeting openrtb_ext.ExtRequestT

// TODO: consider should we remove bids with zero duration here?

pb = GetPriceBucket(bid.Bid.Price, targData.priceGranularity)
priceBucket = GetPriceBucket(*bid.Bid, targData)

newDur := duration
if len(targeting.DurationRangeSec) > 0 {
Expand All @@ -967,10 +967,10 @@ func applyCategoryMapping(ctx context.Context, targeting openrtb_ext.ExtRequestT
var categoryDuration string
var dupeKey string
if brandCatExt.WithCategory {
categoryDuration = fmt.Sprintf("%s_%s_%ds", pb, category, newDur)
categoryDuration = fmt.Sprintf("%s_%s_%ds", priceBucket, category, newDur)
dupeKey = category
} else {
categoryDuration = fmt.Sprintf("%s_%ds", pb, newDur)
categoryDuration = fmt.Sprintf("%s_%ds", priceBucket, newDur)
dupeKey = categoryDuration
}

Expand All @@ -984,7 +984,7 @@ func applyCategoryMapping(ctx context.Context, targeting openrtb_ext.ExtRequestT
if err != nil {
dupeBidPrice = 0
}
currBidPrice, err := strconv.ParseFloat(pb, 64)
currBidPrice, err := strconv.ParseFloat(priceBucket, 64)
if err != nil {
currBidPrice = 0
}
Expand Down Expand Up @@ -1025,7 +1025,7 @@ func applyCategoryMapping(ctx context.Context, targeting openrtb_ext.ExtRequestT
}
}
res[bidID] = categoryDuration
dedupe[dupeKey] = bidDedupe{bidderName: bidderName, bidIndex: bidInd, bidID: bidID, bidPrice: pb}
dedupe[dupeKey] = bidDedupe{bidderName: bidderName, bidIndex: bidInd, bidID: bidID, bidPrice: priceBucket}
}

if len(bidsToRemove) > 0 {
Expand Down Expand Up @@ -1349,20 +1349,6 @@ func listBiddersWithRequests(bidderRequests []BidderRequest) []openrtb_ext.Bidde
return liveAdapters
}

func getPrebidMediaTypeForBid(bid openrtb2.Bid) (openrtb_ext.BidType, error) {
if bid.Ext != nil {
var bidExt openrtb_ext.ExtBid
err := json.Unmarshal(bid.Ext, &bidExt)
if err == nil && bidExt.Prebid != nil {
return openrtb_ext.ParseBidType(string(bidExt.Prebid.Type))
}
}

return "", &errortypes.BadServerResponse{
Message: fmt.Sprintf("Failed to parse bid mediatype for impression \"%s\"", bid.ImpID),
}
}

func buildStoredAuctionResponse(storedAuctionResponses map[string]json.RawMessage) (
map[openrtb_ext.BidderName]*entities.PbsOrtbSeatBid,
*openrtb_ext.Fledge,
Expand All @@ -1383,28 +1369,10 @@ func buildStoredAuctionResponse(storedAuctionResponses map[string]json.RawMessag
//set imp id from request
for i := range seat.Bid {
seat.Bid[i].ImpID = impId
mType := seat.Bid[i].MType
var bidType openrtb_ext.BidType
if mType > 0 {
switch mType {
case openrtb2.MarkupBanner:
bidType = openrtb_ext.BidTypeBanner
case openrtb2.MarkupVideo:
bidType = openrtb_ext.BidTypeVideo
case openrtb2.MarkupAudio:
bidType = openrtb_ext.BidTypeAudio
case openrtb2.MarkupNative:
bidType = openrtb_ext.BidTypeNative
default:
return nil, nil, nil, &errortypes.BadServerResponse{
Message: fmt.Sprintf("Failed to parse bid mType for impression \"%s\"", seat.Bid[i].ImpID),
}
}
} else {
var err error
bidType, err = getPrebidMediaTypeForBid(seat.Bid[i])
if err != nil {
return nil, nil, nil, err
bidType, err := getMediaTypeForBid(seat.Bid[i])
if err != nil {
return nil, nil, nil, &errortypes.BadServerResponse{
Message: fmt.Sprintf("Failed to parse bid mType for impression \"%s\"", seat.Bid[i].ImpID),
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest we move this error message inside getMediaTypeForBid to be returned when the mType doesn't match one of the values we expect i.e. the default case here. On this line you can then just return the error returned by the getMediaTypeForBid call. This allows for specific errors to be returned based on the logic in getMediaTypeForBid

Copy link
Contributor Author

@VeronikaSolovei9 VeronikaSolovei9 May 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I see your idea. I tried it locally and find out we need to intercept a final error and replace error message.
When error happens in ParseBidType it has this message: "invalid BidType: incorrect"
It goes up the stack to getPrebidMediaTypeForBid and then we return it as is.
To return the right message we need to intercept error outside of getMediaTypeForBid and replace it with the right message.

What do you think?

}
}
bidsToAdd = append(bidsToAdd, &entities.PbsOrtbBid{Bid: &seat.Bid[i], BidType: bidType})
Expand Down
2 changes: 1 addition & 1 deletion exchange/exchange_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4388,7 +4388,7 @@ func TestBuildStoredAuctionResponses(t *testing.T) {
},
liveAdapters: []openrtb_ext.BidderName{openrtb_ext.BidderName("appnexus")},
},
errorMessage: "invalid BidType: incorrect",
errorMessage: "Failed to parse bid mType for impression \"impression-id\"",
},
{
desc: "Single imp with multiple bids in stored response one bidder",
Expand Down
Loading