-
Notifications
You must be signed in to change notification settings - Fork 186
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
Core: Transition PAAPI parameters #3670
base: master
Are you sure you want to change the base?
Conversation
# Conflicts: # src/main/java/org/prebid/server/handler/SetuidHandler.java
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.
Great job!
|
||
final boolean shouldDropIgb = StringUtils.isEmpty(igi.getImpid()); | ||
if (shouldDropIgb) { | ||
conditionalLogger.warn("ExtIgi with absent impId from bidder: " + bidder, logSamplingRate); |
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.
I've noticed the requirement to emit warning in debug mode, shouldn't we add the warning messages to the response?
/** | ||
* Restores ONLY imps from rejection, rejected bids are preserved for analytics. | ||
* A bid can be rejected only once. | ||
*/ |
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.
please keep these comments)
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.
We don't comment code. Only in super tricky situations, which is not the case.
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.
Can we make this change without braking existing contract?
.fledgeAuctionConfigs(extractFledge(bidResponse)) | ||
.errors(bidderErrors) | ||
.bids(extractBids(bidRequest, bidResponse, errors)) | ||
.igi(extractIgi(bidResponse)) |
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.
I'm not sure about other exchanges' cases, but changing the existing contract of the IX bid response for PAAPI will immediately break the current PBS-Index integration. Index does not yet support the community extension for Protected Audience, which defines naming and structure differently from Google's defined format.
I assumed that any new implementation would be done separately to ensure backward compatibility with the current integration (i.e., still supporting the older format).
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.
Its not breaking change. What I have done:
- I have rewritten all bidders to supply PBS with new PA format.
- Added ability to convert new PA format to old Fledge format.
Index does not need to support new PA extensions right now. You can configure output format (old/new) and by default it will respond with old format. Check this out: prebid/prebid-server#3536 (comment)
🔧 Type of changes
✨ What's the context?
prebid/prebid-server#3536
🔎 New Bid Adapter Checklist
🧪 Test plan
Unit tests + functional tests
🏎 Quality check