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.

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

Deal specific line items don't work #3899

Closed
benjaminclot opened this issue Jun 12, 2019 · 24 comments
Closed

Deal specific line items don't work #3899

benjaminclot opened this issue Jun 12, 2019 · 24 comments
Labels
feature pinned won't be closed by stalebot

Comments

@benjaminclot
Copy link
Contributor

Type of issue

Deals don't work as advertised in the docs.

Description

When creating a deal specific line item, that LI wins but the Universal Creative code doesn't support it and only uses hb_adid which may not be the deal's ad ID.
My guess is that it's not possible to know why GAM elected a LI to win with macros, so either the creative template or the UC code needs to be changed? Also, the deal's ad ID, if it didn't win, doesn't seem to be sent in the key-values with enableSendAllBids: false.

Other information

This has already been reported (and ignored 😞) in #3266 back in november.

@bretg
Copy link
Collaborator

bretg commented Jun 13, 2019

Sorry for the silence on this issue @benjaminclot. The answer in #3266 is close to correct, but it's not quite as bad as it sounds:

this means we'll need to create a unique line item and unique creative for every deal, which seems long-winded.

Really it's just a unique line item and creative per bidder, not for every deal. e.g. targeting could be to hb_deal_BIDDERCODE in (1111,2222,3333).

I've updated the deals page on prebid.org -- http://prebid.org/adops/deals.html#step-4-attach-creatives-to-line-items

Basically, the line item targeting has to match what the creative sends back to Prebid.js -- so if the line item is targeted to hb_deal_BIDDERCODE, then the creative needs to send back hb_adid_BIDDERCODE. If you see a way around this, happy to take suggestions.

I see that the last screenshot on the deals page needs to be changed -- not recommended to target different BIDDERCODEs in the same line item because of the adid issue.

@benjaminclot
Copy link
Contributor Author

benjaminclot commented Jun 13, 2019

@bretg as much as I do agree with your statement and solution, this doesn't work with enableSendAllBids: false as the adid for the deal is not being sent as far as I can tell.

@bretg
Copy link
Collaborator

bretg commented Jun 17, 2019

Not quite following @benjaminclot.

Also, the deal's ad ID, if it didn't win, doesn't seem to be sent in the key-values with enableSendAllBids: false.

  1. enableSendAllBids was built for deals. hb_adid is always for the bid that had the highest CPM. If the deal wasn't the highest bid, it will still be sent as hb_deal_BIDDER and hb_adid_BIDDER.
  2. line items in the ad server should target to hb_deal_BIDDER. The creatives would refer to hb_adid_BIDDER
  3. I verified the behavior here works as expected in the enableSendAllBids: false scenario. hb_deal and hb_adid are sent when the deal wins.

@benjaminclot
Copy link
Contributor Author

benjaminclot commented Jun 18, 2019

I understand where my explanations might not be clear @bretg so let me try again.

Take these bid responses:

appnexus
  bid: 1.00
  deal ID: 12345
  ad ID: abcde

rubicon
  bid: 1.10
  no deal
  ad ID: fghij

When enableSendAllBids: true, we get:

hb_pb_appnexus = 1.00
hb_deal_appnexus = 12345
hb_adid_appnexus = abcde
hb_pb_rubicon = 1.10
hb_adid_rubicon = fghij
hb_pb = 1.10
hb_bidder = rubicon
hb_adid = fghij

Whereas with enableSendAllBids: false, we get:

hb_deal_appnexus = 12345
hb_pb = 1.10
hb_bidder = rubicon
hb_adid = fghij

What I'm trying to convey here is that, while the docs state that all deal IDs are being sent (even for losing bids), their other properties (ad ID, cpm, etc.) are not. Thus, it is not possible to make them win in a deal specific line item because we don't get (at least) their ad ID; so the creative template will still render the winning bid.

Maybe a solution would be to get something like this with enableSendAllBids: false:

hb_deal_appnexus = 12345
hb_deal_appnexus_pb = 1.00
hb_deal_appnexus_adid = abcde
hb_pb = 1.10
hb_bidder = rubicon
hb_adid = fghij

On a side note, I'm also wondering how is handled the case when a bidder has multiple bid responses, since there seems to be only one hb_KEY_BIDDER 🤔

@bretg
Copy link
Collaborator

bretg commented Jun 19, 2019

Thanks for the explanation -- we'll discuss what to do about this: understood that targeting values come in sets, and if we're only providing one element of the set, that's bad.

how is handled the case when a bidder has multiple bid responses, since there seems to be only one hb_KEY_BIDDER

Only the highest bid from each bidder is considered.

@bretg
Copy link
Collaborator

bretg commented Jun 20, 2019

We discussed in the meeting today and resolved:

  1. We'd like to address this, but we don't want to make any breaking changes until the next major release.
  2. This topic is related to the general recent concern around how many ad targeting parameters are being sent to the ad server.
  3. We want to require a config change that will be needed to change the current behavior.

Proposed config:

pbjs.setConfig({
  enableSendAllBids: false,  // really enableSendAllOpenMarketBids
  sendBidsControl: {
    bidLimit: 5    // how many overall bids are allowed
  }
}]

If both enableSendAllBids and sendBidsControl are specified, then the top N deals will be sent with all of their attendant targeting variables.

Will open a separate issue to discuss how to customize which bids are sent.

@mkendall07
Copy link
Member

