Always use standard KVP.

[CarsonBanov]

For: #196

/cc @mkendall07

[mkendall07]

@CarsonBanov
Looks good, it would be nice if we issued a warning if the value is being overwritten (meaning it was set previously). Do you think you can add that?

[CarsonBanov]

@mkendall07 Added. I used utils.logMessage since the only alternatives were logError and logInfo. Can change it to either of those (there's no logWarning right?) if you prefer.

[mkendall07]

@CarsonBanov
I'm actually having trouble replicating the issue for validating the fix. I'm seeing bid keys being set correctly. Can you post a simple test that replicates the behavior?

I have 2 bidders (1,2) for 1 placement:

Bidder 1 always wins but comes in later than bidder 2. Bidder 2 has alwaysUseBid=true. The keys are being set properly in this case (hb_pb has the winning bid from 1, and a custom sendAlways key from bidder 2). This is working in master.

Want to make sure we are fixing the proper issue.

[CarsonBanov]

Try this:

• In your example, what happens if you have bidder 2 win?
• The custom sendAlways key will be set, but hb_pb will never be set! This is critical since the prebid line items are set to fire based on hb_pb, so hb_pb should always be set, right?

To show this more clearly:

• Set alwaysUseBid=true for both 1 and 2 with custom KVP sendAlways1 and sendAlways2
• Will hb_pb ever be set? As far as I can tell, this is no because they will both set the custom KVP but won't set hb_pb.

Now we can try to fix this by including hb_pb in the custom KVP:

• Set alwaysUseBid=true for both 1 and 2. Bidder 1 has custom KVP sendAlways1and hb_pb. Bidder 2 has sendAlways2 and hb_pb.
• Have bidder 2 win and then bidder 1 comes in.
• Will hb_pb be the correct value? As far as I can tell, this is no because they will both set their custom KVP, but the value for hb_pb will be from bidder 1 overwriting the higher value from bidder 2.

Does that make sense? I hope I am being clear enough.

• The standard KVP hb_pb should always hold the winning bid independent of alwaysUseBid
• The custom KVP should be sent if the bidder wins, or if alwaysUseBid=true (but hb_pb should still hold the winning bid, and you should't be setting hb_pb in the custom KVP)

[CarsonBanov]

.cc @mkendall07

[mkendall07]

@CarsonBanov Thanks - very helpful. Will try those scenarios out now.

[mkendall07]

I think the assumption was that if your using alwaysSendBid your keys will be sent the way you want. It looks like the side effect of that is that those bids get knocked out of the auction, which doesn't feel right to me. Even with this PR that's not working correctly. I can fix it by including those bids in the auction and that seems to work. I'm going to spend more time on this item because it's so core to how prebid.js works. At one point we had test cases for this but it looks like we might have lost them in a refactor. I'm going to add them back...

Changing this line to if instead of else if puts them into the auction:
https://github.com/prebid/Prebid.js/blob/master/src/prebid.js#L280

[CarsonBanov]

@mkendall07 Thanks so much!

Yeah, I'm realizing now that you mention it that this PR will include the standard hb_ KVPs along with the bid, but since that bid will in some cases be marked as alwaysUseBid it will get dropped out of buildBidResponse. I just added a commit to this PR that you can look at to see if it helps.

[mkendall07]

@CarsonBanov
Does adding those bids into the auction solve your use case without the KV changes you made? I just committed the old tests, and 2 are failing with your changes:
1aa69d2

Edit: To clarify my point, this simple test case fails with your change:
https://github.com/prebid/Prebid.js/blob/master/test/spec/bidmanager_spec.js#L103

It's failing because the defined bidder setting is using bidResponse.pbHg instead of the default of bidResponse.pbMg. So it's forcing the default over what the client configured. I don't think that's the correct behavior.

[CarsonBanov]

@mkendall07

I think that would be better. The custom KVP will be set no matter what, but the bid would still be included in the auction. Unfortunately, the "winner of the auction" will set the KVP from the bidder, so if the custom bidder wins, it will just re-set the custom KVP (and never set the hb_pb KVPs).
I think that means in all cases where we use alwaysUseBid we would need to include all of the hb_pb KVP as well since it won't inherit from standard.

For example, this config setup will never set hb_pb KVPs:

pbjs.bidderSettings =
    standard:
      adserverTargeting: [
          key: 'hb_pb'
          val: (bidResponse) ->
            prebidRoundFunction bidResponse.cpm
      ]
    # Custom brealtime
    brealtime:
      alwaysUseBid: true
      adserverTargeting: [
          key: 'custom_brt'
          val: (bidResponse) ->
            bidResponse.bidder
      ]
    # Custom appnexus
    appnexus:
      alwaysUseBid: true
      adserverTargeting: [
          key: 'custom_appnexus'
          val: (bidResponse) ->
            bidResponse.bidder
      ]

Instead, we would have to do something like this (duplicately include the hb_pb with all custom targeting):

pbjs.bidderSettings =
    standard:
      adserverTargeting: [
          key: 'hb_pb'
          val: (bidResponse) ->
            prebidRoundFunction bidResponse.cpm
      ]
    # Custom brealtime
    brealtime:
      alwaysUseBid: true
      adserverTargeting: [
          key: 'custom_brt'
          val: (bidResponse) ->
            bidResponse.bidder
          ,
          key: 'hb_pb'
          val: (bidResponse) ->
            prebidRoundFunction bidResponse.cpm
      ]
    # Custom appnexus
    appnexus:
      alwaysUseBid: true
      adserverTargeting: [
          key: 'custom_appnexus'
          val: (bidResponse) ->
            bidResponse.bidder
          ,
          key: 'hb_pb'
          val: (bidResponse) ->
            prebidRoundFunction bidResponse.cpm
      ]

[CarsonBanov]

Another thing is that the hb_pb KVP will be set by all of those alwaysUseBid partners every bid which means we are relying on the line pb_targetingMap[adUnitCode] = utils.extend(pb_targetingMap[adUnitCode], keyValues); to overwrite them for the winner of var winningBid = getWinningBid(bidArrayTargeting);

[mkendall07]

I see your what your after, I think it's creating other side affects though, like I pointed out above with using default settings instead of what is configured. We'll need to fix this properly to support all use cases.

[CarsonBanov]

tl;dr

• Adding alwaysUseBid bidders' bids into the auction will help.
• With that change alone we will need to duplicately include hb_pb in all alwaysUseBid custom adserverTargeting declarations, and the hb_pb will be overwritten many times over the course of the auction because of it. But overall I this behavior should work for us.
• Always setting the hb_ KVPs would help more by making the hb_ KVPs more independent of the custom adserverTargeting. But this isn't extremely necessary I guess.

[CarsonBanov]

@mkendall07 Thanks for the attention. I guess alwaysUseBid is supposed to be more for targeting a different set of line items like you outlined above (always set those custom KVP) rather than my particular use case.

I wonder if an orthogonal solution would be to add a new feature very similar to bidCpmAdjustment called something like kvpAdjustment which would let us set a custom KVP per partner totally unrelated to hb_pb, custom targeting, and the prebid internal auction.

Anyways, thanks for your help. Feel free to close this and implement whatever fix you feel fit.

[mkendall07]

@CarsonBanov
I'm wondering now if we switched the order to always apply default settings for KVP and then apply the custom settings if that would solve all use cases (along with adding all bids into the auction)... will play with that and create a new PR for you to look at.

[mkendall07]

Following up with #295