Prebid Core: Allow Cpm Custom Rounding

[spotxslagle]

For any user facing change, submit a link to a PR on the docs repo at https://github.com/prebid/prebid.github.io/
-->

Type of change

• [ x ] Feature

Description of change

Allow publishers to set their own rounding rules as desired

Other information

@robertrmartinez

[spotxslagle]

I'll link doc changes when I have them

[robertrmartinez]

Looks fine to me, my only question is that if we should put some protection around the customFunction?

Maybe a try / catch to save pubs from themselves, and if caught use Math.floor ?

What do you think @dgirardi

[spotxslagle]

Maybe a try / catch to save pubs from themselves, and if caught use Math.floor ?

That makes sense to me. I'm guessing a lot of pubs are going to end up calling their rounding function when they try to set it. I'll get that pushed up shortly

[robertrmartinez]

Maybe a test case that also handles if they DO pass a function but the function itself throws an error.

I think the case here actually is testing this part:

  let roundingFunction = Math.floor;
  let customRoundingFunction = config.getConfig('cpmRoundingFunction');
  if (typeof customRoundingFunction === 'function') {
    roundingFunction = customRoundingFunction;
  }

since config.getConfig = () => Math.ceil(5.3) will just return 6.0 not a function.

So typeof customRoundingFunction === 'function' will be false so it will keep default of Math.floor

So another test which does something like:

config.getConfig = () => () => {
   throw new Error('I am an error');
}

So it actually sets roundingFunction to the input customRoundingFunction and tries to execute it, but since it throws an error, it uses Math.floor

[robertrmartinez]

Should we log a warning here so easier to debug when this exception occurs?

[dgirardi]

This change LGTM, but isn't the use case for this already covered by this configurable precision (as explained here)? This is probably more about documentation, it's not clear to me why I'd want to use one over the other.

[spotxslagle]

Precision states where the rounding will occur, but the rounding is always down. This change allows for other rounding methods.

e.g.
Current
cpm = 2.1456, increment = .05, precision = 3
precise cpm = 2.145, rounded down to nearest increment = 2.10
Final result = 2.100

With alternate rounding
cpm = 2.1456, increment = 0.5 precision = 4
precise cpm = 2.1456, rounded with Math.round to nearest increment = 2.15
Final result = 2.1500

At least, that's how I understand it. As far as I can tell, the precision determines what is being rounded down, but it is always rounded down, and always to the nearest increment, which in effect just tacks extra zeroes to the end of the result.

Then again, I'm mostly basing that off the unit tests, which aren't super comprehensive for precision, and the documentation, which only shows the default of 2. If precision is supposed to affect rounding in any way, I don't get it. As long as the precision is greater than or equal to the number of decimal places in the increment, rounding will be unaffected by precision, which makes this change useful.

[dgirardi]

You're right, I thought you could get the equivalent of Math.round by increasing the precision by 1, but that's not exactly what happens.

[patmmccann]

If you round up, what happens if someone bids 50000.00 usd for example. Please add a unit test to demonstrate this is handled and discuss in documentation bids over the top value.

[spotxslagle]

I don't really understand what the requested unit test and docs changes have to do with this change. That behavior is entirely unaffected. As noted in the comment on line 85 (now 86), if the bid exceeds the cap, the cap will be returned. It will do so without ever touching this change. Even if it were related, the current unit tests cover this sufficiently, as the cap is returned for the low price config in the custom rounding function unit test.

[spotxslagle]

@patmmccann What's your take? I haven't worked with the bucket manager before, so I wouldn't be shocked if I missed something. Do you think more unit tests/documentation is needed here?