-
Notifications
You must be signed in to change notification settings - Fork 761
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.
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Just some minor comments
endpoints/openrtb2/auction.go
Outdated
} | ||
} | ||
|
||
return nil | ||
} | ||
|
||
func validatePriceGranularity(pg *openrtb_ext.PriceGranularity) error { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: I think you can remove this blank line
endpoints/openrtb2/auction.go
Outdated
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 { |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added
exchange/price_granularity.go
Outdated
if targetingData.mediaTypePriceGranularity != nil { | ||
bidType, err := getMediaTypeForBid(bid) | ||
if err != nil { | ||
config = targetingData.priceGranularity //assign default price granularity |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because of the config setting on line 17, why do we need this line here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I put it here to show explicitly we want to use default price granularity in case of error - if media type for bid is not found. Otherwise I'll need to ignore the error.
I modified comment.
I'm open to change it if you think it should be different.
exchange/exchange.go
Outdated
@@ -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) | |||
pb = GetPriceBucket(*bid.Bid, targData) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: What do you think about re-naming this variable to be priceBucket
openrtb_ext/request.go
Outdated
DurationRangeSec []int `json:"durationrangesec,omitempty"` | ||
PreferDeals bool `json:"preferdeals,omitempty"` | ||
AppendBidderNames bool `json:"appendbiddernames,omitempty"` | ||
MediaTypePriceGranularity *MediaTypePriceGranularity `json:"mediatypepricegranularity,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: I liked how you had the MediaTypePriceGranularity
come right after PriceGranularity
, could you move the media type one to right under line 154?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, left one minor comment
"banner": { | ||
"precision": 3, | ||
"ranges": [ | ||
{ | ||
"max": 20, | ||
"increment": 4.5 | ||
} | ||
] | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For my understanding, because i've seen this in other mediattypepricegranualrity
tests, is in the filename it specifies native
and
video only, but in the file I see a banner
price granularity specified as well. Why is this the case? Could the banner
part of this be removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, idea here is to have configs for all media types. Native and video are bids that are returned.
Test also tests that req.ext.prebidmediattypepricegranualrity.mediatype.video
will be applied for video bid and req.ext.prebidmediattypepricegranualrity.mediatype.native
will be applied to native bid.
Regardless of what we have in req.ext.prebidmediattypepricegranualrity.mediatype
correct config will be selected.
I'm open to remove it and leave native and video configs only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two more minor comments
|
||
var prevMax float64 = 0 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
exchange/exchange.go
Outdated
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), |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
exchange/price_granularity.go
Outdated
config := targetingData.priceGranularity //assign default price granularity | ||
if targetingData.mediaTypePriceGranularity != nil { | ||
bidType, err := getMediaTypeForBid(bid) | ||
if err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can replace the err != nil
check here to err == nil
and skip the default price granularity assignment as you're already doing that on L17
config := targetingData.priceGranularity //assign default price granularity
if targetingData.mediaTypePriceGranularity != nil {
if bidType, err := getMediaTypeForBid(bid); err == nil {
if bidType == openrtb_ext.BidTypeBanner && targetingData.mediaTypePriceGranularity.Banner != nil {
config = *targetingData.mediaTypePriceGranularity.Banner
} else if bidType == openrtb_ext.BidTypeVideo && targetingData.mediaTypePriceGranularity.Video != nil {
config = *targetingData.mediaTypePriceGranularity.Video
} else if bidType == openrtb_ext.BidTypeNative && targetingData.mediaTypePriceGranularity.Native != nil {
config = *targetingData.mediaTypePriceGranularity.Native
}
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point!
ortb/default.go
Outdated
@@ -66,6 +88,16 @@ func setDefaultsTargeting(targeting *openrtb_ext.ExtRequestTargeting) bool { | |||
return modified | |||
} | |||
|
|||
func adjustDefaultsPriceGranularity(priceGranularity *openrtb_ext.PriceGranularity) (*openrtb_ext.PriceGranularity, bool) { | |||
if priceGranularity == nil || len(priceGranularity.Ranges) == 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding this condition in the setDefaultsPriceGranularity
function itself at the very start. Something like:
func setDefaultsPriceGranularity(pg *openrtb_ext.PriceGranularity) (*openrtb_ext.PriceGranularity, bool) {
if pg == nil || len(pg.Ranges) == 0 {
pg = ptrutil.ToPtr(openrtb_ext.NewPriceGranularityDefault())
return pg, true
}
modified := false
if pg.Precision == nil {
pg.Precision = ptrutil.ToPtr(DefaultPriceGranularityPrecision)
modified = true
}
if setDefaultsPriceGranularityRange(pg.Ranges) {
modified = true
}
return pg, modified
}
The reason I am saying this is because having two functions with names adjustDefaultsPriceGranularity
and setDefaultsPriceGranularity
doesn't clearly describe the difference between what they are doing and I think we can easily accommodate this behavior in the setDefaultsPriceGranularity
function itself
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, modified
|
||
func validatePriceGranularity(pg *openrtb_ext.PriceGranularity) error { | ||
if pg.Precision == nil { | ||
return errors.New("Price granularity error: precision is required") |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Left one minor comment
openrtb_ext/request.go
Outdated
// PriceGranularity defines the allowed values for bidrequest.ext.prebid.targeting.pricegranularity | ||
// or bidrequest.ext.prebid.targeting.mediatypepricegranularity |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this comment live above the definition of MediaTypePriceGranularity
on line 172?
e.g: MediaTypePriceGranulairty defines allowed values for bidrequest.ext.prebid.targeting.mediatypepricegranularity
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added comment for MediaTypePriceGranularity
.
Modified this comment to this:
// PriceGranularity defines the allowed values for bidrequest.ext.prebid.targeting.pricegranularity
// or bidrequest.ext.prebid.targeting.mediatypepricegranularity.banner|video|native
Idea here is to show that same structure is used for both pricegranularity
and mediatypepricegranularity.banner|video|native
exchange/utils.go
Outdated
err := json.Unmarshal(bid.Ext, &bidExt) | ||
if err == nil && bidExt.Prebid != nil { | ||
bidType, bidTypeErr := openrtb_ext.ParseBidType(string(bidExt.Prebid.Type)) | ||
if bidTypeErr != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about updating this function to:
func getPrebidMediaTypeForBid(bid openrtb2.Bid) (openrtb_ext.BidType, error) {
var err error
if bid.Ext != nil {
var bidExt openrtb_ext.ExtBid
err = json.Unmarshal(bid.Ext, &bidExt)
if err == nil && bidExt.Prebid != nil {
bidType, err := openrtb_ext.ParseBidType(string(bidExt.Prebid.Type))
if err == nil {
return bidType, nil
}
}
}
errMsg := fmt.Sprintf("Failed to parse bid mediatype for impression \"%s\"", bid.ImpID)
if err != nil {
errMsg = fmt.Sprintf("%s: %s", errMsg, err.Error())
}
return "", &errortypes.BadServerResponse{
Message: errMsg,
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per our offline discussion we agreed to append error from ParseBidType
to result message. With this implementation if error occurs in ParseBidType
it becomes shadowed and if err != nill
check returns false and err.Error()
concatenation never executes.
To fix this, 2 different error variables should be declared, here is a working version:
func getPrebidMediaTypeForBid(bid openrtb2.Bid) (openrtb_ext.BidType, error) {
var err, err2 error
var bt openrtb_ext.BidType
if bid.Ext != nil {
var bidExt openrtb_ext.ExtBid
err = json.Unmarshal(bid.Ext, &bidExt)
if err == nil && bidExt.Prebid != nil {
bt, err2 = openrtb_ext.ParseBidType(string(bidExt.Prebid.Type))
if err2 == nil {
return bt, nil
}
}
}
errMsg := fmt.Sprintf("Failed to parse bid mediatype for impression \"%s\"", bid.ImpID)
if err2 != nil {
errMsg = fmt.Sprintf("%s, %s", errMsg, err2.Error())
}
return "", &errortypes.BadServerResponse{
Message: errMsg,
}
}
It feels a little too complicated for such a simple function, but I can modify it if you think it's better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah! err
was getting shadowed because I think I missed this line:
bidType, err := openrtb_ext.ParseBidType(string(bidExt.Prebid.Type))
where we're assigning a new err
variable. You can modify it to as you suggested above but without two error variables
func getPrebidMediaTypeForBid(bid openrtb2.Bid) (openrtb_ext.BidType, error) {
var err error
var bidType openrtb_ext.BidType
if bid.Ext != nil {
var bidExt openrtb_ext.ExtBid
err = json.Unmarshal(bid.Ext, &bidExt)
if err == nil && bidExt.Prebid != nil {
if bidType, err = openrtb_ext.ParseBidType(string(bidExt.Prebid.Type)); err == nil {
return bidType, nil
}
}
}
errMsg := fmt.Sprintf("Failed to parse bid mediatype for impression \"%s\"", bid.ImpID)
if err != nil {
errMsg = fmt.Sprintf("%s: %s", errMsg, err.Error())
}
return bidType, &errortypes.BadServerResponse{
Message: errMsg,
}
}
The reason I asked for this change was to keep the error handling common and as descriptive as possible for the user. For example, with the current code, if there was an invalid JSON error, it wouldn't show up in the final error message but with this change it will. This helps users to get a better understanding of the problem and how they can solve it. It also makes it easier to maintain the code in the future as we make changes.
openrtb_ext/request.go
Outdated
PreferDeals bool `json:"preferdeals,omitempty"` | ||
AppendBidderNames bool `json:"appendbiddernames,omitempty"` | ||
PriceGranularity *PriceGranularity `json:"pricegranularity,omitempty"` | ||
MediaTypePriceGranularity *MediaTypePriceGranularity `json:"mediatypepricegranularity,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need MediaTypePriceGranularity
to be a pointer? I think it should be okay if we keep this as a non-pointer. The only use I see of this being a pointer type currently is for existence checks. However, I'd argue that we don't need those as the fields in this object for video
, banner
and native
are pointers so you could just directly check for their existence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@VeronikaSolovei9 Look at this example here that tries to mimic what we're doing with MediaTypePriceGranularity
. If you run the benchmark tests in that file, you'll see something like:
➜ heap_allocs go test -bench . -benchmem
goos: linux
goarch: amd64
pkg: heap_allocs
cpu: 11th Gen Intel(R) Core(TM) i7-1185G7 @ 3.00GHz
BenchmarkFooWithBar-8 15377624 73.71 ns/op 48 B/op 5 allocs/op
BenchmarkFooWithBarPtr-8 12967056 88.05 ns/op 56 B/op 6 allocs/op
BenchmarkFooWithBarEmpty-8 23041183 48.98 ns/op 32 B/op 3 allocs/op
BenchmarkFooWithBarPtrNil-8 29767474 41.38 ns/op 24 B/op 3 allocs/op
PASS
ok heap_allocs 8.178s
BenchmarkFooWithBar
and BenchmarkFooWithBarPtr
account for scenarios where MediaTypePriceGranularity
(aka bar
in above example) is present in the request vs BenchmarkFooWithBarEmpty
and BenchmarkFooWithBarPtrNil
account for scenarios where MediaTypePriceGranularity
(aka bar
in above example) is not present in the request.
In the scenarios where MediaTypePriceGranularity
is present in the request, you can see that using a non-pointer is better both in terms of number of operations and number of heap allocations (look at allocs/op).
In the scenarios where MediaTypePriceGranularity
is not present in the request, a pointer version seems to do better in terms of number of operations. However, the number of heap allocations are same.
So to conclude, considering we could have both cases where MediaTypePriceGranularity
may or may not be specified in the request, the non-pointer version makes more sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow! This is a great effort!
Provided example is a good way to demonstrate the difference, however it's far from what happens in actual code including data creation. In your example you create data directly like
return &FooWithBarPtr{
bar: &Bar{x: &x, y: &y},
}
}
in real prod code all objects are created by json unmarshal.
Here is the test with similar approach, but a bit closer to what we have in prod.
Here is the result:
go test -bench . -benchmem
goos: darwin
goarch: amd64
pkg: github.com/prebid/prebid-server
cpu: Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz
BenchmarkTargetingPtrNoData-16 2089046 566.0 ns/op 248 B/op 7 allocs/op
BenchmarkTargetingNoPtrNoData-16 2011388 597.1 ns/op 264 B/op 7 allocs/op
BenchmarkTargetingPtrWithData-16 176634 6913 ns/op 944 B/op 31 allocs/op
BenchmarkTargetingNoPtrWithData-16 179672 6633 ns/op 936 B/op 30 allocs/op
BenchmarkTargetingPtrNoDataModify-16 2089758 555.4 ns/op 248 B/op 7 allocs/op
BenchmarkTargetingNoPtrNoDataModify-16 2097586 577.9 ns/op 264 B/op 7 allocs/op
BenchmarkTargetingPtrWithDataModify-16 177097 6546 ns/op 944 B/op 31 allocs/op
BenchmarkTargetingNoPtrWithDataModify-16 177206 6833 ns/op 936 B/op 30 allocs/op
PASS
I consider them equal. Run it couple of times to see how it varies.
I went through Benchmark results explanation and still don't understand how this shows GC usage (improvement or deterioration), please explain, it will be useful for future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed offline with the team, decided to change to no-pointer.
exchange/targeting.go
Outdated
includeFormat bool | ||
preferDeals bool | ||
priceGranularity openrtb_ext.PriceGranularity | ||
mediaTypePriceGranularity *openrtb_ext.MediaTypePriceGranularity |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar comment about making this type a non-pointer as in openrtb_ext/request.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed
@@ -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", |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
# Conflicts: # adapters/appnexus/appnexustest/supplemental/multiple-member-ids.json
exchange/utils_test.go
Outdated
@@ -4129,3 +4129,97 @@ func Test_buildRequestExtMultiBid(t *testing.T) { | |||
}) | |||
} | |||
} | |||
|
|||
func TestGetPrebidMediaTypeForBid(t *testing.T) { | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: Could remove this blank line?
exchange/utils_test.go
Outdated
} | ||
|
||
func TestGetMediaTypeForBid(t *testing.T) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: Could you remove this blank line?
Precision: ptrutil.ToPtr(2), | ||
Ranges: []openrtb_ext.GranularityRange{ | ||
{Min: 0.0, Max: 10.0, Increment: 1}, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for separating the test cases out. This looks great. Can you please just add one more test case where a valid precision value is present but ranges is empty?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added
exchange/auction.go
Outdated
@@ -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) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed
}, | ||
"ext": { | ||
"debug": { | ||
"resolvedrequest": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a specific reason to be using test
flag in these JSON tests? I'd expect us to use outgoingRequests.appnexus.expectRequest
to assert the request being sent to the appnexus bidder is the same as what you expect it to be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, test
is not needed here, same as debug info in response.ext
.
Hovewer we need to get auction response, not bidder response. Bidder, in this case appnexus, returnes 3 bids with different media types. They don't have targeting keys. We need to run auction part where targeting keys and adjusted price are set and compare final auction response, not bidder response.
exchange/price_granularity_test.go
Outdated
}, | ||
}, | ||
{ | ||
groupDesc: "media type price granularity video for incorrect bid type", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The description for this test is slightly misleading as it has cases where you're providing banner and native mediatype price granularities as well. I suggest we change it to something along the lines of media type price granularity set but bid type incorrect
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, good point, changed
{ | ||
description: "Valid bid ext with non-existing bid type", | ||
inputBid: openrtb2.Bid{ID: "bidId", ImpID: "impId", Ext: json.RawMessage(`{"prebid": {"type": "unknown"}}`)}, | ||
expectedError: "Failed to parse bid mediatype for impression \"impId\", invalid BidType: unknown", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding these test cases. Can you please also add two more test cases here, one where bid.ext
is nil
aka not specified at all and one where bid.ext
is an empty JSON. I realize that your code will properly handle both of these test cases but this more to safeguard the code from future changes where someone might accidently remove the nil check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added
openrtb_ext/request.go
Outdated
@@ -201,7 +202,15 @@ type ExtIncludeBrandCategory struct { | |||
TranslateCategories *bool `json:"translatecategories,omitempty"` | |||
} | |||
|
|||
// MediaTypePriceGranularity defines values for bidrequest.ext.prebid.targeting.mediatypepricegranularity |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding another line in this description specifying what these values mean. Something like:
These values specify price granularity configuration at the bid type level.
MediaType PriceGranularity - when a single OpenRTB request contains multiple impressions with different mediatypes, or a single impression supports multiple formats, the different mediatypes may need different price granularities. If mediatypepricegranularity is present, pricegranularity would only be used for any mediatypes not specified.
Request format:
Response format (returned in bid.ext.prebid.targeting)
For more information please refer to this Prebid documentation