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

Jump to bottom

Correct GetCpmStringValue's second return value #1520

Merged
merged 9 commits into from
Oct 15, 2020
Merged

Correct GetCpmStringValue's second return value #1520

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
0916618
First draft
Oct 2, 2020
b5f85fc
Correctinos after feedback
Oct 8, 2020
601ffd4
legacy endpoint issue
Oct 8, 2020
6a05aad
Rolledback bucketMax if statement
Oct 8, 2020
df76541
set the MaxDecimalFigures to 15
Oct 14, 2020
61365d0
Return error if precision is greater than 15
Oct 14, 2020
388a8a0
Merge branch 'master' into PBS-768
Oct 14, 2020
0ecbfed
Correct maximum precision corner case
Oct 15, 2020
69a2ded
Removed leftover assertion
Oct 15, 2020
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
23 changes: 15 additions & 8 deletions exchange/price_granularity.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package exchange

import (
"fmt"
"math"
"strconv"

Expand All @@ -15,21 +16,27 @@ func GetCpmStringValue(cpm float64, config openrtb_ext.PriceGranularity) (string
bucketMax := 0.0
SyntaxNode marked this conversation as resolved.
Show resolved Hide resolved
increment := 0.0
precision := config.Precision
// calculate max of highest bucket

// Limit the number of decimal significant figures. Very large values lead to Panics
if precision > 4 {
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs review from @camrice and/or @VeronikaSolovei9. Can you please explain how large values lead to panics?

Copy link
Contributor

@VeronikaSolovei9 VeronikaSolovei9 Oct 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I tested current upstream implementation with precision = 10 and it works.

{
                    "hb_pb": "17.0000000000",
                    "hb_pb_cat_dur": "17.0000000000_food_30s",
                    "hb_cache_id": "37be5f83-4be9-41b4-ab3f-84883205591d"
                }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@VeronikaSolovei9 do you think we should should put a limit to precision? If so, what should it be?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should not return error in this case, instead just overwrite precision with maximum reasonable value?

Copy link
Contributor

Choose a reason for hiding this comment

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

@guscarreon 2 and 4 sounds reasonable to me, but please confirm it with product owners as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

In this case, the product owners are the Prebid Server Committee. This is already configurable by the publisher. What's the worst thing that would happen if they enter a value too large? Is this really something we need to code logic for?

Copy link
Contributor

Choose a reason for hiding this comment

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

"hb_pb_cat_dur" is a targeting key for Ad server to select Line Items. Redirect URL also contains it. With wrong precision Ad server set up will not work.

return cpmStr, fmt.Errorf("Limit the number of precision figures to 4. Parsed value: %d", precision)
}

for i := 0; i < len(config.Ranges); i++ {
if config.Ranges[i].Max > bucketMax {
// calculate max of highest bucket
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider removing this comment. I think the logic combined with the name bucketMax is self explanatory.

if bucketMax < config.Ranges[i].Max {
SyntaxNode marked this conversation as resolved.
Show resolved Hide resolved
bucketMax = config.Ranges[i].Max
}
} // calculate which bucket cpm is in
if cpm > bucketMax {
// If we are over max, just return that
return strconv.FormatFloat(bucketMax, 'f', precision, 64), nil
}
for i := 0; i < len(config.Ranges); i++ {
// find range cpm is in
if cpm >= config.Ranges[i].Min && cpm <= config.Ranges[i].Max {
increment = config.Ranges[i].Increment
}
}
// If we are over max, just return that
if cpm > bucketMax {
return strconv.FormatFloat(bucketMax, 'f', precision, 64), nil
}
// If increment exists, get cpm string value
if increment > 0 {
cpmStr = getCpmTarget(cpm, increment, precision)
}
Expand Down
134 changes: 108 additions & 26 deletions exchange/price_granularity_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
package exchange

import (
"fmt"
"math"
"testing"

"github.com/prebid/prebid-server/openrtb_ext"
"github.com/stretchr/testify/assert"
)

func TestGetPriceBucketString(t *testing.T) {
Expand All @@ -28,32 +31,111 @@ func TestGetPriceBucketString(t *testing.T) {
},
}

price := 1.87
getOnePriceBucket(t, "low", low, price, "1.50")
getOnePriceBucket(t, "medium", medium, price, "1.80")
getOnePriceBucket(t, "high", high, price, "1.87")
getOnePriceBucket(t, "auto", auto, price, "1.85")
getOnePriceBucket(t, "dense", dense, price, "1.87")
getOnePriceBucket(t, "custom1", custom1, price, "1.86")

// test a cpm above the max in low price bucket
price = 5.72
getOnePriceBucket(t, "low", low, price, "5.00")
getOnePriceBucket(t, "medium", medium, price, "5.70")
getOnePriceBucket(t, "high", high, price, "5.72")
getOnePriceBucket(t, "auto", auto, price, "5.70")
getOnePriceBucket(t, "dense", dense, price, "5.70")
getOnePriceBucket(t, "custom1", custom1, price, "5.70")

}

func getOnePriceBucket(t *testing.T, name string, granularity openrtb_ext.PriceGranularity, price float64, expected string) {
t.Helper()
priceBucket, err := GetCpmStringValue(price, granularity)
if err != nil {
t.Errorf("Granularity: %s :: GetPriceBucketString: %s", name, err.Error())
// Define test cases
type aTest struct {
granularityId string
inGranularity openrtb_ext.PriceGranularity
Copy link
Contributor

Choose a reason for hiding this comment

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

Rename to granularity?

expectedPriceBucket string
expectedError error
}
testGroups := []struct {
groupDesc string
inCpm float64
Copy link
Contributor

Choose a reason for hiding this comment

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

Rename to cpm?

testCases []aTest
}{
{
"cpm below the max in every price bucket",
Copy link
Contributor

Choose a reason for hiding this comment

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

The values here are not self descriptive enough. Please add field labels.

1.87,
[]aTest{
{"low", low, "1.50", nil},
{"medium", medium, "1.80", nil},
{"high", high, "1.87", nil},
{"auto", auto, "1.85", nil},
{"dense", dense, "1.87", nil},
{"custom1", custom1, "1.86", nil},
},
},
{
"cpm above the max in low price bucket",
5.72,
[]aTest{
{"low", low, "5.00", nil},
{"medium", medium, "5.70", nil},
{"high", high, "5.72", nil},
{"auto", auto, "5.70", nil},
{"dense", dense, "5.70", nil},
{"custom1", custom1, "5.70", nil},
},
},
{
"Precision value corner cases",
1.876,
[]aTest{
{
"Negative precision defaults to number of digits already in CPM float",
SyntaxNode marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where is the code that deals with a negative precision?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Function strconv.FormatFloat(roundedCPM, 'f', precision, 64) deals with negative precision values. More details in the function definition itself.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Are you testing that strconv.FormatFloat() is working properly, or that something is handling the case properly when FormatFloat() does what it is supposed to do, or that your code is subsequently handling that output correctly? I don't see any code that would react to anything particular about a negative number.

Copy link
Contributor

Choose a reason for hiding this comment

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

prebid-server/openrtb_ext/request.go

Line 178 in 5507707

if pgraw.Precision < 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.

Given that PriceGranularity has its own func (pg *PriceGranularity) UnmarshalJSON(b []byte) error inside openrtb_ext/request.go that sets a bunch of default values if found missing in the Json string, I thought it'd be better to cap the precision value there instead.

openrtb_ext.PriceGranularity{Precision: -1, Ranges: []openrtb_ext.GranularityRange{{Max: 5, Increment: 0.05}}},
"1.85",
nil,
},
{
"Precision value equals zero, we expect to round up to the nearest integer",
openrtb_ext.PriceGranularity{Ranges: []openrtb_ext.GranularityRange{{Max: 5, Increment: 0.05}}},
"2",
nil,
},
{
"Very large precision value: expect error and an empty string in return",
openrtb_ext.PriceGranularity{Precision: int(^uint(0) >> 1), Ranges: []openrtb_ext.GranularityRange{{Max: 5, Increment: 0.05}}},
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we really want to test with about the largest possible integer when the limit is only 4? I would worry that this could potentially generate an error due to being silly large rather than violating our stated limit of 4. Would probably generate a different error message, or error out in another way, so would still be flagged, but doesn't make sense as the number doesn't really compare to 4.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are totally right, that was too much. The latest version limits the precision value to 2^15 that both not panics and seems to not overflow the return string. I believe 2^15 = 32768 is more than enough. but let me know your take on this.

"",
fmt.Errorf("Limit the number of precision figures to 4. Parsed value: %d", int(^uint(0)>>1)),
},
},
},
{
"Increment value corner cases",
1.876,
[]aTest{
{
"Negative increment, return empty string",
openrtb_ext.PriceGranularity{Precision: 2, Ranges: []openrtb_ext.GranularityRange{{Max: 5, Increment: -0.05}}},
"",
nil,
},
{
"Zero increment, return empty string",
openrtb_ext.PriceGranularity{Precision: 2, Ranges: []openrtb_ext.GranularityRange{{Max: 5}}},
"",
nil,
},
{
"Increment value is greater than CPM itself, return zero float value",
openrtb_ext.PriceGranularity{Precision: 2, Ranges: []openrtb_ext.GranularityRange{{Max: 5, Increment: 1.877}}},
"0.00",
nil,
},
},
},
{
"Negative Cpm, return empty string since it does not belong into any range",
-1.876,
[]aTest{{"low", low, "", nil}},
},
{
"Zero value Cpm, return the same, only in string format",
0,
[]aTest{{"low", low, "0.00", nil}},
},
{
"Large Cpm, return bucket Max",
math.MaxFloat64,
[]aTest{{"low", low, "5.00", nil}},
},
}
if priceBucket != expected {
t.Errorf("Granularity: %s :: Expected %s, got %s from %f", name, expected, priceBucket, price)
for _, testGroup := range testGroups {
for _, test := range testGroup.testCases {
priceBucket, err := GetCpmStringValue(testGroup.inCpm, test.inGranularity)
assert.Equalf(t, test.expectedPriceBucket, priceBucket, "Group: %s Granularity: %s :: Expected %s, got %s from %f", testGroup.groupDesc, test.granularityId, test.expectedPriceBucket, priceBucket, testGroup.inCpm)
assert.Equalf(t, test.expectedError, err, "Error value doesn't match")
}
}
}