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

Prebid Server Adapter: use bidRequest.auctionid as source.tid #8608

Closed
wants to merge 2 commits into from

Conversation

patmmccann
Copy link
Collaborator

Related:

#7585
#8573
#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
Copy link
Collaborator Author

@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
Copy link
Collaborator Author

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 ?

@robertrmartinez
Copy link
Collaborator

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.

@patmmccann patmmccann closed this Jun 26, 2022
@patmmccann patmmccann reopened this Jul 26, 2022
@patmmccann
Copy link
Collaborator Author

patmmccann commented Jul 26, 2022

@robertrmartinez reopening as consensus seems to be building this is valuable (eg #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.

@bretg
Copy link
Collaborator

bretg commented Jul 26, 2022

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.

@patmmccann
Copy link
Collaborator Author

patmmccann commented Jul 26, 2022

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.

@bretg
Copy link
Collaborator

bretg commented Jul 27, 2022

@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.

@patmmccann
Copy link
Collaborator Author

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

@bretg
Copy link
Collaborator

bretg commented Jul 27, 2022

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.

@patmmccann
Copy link
Collaborator Author

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

@patmmccann
Copy link
Collaborator Author

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

@jbartek25 jbartek25 mentioned this pull request Aug 24, 2022
1 task
@patmmccann patmmccann changed the title Prebid Server Adapter: use bidRequest[0].transactionid as source.tid Prebid Server Adapter: use bidRequest.auctionid as source.tid Sep 7, 2022
@patmmccann
Copy link
Collaborator Author

@robertrmartinez I changed this to auction id

@robertrmartinez
Copy link
Collaborator

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

@@ -341,7 +341,7 @@ adapterManager.callBids = (adUnits, bidRequests, addBidResponse, doneCb, request
let counter = 0;

// $.source.tid MUST be a unique UUID and also THE SAME between all PBS Requests for a given Auction
const sourceTid = generateUUID();
const sourceTid = bidRequests.auctionId;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this works? bidRequests is an array here.

@dgirardi
Copy link
Collaborator

dgirardi commented Oct 3, 2022

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.

@patmmccann
Copy link
Collaborator Author

this work will be incorporated into solution to #8573 pr

@patmmccann patmmccann closed this Oct 4, 2022
@patmmccann patmmccann deleted the use-tid-zero-as-tid branch October 4, 2022 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants