-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
AMX RTB: remove use of auctionId #10052
Conversation
modules/amxBidAdapter.js
Outdated
const payload = { | ||
a: bidderRequest.auctionId, | ||
a: transactionId, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quick question @nickjacob ; do you plan to put this field in ortb2 bid requests to downstream parties in either source.tid or imp.ext.tid? If so, imp.ext.tid should not fall back to a value that isn't shared by all bidders. The openrtb spec doesn't allow for exchanges to make up their own value here. Perhaps you would want to pass yourself an indicator of if this value is shared or null if you can't take the null here?
modules/amxBidAdapter.js
Outdated
@@ -302,8 +303,13 @@ export const spec = { | |||
bidRequestsCount: 0, | |||
}; | |||
|
|||
const transactionId = deepAccess( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are you using this for? I can't make sense of this if payload.a
is supposed to have any meaning at all when it gets to your server:
auctionId
andtransactionId
are not meant to be mixed, picking them interchangeably means you cannot identify either the transaction nor the auction.- if you're ok with falling back to a randomly generated value, why not always use a randomly generated value? what's the reason for anything more complicated?
ortb2.ext.tid
should beortb2.source.tid
instead (if the above is not a problem).
0fc212c
to
c7f1015
Compare
@dgirardi you and @patmmccann were right -- we switched to just using generateUUID() Thanks! |
@nickjacob can you resolve conflicts? |
c7f1015
to
b184f41
Compare
Sorry about the delay -- @patmmccann fixed merge conflict |
Type of change
Bugfix
Feature
New bidder adapter
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Does this change affect user-facing APIs or examples documented on http://prebid.org?
Other
Description of change
Remove use of bidderRequest.auctionId, replacing it with ortb2 "tid" / transaction ID. For compliance with: #9781 (comment)
Other information