-
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
OpenX Video Adapter update to Prebid v1.0 #1724
Conversation
hey @IHateCodesss ... did you plan on keeping the openx In Prebid adapters can support several media types, so there's only one needed for each company. |
Unfortunately, Yes, OpenX does not support multiple media types in a single request for now. We have different endpoints for different media type ad requests. |
to be clear, you could add logic that changes the endpoint to hit based on the media type. That might be easier to maintain than 2 separate adapters. |
The |
modules/openxBidAdapter.js
Outdated
const parts = oxResponseObj.match(/^[a-z0-9]+\(([^\)]+)\)/i); | ||
let response = !parts || parts.length != 2 ? undefined : JSON.parse(parts[1]); | ||
if (response && response.pixels) { | ||
userSync.registerSync('iframe', 'openx', response.pixels); |
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.
1.0 adapters shouldn't be doing this. User syncs are fired after the auction has completed.
Implement getUserSyncs()
on your spec instead: https://github.com/prebid/Prebid.js/blob/master/src/adapters/bidderFactory.js#L50
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.
Unfortunately, we do not have user sync pixel URLs available until after the bidResponse, which is why we're registering it as part of handling the response. The official doc here: http://prebid.org/dev-docs/bidder-adapter-1.html#register-user-syncs seems to say that we can do it either way. Has this been changed?
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 think this is OK. We'll need to check and see if this queue get's emptied if it's after auction close.
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.
akh... I had a brain fart.
In interpretResponse
calls createBidResponses
, which calls buildBoPixel
, which creates an Image
and sets the src
property--in effect, firing off a user sync in the middle of the auction.
I saw that, but must have lost my head and commented on the wrong line.
modules/openxBidAdapter.js
Outdated
return !!(bid.params.unit || bid.params.delDomain); | ||
}, | ||
buildRequests: function(bids) { | ||
let isIfr; |
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.
There's a utils.inIframe()
for this.
modules/openxBidAdapter.js
Outdated
let adUnit = adUnits[i]; | ||
let bidResponse = {}; | ||
bidResponse.requestId = bids[i].bidId; | ||
bidResponse.bidderCode = BIDDER_CODE; |
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.
This... will actually break aliasing, in the bidderFactory
.
I think our docs are misleading. Bidders don't need to send back bidderCode
, because the bidderFactory
does it for you. So you can remove this line.
modules/openxBidAdapter.js
Outdated
beaconParams.bp = adUnit.pub_rev; | ||
beaconParams.ts = adUnit.ts; | ||
if (shouldSendBoPixel) { | ||
buildBoPixel(adUnit.creative[0], beaconParams); |
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.
Same thing here... pixel syncs aren't allowed during the actual auction. Implement this in getUserSyncs
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.
This is for performance measurement and is not related to user syncing. Do you have other alternatives for us to report on performance measurements?
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.
Not sure what you mean by "performance measurement." There are three mechanisms I've seen people do, depending on the case.
- Put the image inside the creative. The URL will get hit if it wins.
- Make an analytics adapter.
- Use
userSync
to call the URL after the auction has finished.
Yours is the first adapter I've seen doing this... so I checked in with @mkendall07 for the rules. The suggestion was to just use userSync
anyway here, even though it's not exactly what you're doing.
One more change, now that #1742 got merged. The first argument to interpretResponse now looks like this:
You'll have to update the spec so that it looks through that object as well now. |
@dbemiller Thanks for the review. We'd like to get this PR in first: #1714. This one (#1724) will be the next for this release also. I will keep updating this PR according to your comments and the update in this PR #1714 |
Ok. I'll tag this PR as in progress until #1714 goes in. |
Thanks very much! |
ok... I just heard that your first one got merged in. So I can review this once the conflicts are fixed. |
768f8a2
to
0038e8e
Compare
@dbemiller Thanks for ur help. Just update the PR with no conflict now. |
c25ceb5
to
68e299e
Compare
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 getting any bids back, using your video params from the .md
file.
The terminal says that the server call is returning a 400.
}); | ||
|
||
it('handles nobid responses', () => { | ||
bidResponse = {'cache_key': '', 'pub_rev': '', 'per_colo_domain': '', 'ph': ''}; |
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.
Tests shouldn't have side-effects
@dbemiller I just update the unit tests to avoid side-effects and the ad domain in .md file |
@IHateCodesss ok, tests look fine now and I'm definitely getting a response back from the server. ...but it doesn't look like it's a video bid? The This is what makes it into the |
@dbemiller I just fix this. BTW, are 'width' and 'height' required in response? When I did test for undefined width and height, ad delivery still worked and I saw ad rendered |
02f7b59
to
f17206f
Compare
f17206f
to
73e14eb
Compare
No worries there. Video bids should be fine without a width & height. |
@@ -13,6 +13,11 @@ export const spec = { | |||
code: BIDDER_CODE, | |||
supportedMediaTypes: SUPPORTED_AD_TYPES, | |||
isBidRequestValid: function(bid) { | |||
if (bid.mediaType === VIDEO) { |
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.
It's worth noting that there is another way to define a video bid now. I'm not going to hold up this PR though since it's been sitting for awhile. You can see the pattern here: https://github.com/prebid/Prebid.js/blob/master/modules/appnexusAstBidAdapter.js#L291
If you want, please open a new PR with this small change. Thanks
* unstream/master: (36 commits) + Add Optimatic Bid Adapter (prebid#1837) Add Bridgewell adapter (prebid#1825) Kumma adapter updated for Prebid 1.0 (prebid#1766) Touchup add bid response (prebid#1822) Fix skipped test (prebid#1836) Added new size in Rubicon pbjs Adapter (prebid#1842) HuddledMasses header bidding adapter (prebid#1806) Increment pre version Prebid 0.33.0 Release Update AOL adapter for v1.0 (prebid#1693) Sovrn 1.0 compliance (prebid#1796) Platform.io Bidder Adapter update (prebid#1817) Drop non-video bidders from video ad units (prebid#1815) Update renderAd to replace ${AUCTION_PRICE} in adUrl (prebid#1795) Pulsepoint adapter: fixing bid rejection due to missing mandatory bid params. (prebid#1823) Remove require.ensure entirely (prebid#1816) Add custom keyword support for pbs bid adapter (prebid#1763) OpenX Video Adapter update to Prebid v1.0 (prebid#1724) Fix test that hard-coded pbjs global. (prebid#1786) Update Pollux Adapter to v1.0 (prebid#1694) ...
….33.0 to aolgithub-master * commit '3e9756098bb20ecbe0314f16eed5298c5675b24c': (32 commits) Wrapped content type in options object. Added partners ids. Added changelog entry. Prebid 0.33.0 Release Update AOL adapter for v1.0 (prebid#1693) Sovrn 1.0 compliance (prebid#1796) Platform.io Bidder Adapter update (prebid#1817) Drop non-video bidders from video ad units (prebid#1815) Update renderAd to replace ${AUCTION_PRICE} in adUrl (prebid#1795) Pulsepoint adapter: fixing bid rejection due to missing mandatory bid params. (prebid#1823) Remove require.ensure entirely (prebid#1816) Add custom keyword support for pbs bid adapter (prebid#1763) OpenX Video Adapter update to Prebid v1.0 (prebid#1724) Fix test that hard-coded pbjs global. (prebid#1786) Update Pollux Adapter to v1.0 (prebid#1694) PubMatic adapter (prebid#1707) Added sizes to Rubicon Adapter (prebid#1818) jsonpFunction name should match the namespace (prebid#1785) Adding 33Across adapter (prebid#1805) Unit test fix (prebid#1812) ...
Type of change
Description of change
OpenX Adapter for Prebid v1.0
Bidder code: openx
Send All Bids Ad Server Keys: hb_pb, hb_adid, hb_size
new openxvideo biddersettings:
Other information
The OpenX adapter requires setup and approval from your OpenX team. Please reach out to your OpenX representative or Support@openx.com for more information and to enable using this adapter