Prebid Core: Refine coordination of PBS+PBJS Floors #8865

jlquaccia · 2022-08-18T19:38:04Z

Type of change

Enhancement

Description of change

stronger coordination between pbs and pbjs floors
will set the floorMins pbs if bidfloor values within imp's are detected

Other information

Related Issue: #8749
@dgirardi could you review when you have time?

dgirardi · 2022-08-19T15:59:42Z

modules/prebidServerBidAdapter/index.js

@@ -902,6 +902,20 @@ Object.assign(ORTB2.prototype, {
    addBidderFirstPartyDataToRequest(request, s2sBidRequest.ortb2Fragments?.bidder || {});

    request.imp.forEach((imp) => this.impRequested[imp.id] = imp);
+
+    if (typeof request.ext.prebid.floors === 'object' && request.ext.prebid.floors.enabled) {


the typeof check is not that useful because typeof null === 'object'. IMO it's better to use if (request.ext?.prebid?.floors?.enabled).

dgirardi · 2022-08-19T16:02:14Z

modules/prebidServerBidAdapter/index.js

+          bidfloorArray.push(imp.bidfloor);
+        }
+      });
+      request.ext.prebid.floors.floorMin = Math.min(...bidfloorArray);


this loops over floors twice more, which is rather inefficient compared to the zero that are required. You could keep track of the minimum as we calculate the floors, and here just append it to the request if ext.prebid.floors.enabled is true.

thanks @dgirardi. would the changes for my latest commit work? also, one additional question, for each imp would i still need to assign imp.ext.prebid.floors.floorMin = imp.bidfloor? (as seen in the comment above)

dgirardi · 2022-08-19T19:56:55Z

modules/prebidServerBidAdapter/index.js

