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

adId when set by bid adapters in the response object must be unique #6381

Closed
bretg opened this issue Mar 2, 2021 · 14 comments
Closed

adId when set by bid adapters in the response object must be unique #6381

bretg opened this issue Mar 2, 2021 · 14 comments
Assignees
Labels

Comments

@bretg
Copy link
Collaborator

bretg commented Mar 2, 2021

Type of issue

bug

Description

As discovered in #6329, at least 12 bid adapters are setting the adId on the bid response. This isn't necessary, and in fact has some downsides.

Rather than trying to coordinate fixes in 12 bid adapters, I propose updating PBJS-core to move the initialization of bidResponse.adId to just overwrite whatever the bid adapter responds with.

I'd argue this isn't a breaking change because adId is a PBJS-only concept and it's currently being mis-used as evidenced by #6329.

Besides this PBJS-core change, we ought to update the Bid Adapter documentation to make it clear that adId is not something adapters should set.

@bretg bretg added the bug label Mar 2, 2021
@AskRupert-DM
Copy link
Contributor

hey @bretg this would potentially be a breaking change for our bid adapter (ozone)

we append an identifier on to the end of an adId for each bid we return back for an impression object so each bid can be uniquely identified that we return for an impression object. (For background our servers return all bids back on the client that we received from our server-side auctions).

sb.bid[j]['adId'] = ${sb.bid[j]['impid']}-${i}-${j}

Our publishers don't necessarily always select the winning/highest priced bid to render/win in GAM and therefore will prioritise certain bids from specific bidders and use the adId to allow GAM to pick/render the correct bid for that bidder regardless of it being the highest priced bid for that impression object.

If our adapter is prevented from doing this - it will potentially result in our publishers GAM instances rendering a bid from a different bidder their GAM instances would've intended to select.

Unless there a different method/way bid adapters should be using to expose all bids and making each of them available for GAM to render/pick - I imagine the other 11/12 bid adapters that are doing this will be doing so for a similar reason and they'd probably face the same implications/issues should this be handled via pbjs-core without any consultation/notice ...

@bretg
Copy link
Collaborator Author

bretg commented Mar 5, 2021

If our adapter is prevented from doing this - it will potentially result in our publishers GAM instances rendering
a bid from a different bidder their GAM instances would've intended to select.

This isn't going to happen @AskRupert-DM - each and every element added to the bid response array gets a unique adId. Try it. Hack up a version of your adapter that doesn't set adId, set a breakpoint, and you'll see that each one gets assigned its own adId. I've tried this recently. But if it doesn't in your case, please tell us what you did in enough detail we can replicate.

So uniqueness is not a reason to specify adId. If you've built logic around adId including the impId, then that would indeed prevent us from simply ignoring adId.

And I suppose that the existence of this very concept means that we're going to need to solve this problem by informing each of the adapters of the potential problem:

  1. It was not intended for bid adapters to set adId.
  2. If you are setting adId, it must be unique for each and every bid response
  3. If it's not unique, you may inadvertently trample on publisher's auctions

Attention:

a4g - @Junus , @adilets
audiencerun - @audiencerun
criteo - @allanjun
improvedigital - @jbartek25 , @agregorio-improve
papyrus - @HolodovAlexander
platformio - @varashellov
pulsepoint - @anand-venkatraman
Somo - @SuprPhatAnon
Synacormedia - @rajcspsg , @slaufer
Telaria - @telariaEng
windtalker - @degroat

@bretg bretg changed the title Ignore adId as set by bid adapters in the response object adId when set by bid adapters in the response object must be unique Mar 5, 2021
@AskRupert-DM
Copy link
Contributor

AskRupert-DM commented Mar 8, 2021

hey @bretg - I setup a quick test page as suggested - https://bit.ly/2MVUz7K - and can confirm you are indeed correct/right with regards to unique adIds being set by prebid core.