mkendall07 commented Jun 20, 2019

Hey @bretg . We already have a targetingControls attribute: http://prebid.org/dev-docs/publisher-api-reference.html#setConfig-targetingControls

  targetingControls: {
    auctionKeyMaxChars: 5000
  }
});

Should we put the bidLimit there?

Also, I'm not clear if you are saying bidLimit would operate independently of enableSendAllBids or not.

@benjaminclot
Copy link
Contributor Author

benjaminclot commented Jun 21, 2019

As a temporary fix, I've enabled enableSendAllBids: true for now and added some new creatives like @bretg suggested. Lots of key-values but that's better than nothing.

Talking about the future, I think the goal for deals should be to get them all. In the case of deals, you can't let Prebid decide: the ad server should get all the info it needs to make a decision and cpms are (often? sometimes?) meaningless. That means getting all the deals and their associated metadata, whatever their cpm and even if there are multiple responses for the same bidder.
Then comes the Universal Creative problem, which I think the aforementioned solution solves.

@stale
Copy link

stale bot commented Jul 5, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jul 5, 2019
@bretg bretg removed the stale label Jul 5, 2019
@bretg
Copy link
Collaborator

bretg commented Jul 5, 2019

Following up and referencing #3906

I think what we're facing here is the general desire for more flexibility in deciding which bids go to the ad server. In some cases there are too many KVPs, so there's a desire to control how many are going out.

But previous efforts to limit the KVPs haven't provided a lot of flexibility in controlling which go out. One option would be to just provide an easy-to-grok example of writing your own setTargetingForGPT function.

Another option would be the ability for the publisher to supply a function that sorts the bids array. e.g.

sendBidsControl: {
    sortBidsFunction: fn,
    bidLimit: 5
}

@benjaminclot
Copy link
Contributor Author

When using an ad server, I'm not a big fan of having a client-side function as It prevents adops from doing their job because they don't have access to the Prebid config... and don't know how to code. Also, it's not good for maintainability -- in the case of this issue, editing the function each time you get a new "win-them-all" deal would quickly become a nightmare.
Priority rules are the ad server's job. I believe Prebid's job is to give the ad server all it needs to make the right decision, not to do its job for it.

@mkendall07
Copy link
Member

@benjaminclot I generally agree with you, however, if prebid sets too many keys than it completely breaks the ad server call. So some control is needed, to prevent catastrophic failure.

@benjaminclot
Copy link
Contributor Author

benjaminclot commented Jul 9, 2019

@mkendall07 yes, of course. My proposal was that when enableSendAllBid is set to false, only top bids are sent along with all bids containing a deal ID (that means every key/value for these specific deals).
The general consensus here is that generally the top bid should be the winning bid, so we don't really need the other bids, except when you have to deal with... well... deals 🙂

@bretg
Copy link
Collaborator

bretg commented Jul 20, 2019

@mkendall07 - the auctionKeyMaxChars is a sub-optimal solution because it can break up bids. i.e. it might have hb_pb_bidderA, but then run out of chars and not add hb_adid_bidderA.

IMO, either we need to fix auctionKeyMaxChars to keep all bid attributes together or add the bidLimit option.

And I'm ok with what @benjaminclot suggests here and adding deal KVPs for enableSendAllBids=false.

Not sure I agree with the concern that adding a sort function would make AdOps' jobs harder -- such a function could be packaged up with Prebid.js so could have parameters tweaked by the PBJS packaging process. But in any case, the other solutions are fine.

@bretg
Copy link
Collaborator

bretg commented Jul 28, 2019

I've opened an internal Rubicon ticket to add the deal KVPs on enableSendAllBids=false in our next sprint starting Aug 7

@bretg
Copy link
Collaborator

bretg commented Aug 21, 2019

Update: this was pushed out of last sprint, but is in our current sprint

@bretg
Copy link
Collaborator

bretg commented Aug 27, 2019

I know we discussed this in the Prebid.js meeting, but I couldn't find the results of the conversation. We're building this now and are concerned about the change the behavior of the existing system without adding a new flag.

So that's what we propose:

enableSendAllBids: false,
targetingControls: {
    alwaysIncludeDeals: true   // defaults to false
}

@benjaminclot
Copy link
Contributor Author

This seems like a good option to me.

@robertrmartinez
Copy link
Collaborator

Have a PR open and waiting for review.

#4136

@benjaminclot
Copy link
Contributor Author

That's great! Now all that's missing is support of this by the Universal Creative...

@bretg
Copy link
Collaborator

bretg commented Sep 23, 2019

@benjaminclot - I'm not seeing a Prebid Universal Creative issue.

LI wins but the Universal Creative code doesn't support it and only uses hb_adid
which may not be the deal's ad ID.

As noted in the comment way above, I updated the documentation to reflect how deals need to be set up. You're right that won't work without hb_adid_BIDDERCODE, but the alwaysIncludeDeals flag should fix that.

Note also that I submitted a docs PR to revise the enableSendAll bids section

prebid/prebid.github.io#1507

@benjaminclot
Copy link
Contributor Author

OK this is good @bretg. Thanks!

@bretg
Copy link
Collaborator

bretg commented Sep 25, 2019

Cool. So we can close this @benjaminclot ?

@benjaminclot
Copy link
Contributor Author

Yes, I'll close it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature pinned won't be closed by stalebot
Projects
None yet
Development

No branches or pull requests

4 participants