@@ -728,6 +729,7 @@ Object.assign(ORTB2.prototype, {
      if (floor) {
        imp.bidfloor = floor.floor;
        imp.bidfloorcur = floor.currency;
+        bidfloorArray.push(floor.floor);


yes, you should copy over to floorMin here, and still spare the Math.min loop by just keeping the minimum here. (Instead of bidfloorArray = [] to start, you could set let floorMin = null and here do if (floorMin == null || floorMin > imp.bidfloor) { floorMin = imp.bidfloor }.

@dgirardi thanks, just made those changes to the PR

dgirardi · 2022-08-19T21:29:54Z

modules/prebidServerBidAdapter/index.js

@@ -728,6 +729,8 @@ Object.assign(ORTB2.prototype, {
      if (floor) {
        imp.bidfloor = floor.floor;
        imp.bidfloorcur = floor.currency;
+        imp.ext.prebid.floors = { floorMin: imp.bidfloor };


I'm not sure imp.ext.prebid always exists at this point; I would prefer deepSetValue(imp, 'ext.prebid.floors.floorMin', imp.bidfloor).

(I believe in some cases, like stored impressions where no params were set in imp.ext.prebid.bidder, this as it is would throw an error. Although thinking about it maybe floors would also never be present? maybe it's a bit overprudent).

actually I wonder - if the pub already set ortb2Imp.ext.prebid.floors.floorMin, should we keep it or override it here? I think keeping it makes more sense but then that should also become the value you check against the request-wide floorMin.

I still don't understand the difference between a floor and a floorMin, so I'm not completely sure sure what makes sense here. The floors module should respect ortb2Imp.ext.prebid.floors.floorMin, but I think it may return a lower floor. Should floorMin be the pub-provided value or the actual floor in this case?

hmm, not sure. so the question is should imp.ext.prebid.floors.floorMin be set to ortb2Imp.ext.prebid.floors.floorMin (if exists) or imp.bidfloor as a fallback?

inside of the description for #8749, it says if imp[n].bidfloor exists, copy it to imp[n].ext.prebid.floors.floorMin. also states that imp[n].bidfloor is the default floor and that imp-level minFloor is not yet supported server-side. does that mean (for now at least), that the logic should be if imp[n].bidfloor exists, copy it to imp[n].ext.prebid.floors.floorMin?

maybe @bretg could provide some more clarity around this?

In my opinion if the pub has provided ortb2Imp.ext.prebid.floors.floorMin we should keep it and use that as the floorMin. But if the request-wide floor min is higher, than it should set it to the higher one.

The imp level floor mins should not overwrite the pub provided one if they are lower. Maybe I am misinterpreting though.

@dgirardi @robertrmartinez could you review when you have time? Spoke with @bretg on the issue page for this ticket (https://github.com/prebid/Prebid.js/issues/8749) and updated my PR just now.

dgirardi · 2022-09-01T15:15:59Z

Does this need to take floor currency into account? See #8938

jlquaccia · 2022-09-15T22:23:40Z

@dgirardi updated the PR to address currency as we discussed. also updated the tests on this PR

dgirardi · 2022-09-16T15:42:28Z

modules/prebidServerBidAdapter/index.js

@@ -728,6 +730,13 @@ Object.assign(ORTB2.prototype, {
      if (floor) {
        imp.bidfloor = floor.floor;
        imp.bidfloorcur = floor.currency;
+
+        const ortb2ImpFloorMin = imp.ext?.prebid?.floors?.floorMin;


this should fall back to imp.ext?.prebid?.floorMin (see #8938 (comment));

also, if we are considering the currency, this should be converted to floorMinCur before the comparison with floorMin.

thanks, just pushed a new commit with the changes

dgirardi · 2022-09-27T14:30:18Z

modules/prebidServerBidAdapter/index.js

+        const ortb2ImpFloorMin = imp.ext?.prebid?.floors?.floorMin || imp.ext?.prebid?.floorMin;
+        const lowestImpFloorMin = ortb2ImpFloorMin && ortb2ImpFloorMin < convertedFloorValue ? ortb2ImpFloorMin : convertedFloorValue;
+
+        deepSetValue(imp, 'ext.prebid.floors.floorMin', lowestImpFloorMin);


Here's a test case: you have a bidRequest with

getFloor() {return {currency: 'JPY', floor: 2}}, ortb2Imp: { ext: { prebid: { floors: { floorMinCur: 'USD', floorMin: 1 }}} }

Since 2 JPY is less than 1 USD, the expectation is that imp[].ext.prebid.floors is set to either 2 JPY, or its equivalent in USD. I think this implementation would set 1 JPY.

…floorMinCur is set

robertrmartinez

I think this is fine.

My only suggestion is that maybe add some comments somewhere describing the behavior?

Maybe this behavior is outlined in the docs, but for someone who is looking at code and does not know much about prebid + floors this may be a confusing section of code. Comments get removed when we browserify so I do not see any harm in adding some.

jlquaccia · 2022-10-03T19:11:09Z

@robertrmartinez thanks, and good call. Just added a few comments referencing the GH ticket and walking through the logic

robertrmartinez

Awesome looks great

* stronger coordination between pbs and pbjs floors * reverted changes for test page * spacing change * refactored code * reverted change to adunits html * refactored pbjs to pbs floormin logic * refactored pbjs to pbs floormin logic * removed comment * refactored code * updated logic * no longer ignoring currencies * added fallback to deprecating prebid.floorMin and repositioned where floorMinCur is set * progress * updated floorMinCur logic * reverted code on example pages * format adjustmnet * format adjustment * format adjustment * updated currency conversion logic * updated currency conversion logic * updated floorMinCur default for ortb2 * resolved conflicts * formatting * formatting * added few comments explaining the logic for this change Co-authored-by: Jason Quaccia <jasonquaccia@RWC-WS233.local>

jlquaccia mentioned this pull request Aug 18, 2022

Refine coordination of PBS+PBJS Floors #8749

Closed

dgirardi reviewed Aug 19, 2022

View reviewed changes

patmmccann changed the title ~~Issue 8749 - Refine coordination of PBS+PBJS Floors~~ Prebid Core: Refine coordination of PBS+PBJS Floors Aug 19, 2022

patmmccann requested a review from robertrmartinez August 19, 2022 16:08

patmmccann assigned dgirardi and robertrmartinez Aug 19, 2022

dgirardi reviewed Aug 19, 2022

View reviewed changes

dgirardi approved these changes Sep 1, 2022

View reviewed changes

dgirardi mentioned this pull request Sep 1, 2022

Price Foors: support imp-level floorMin #8938

Closed

patmmccann added the maintenance label Sep 7, 2022

patmmccann linked an issue Sep 7, 2022 that may be closed by this pull request

Refine coordination of PBS+PBJS Floors #8749

Closed

dgirardi reviewed Sep 16, 2022

View reviewed changes

jlquaccia force-pushed the ISSUE-8749 branch from 554ea0b to 36ebe93 Compare September 27, 2022 00:07

dgirardi reviewed Sep 27, 2022

View reviewed changes

dgirardi self-requested a review September 27, 2022 14:53

jlquaccia force-pushed the ISSUE-8749 branch from e68a368 to 4316c49 Compare September 29, 2022 20:40

Jason Quaccia added 11 commits September 29, 2022 15:02

stronger coordination between pbs and pbjs floors

f64f05a

reverted changes for test page

d7707d7

spacing change

5c30beb

refactored code

3c52473

reverted change to adunits html

cf7b992

refactored pbjs to pbs floormin logic

2aab46a

refactored pbjs to pbs floormin logic

cd53582

removed comment

42993af

refactored code

0c18833

updated logic

21dd216

no longer ignoring currencies

24583e0

Jason Quaccia added 11 commits September 29, 2022 15:03

added fallback to deprecating prebid.floorMin and repositioned where …

6bcb87f

…floorMinCur is set

progress

76d2924

updated floorMinCur logic

fc9c3fe

reverted code on example pages

54b55e7

format adjustmnet

df29ca7

format adjustment

b4b4712

format adjustment

e89357f

updated currency conversion logic

bd4f224

updated currency conversion logic

e677d74

updated floorMinCur default for ortb2

7bbc251

resolved conflicts

de6864b

jlquaccia force-pushed the ISSUE-8749 branch from 2ffd355 to de6864b Compare September 29, 2022 22:10

Jason Quaccia added 2 commits September 29, 2022 15:12

formatting

e33a67d

formatting

2a23818

dgirardi approved these changes Oct 3, 2022

View reviewed changes

robertrmartinez approved these changes Oct 3, 2022

View reviewed changes

added few comments explaining the logic for this change

c31c216

robertrmartinez approved these changes Oct 3, 2022

View reviewed changes

ChrisHuie added feature and removed maintenance labels Oct 6, 2022

ChrisHuie merged commit 17b9b32 into prebid:master Oct 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prebid Core: Refine coordination of PBS+PBJS Floors #8865

Prebid Core: Refine coordination of PBS+PBJS Floors #8865

jlquaccia commented Aug 18, 2022

dgirardi Aug 19, 2022

dgirardi Aug 19, 2022

jlquaccia Aug 19, 2022

dgirardi Aug 19, 2022 •

edited

Loading

jlquaccia Aug 19, 2022

dgirardi Aug 19, 2022 •

edited

Loading

dgirardi Aug 19, 2022 •

edited

Loading

jlquaccia Aug 19, 2022

robertrmartinez Aug 26, 2022

jlquaccia Aug 30, 2022

dgirardi commented Sep 1, 2022

jlquaccia commented Sep 15, 2022

dgirardi Sep 16, 2022 •

edited

Loading

jlquaccia Sep 16, 2022

dgirardi Sep 27, 2022

robertrmartinez left a comment

jlquaccia commented Oct 3, 2022

robertrmartinez left a comment

Prebid Core: Refine coordination of PBS+PBJS Floors #8865

Prebid Core: Refine coordination of PBS+PBJS Floors #8865

Conversation

jlquaccia commented Aug 18, 2022

Type of change

Description of change

Other information

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgirardi Aug 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgirardi Aug 19, 2022 • edited Loading

Choose a reason for hiding this comment

dgirardi Aug 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgirardi commented Sep 1, 2022

jlquaccia commented Sep 15, 2022

dgirardi Sep 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertrmartinez left a comment

Choose a reason for hiding this comment

jlquaccia commented Oct 3, 2022

robertrmartinez left a comment

Choose a reason for hiding this comment

dgirardi Aug 19, 2022 •

edited

Loading

dgirardi Aug 19, 2022 •

edited

Loading

dgirardi Aug 19, 2022 •

edited

Loading

dgirardi Sep 16, 2022 •

edited

Loading