Openx bid adapter: support for MAP requests#14477
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the OpenX bid adapter to consolidate multiple bid requests into a single HTTP request, improving performance by reducing network overhead. Previously, the adapter would send separate HTTP requests for video bids versus banner/native bids. With this change, all bid requests are combined into a single HTTP request with multiple impressions.
Changes:
- Simplified request building logic to create one HTTP request for all bid types
- Removed mediaType context parameter to allow ortbConverter to generate all media type objects in impressions
- Updated test expectations to validate single request with multiple impressions
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| modules/openxBidAdapter.js | Simplified buildRequests to return single consolidated request; removed helper functions for bid type detection and mediaType parameter |
| test/spec/modules/openxBidAdapter_spec.js | Updated tests to expect 1 request with multiple impressions instead of multiple requests; added mtype field to video response fixtures; added new tests for consolidated request behavior |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 719568da26
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
patmmccann
changed the title
marcin-wrobel-ox
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 82ff0154de
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| function isBannerBidRequest(bidRequest) { | ||
| const isNotVideoOrNativeBid = !isVideoBidRequest(bidRequest) && !isNativeBidRequest(bidRequest) | ||
| return utils.deepAccess(bidRequest, 'mediaTypes.banner') || isNotVideoOrNativeBid; | ||
| data: converter.toORTB({bidRequests, bidderRequest}) |
There was a problem hiding this comment.
Calling converter.toORTB without a context.mediaType makes every getFloor() lookup run with mediaType='*' via setOrtbImpBidFloor (modules/priceFloors.ts), so video-only OpenX bids no longer receive video-specific floors and instead use wildcard floors. Before this change, video requests were built with context.mediaType = 'video', so this is a regression whenever Floors is enabled and publishers define different banner/video floor values, and it can change bid acceptance and auction results.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This one I'm not sure if it's a problem or not. I would assume that it should work regardless, but if someone has some insights into this I would appreciate their opinion.
There was a problem hiding this comment.
The problem is imp.bidfloor can only be one value, so for multi-format video requests if the priceFloors mod has different floor for banner vs video, one can only be chosen.
Usually a floor provider may have something like
banner: 0.4,
video: 1.1,
*: 0.5
And by default I believe your imp.bidfloor would be the * rule 0.5
The price floors module now sends the "per mediaType floors"
imp.banner.ext.bidfloor
imp.video.ext.bidfloor
So you would get to your exchange:
{
"imp": [{
"bidfloor": 0.5,
"bidfloorcur": "USD",
"banner": {
"ext": {
"bidfloor": 0.4,
"bidfloorcur": "USD"
}
},
"video": {
"ext": {
"bidfloor": 1.1,
"bidfloorcur": "USD"
}
}
}]
}Of course your exchange (or downstream dsps if you pass through) would need to have logic to do what they want.
When setting imp.bidfloor I have seen bidders do any of the following
- let this default happen
- prefer banner if multiformat
- some pick lowest of two format floors
So its up to your team on what makes most sense!
Before this change you were getting two HTTP calls to your exchange, one for banner one for video, where imp.bidfloor was the specific one for that request.
There was a problem hiding this comment.
Thanks for great explanation @robertrmartinez :). I will talk with the team about this case.
There was a problem hiding this comment.
Sorry for the late reply, I was out sick.
So after some internal discussion, we agreed that this is not a problem for us.
Pull Request Test Coverage Report for Build 23344858755Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
|
I believe this PR is ready for review. I don't know if failing Notify Code Path Changes / notify (pull_request_target) check blocks this PR in any way. |
|
@bukrain emailing you guys that someone is changing your adapter failing does not block merge; you guys are pr'ing yourself |
Thanks @patmmccann that does make sense. |
|
@codex review again |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 65de0ad2a9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| function isBannerBidRequest(bidRequest) { | ||
| const isNotVideoOrNativeBid = !isVideoBidRequest(bidRequest) && !isNativeBidRequest(bidRequest) | ||
| return utils.deepAccess(bidRequest, 'mediaTypes.banner') || isNotVideoOrNativeBid; | ||
| data: converter.toORTB({ bidRequests, bidderRequest }) |
There was a problem hiding this comment.
This change removed the per-request media type context that was previously passed for video requests, so fromORTB now depends on seatbid.bid[].mtype to classify video bids. In setResponseMediaType, missing mtype with no context.mediaType throws and the bid is skipped, which regresses compatibility with video responses that omit ORTB 2.6 mtype. Keep a media-type fallback (or infer from the matched imp) to avoid dropping valid video bids.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@bukrain is mtype always there? i see it in this response
but do you want to add a fallback inference?
There was a problem hiding this comment.
Sorry for late reply. We always return mtype in our bids so this should be ok.
Type of change
Bugfix
Feature
New bidder adapter
Updated 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
Other information