Prebid.js icon indicating copy to clipboard operation
Prebid.js copied to clipboard

Prebid Server Adapter: use bidRequest[0].transactionid as source.tid

Open patmmccann opened this issue 3 years ago • 10 comments

Related:

https://github.com/prebid/Prebid.js/pull/7585 https://github.com/prebid/Prebid.js/issues/8573 https://github.com/prebid/Prebid.js/pull/3190

7585 Made a new uuid for source.tid for prebid server because auction id was flawed. The first transaction id seems more appropriate, as other adapters can refer to it for source.tid as well. imp.ext.tid will be preferred by buyers in the long term, but source.tid has remaining interim utility and we want to unite their definitions

patmmccann avatar Jun 23 '22 19:06 patmmccann

@bretg if PBS is to get a source.tid, 7585 and the comments in the source say it cannot be auctionId, this only leaves transactionid for the request[0]. It seems we should standardize on that.

patmmccann avatar Jun 23 '22 19:06 patmmccann

There is a strong case to make that if there is more than one impression in the request, source.tid should just be blank. If I changed it to that, Would that break anything @robertrmartinez ?

patmmccann avatar Jun 23 '22 20:06 patmmccann

It should just be left blank? I do not see how that could be valid if more than one impression is there.

I still think auctionId is the right approach, even with its downfalls.

From DSP perspective (Which I admit I am not knowledgeable about them much)

I would want to know an auctionId so I can say how many people are asking me for bids for a single auciton

As well as how many transactionId's came from that auction.

Either way, I still do not love the idea of setting sourct.tid to the first transactionId? They are different.

Maybe we should talk this out more but I like the idea of source.tid being the aucitonId, and if it is not a UUID (maybe we just check length of the string) then we generate one?

@bretg obvioulsy understands PBS more than I do. But from an Analytics perspective I would think having source.tid and imp[x].tid different is important.

robertrmartinez avatar Jun 24 '22 19:06 robertrmartinez

@robertrmartinez reopening as consensus seems to be building this is valuable (eg https://github.com/prebid/Prebid.js/pull/8679).

Is blank even an option? Does pbs use this id for things other than shoving into source.tid ? If not, blank seems great

I certainly understand the commentary that this id does not make sense in a multi-imp request, but when prebid server is making up its own id, it risks acting as a malicious entity, seemingly communicating these are new avails to downstream entitites, when in fact they are identical avails.

Source.tid seems infinitely inferior to imp.ext.tid, but PRs to lower its cardinality to downstream entitites seem to be worthwhile.

patmmccann avatar Jul 26 '22 13:07 patmmccann

Does pbs use this id for things other than shoving into source.tid ?

No - source.tid is just passed through to bidders.

when prebid server is making up its own id

PBS does not make up any IDs other than a bid response ID, and that's placed in seatbid.bid.ext.prebid.bidid. And that's only because we found that quite a few bidders return non-unique bid IDs that are unusable for tying auction events to impression events.

I don't understand why having an auction-level ID isn't helpful. PBJS creates an auction-level ID that would align quite nicely with source.tid.

bretg avatar Jul 26 '22 16:07 bretg

PBS does not make up any IDs other than a bid response ID,

I misspoke, the prebid server adapter makes up its own one, not PBS itself. This pr changes that and if it changed it to auctionid, it would just be a revert of 7585, which isn't desirable for the same reasons 7585 was written to address.

why having an auction-level ID isn't helpful.

It is, and bidRequests[0].transactionId is one and so is auctionID. The problem is that auctionid can be overridden and we want a value that is always unique, which is why this line was written in #7585

I cannot follow why having an auction id that doesn't collide with transaction id is useful. It actually seems to be a degradation of functionality from an analytics perspective.

patmmccann avatar Jul 26 '22 17:07 patmmccann

@patmmccann - I think I hear you saying that you want the pbsBidAdapter to accept whatever auctionId the pub defines, even if crappy. I'm ok with that.

bretg avatar Jul 27 '22 15:07 bretg

I'm confused, if that is the case, why was #7585 implemented?

patmmccann avatar Jul 27 '22 15:07 patmmccann

We debated what to do:

  • some kind of validation (e.g. > 12 chars) // any validation rule is going to make someone unhappy
  • just let it be crappy // I've been burned by entities who do a bad job at creating IDs
  • generate a proper UUID

There was no input about which way to go from the community. I'm willing to re-evaluate and admit having made the wrong choice.

bretg avatar Jul 27 '22 21:07 bretg

what about a fourth option, if anyone has overriden the auctionid that was generated, set it to blank?

patmmccann avatar Aug 02 '22 17:08 patmmccann

Another, simpler proposal comes to mind: always set tid zero to auctionid.

patmmccann avatar Aug 17 '22 00:08 patmmccann

@robertrmartinez I changed this to auction id

patmmccann avatar Sep 07 '22 17:09 patmmccann

Ok, I am good with this, with the understanding that if a pub does:

pbjs.requestBids({
   auctionId: 'bobby',
});

or

pbjs.requestBids({
   auctionId: ${some very small random id which will have many clashes},
});

that this ends up sucking for downstream entities.

But I consider this an implementation mistake and should be fixed by the pub to generate a true UUID or just let Prebid do its thing!

What do you all think?

@patmmccann @bretg @dgirardi

robertrmartinez avatar Oct 03 '22 20:10 robertrmartinez

On low entropy ids: because no pubs should have a reason to do that, and I'd expect it to be rare, I think it's fine. But at the same time I'm certain that whatever the goal of that feature is, there is some other way of achieving it that does not need to touch ids. I don't know the history though.

dgirardi avatar Oct 03 '22 22:10 dgirardi

this work will be incorporated into solution to #8573 pr

patmmccann avatar Oct 04 '22 18:10 patmmccann