However, it looks like this is done by prebid core a little bit late so the Ids aren't available to the bid-adapters themselves, which for the use case of my bid-adapter causes an issue as we try to set bespoke key-values (specific to my adapter) for each bid we've received/returned on the client of which one is an adID specific key (so the publishers GAM instance can correctly render that bid when / if it selects it).

It looks like prebid core is creating/generating the adIds after bid-adapters have done their thing / after my bid adapter has finished interpreting the server response.

I recall us having a similar conversation about this almost 2 years back - prebid/prebid-server#860 - in short I ended up following @jsnellbaker suggestion there on creating these so they'd be available to my bid adapter to use.

I can confirm the adId our adapter sets is it unique for each and every bid response (& to ensure this is the case can add some additional characters/logic to ensure its always bespoke to the bids our bid adapter returns).

@spormeon
Copy link

how long will the consultation/ notice period be on this?, I see from #6329 aol managed to make appropriate changes to their adapter within 2 days and the fixes went out in V 4.30.0

@bretg
Copy link
Collaborator Author

bretg commented Mar 11, 2021

Ok thanks @AskRupert-DM - so what adapter are you vouching for?

@spormeon - not sure what you're asking for here. This matter is out of Prebid's hands... as indicated by the exceptions noted above, the PBJS-core team cannot simply go in and remove the adid as set by these adapters. Each of them has to respond individually. If there are a set who don't respond, we could put a warning on their adapter page noting that there's an open question about their setting of adid.

@spormeon
Copy link

this is what I'm wondering about, If it can be "overwritten", you can just place a timeframe on it, so others can consult/ have notice of? No ones responded within a week, so cant be that worried/ bothered about it? "Rather than trying to coordinate fixes in 12 bid adapters, I propose updating PBJS-core to move the initialization of bidResponse.adId to just overwrite whatever the bid adapter responds with"

@bretg
Copy link
Collaborator Author

bretg commented Mar 11, 2021

Given what a couple of bidders have said above, PBJS-core will not be able to overwrite adid. There are scenarios where that would break those adapters.

My original concept(hope) that adid was internal to Prebid is false --apparently that cow left the barn years ago. So now we're stuck with asking bid adapters to do the right thing. Otherwise it's simply a bug on their part. Bugs exist. My view is that Prebid's role here is to make sure the adapters know about the bug/potential bug and confirm/fix.

If these entities don't confirm by the end of March, I will flag their bidder documentation with a notice. I've created a calendar notice for myself to do so.

@bretg
Copy link
Collaborator Author

bretg commented Mar 31, 2021

We only had two adapters make the updates (thanks a4g and improvedDigital)

Adapters that have confirmed they need to set adid on their own for various reasons:

Adapters that haven't responded:

I've opened prebid/prebid.github.io#2818 with the disclosure:

This bidder sets adId on the bid response and hasn't responded to the Prebid.js team toconfirm uniqueness
of this value. See Issue 6381.

Bidders have another week to respond and either remove the adId or confirm that they absolutely must set it and they've confirmed it's always unique.

@bretg bretg mentioned this issue Mar 31, 2021
1 task
@rajcspsg
Copy link
Contributor

rajcspsg commented Apr 2, 2021

@bretg We are not using it. I will remove setting the value for the same.

@spormeon
Copy link

spormeon commented Apr 8, 2021

any reason why the disclosures didn't go out in yesterdays 4.34.0 ?

@patmmccann
Copy link
Collaborator

Docs commits are not compiled into versions

@gglas
Copy link

gglas commented Apr 12, 2021

@bretg @patmmccann are we going to take any further actions besides the docs, or should we consider this addressed?

@bretg
Copy link
Collaborator Author

bretg commented Apr 12, 2021

Disclosure added. Closing this issue.

@anand-venkatraman
Copy link
Contributor

Not sure why I didnt get a notification on this, apologies for the late response. Explicit setting of adId has been removed effective #Prebid. Removing the disclosure from our bidder documentation page, in the subsequent PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants