APS Bid Adapter: Initial Release #14255
Conversation
|
This pull request is pending final verification and approval from Amazon and Prebid. Please keep this comment unresolved until explicit confirmation is received to proceed with the merge. |
|
Tread carefully! This PR adds 6 linter warnings (possibly disabled through directives):
|
Noted, will resolve these warnings prior to merge. |
bjoberg-amzn
force-pushed
the
apsBidAdapter/v2.0.0
branch
from
f084d56 to
31d2df7
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
modules/apsBidAdapter.js
Outdated
|
|
||
| // Validate and process impressions - fail fast on structural issues | ||
| if (!request.imp || !Array.isArray(request.imp)) { | ||
| throw new Error('Request must contain a valid impressions array'); |
There was a problem hiding this comment.
We don't allow throws, please fix to logError
There was a problem hiding this comment.
Understood, we will fix this.
modules/apsBidAdapter.js
Outdated
| (s.type === 'image' && syncOptions.pixelEnabled) | ||
| ); | ||
| } catch (err) { | ||
| record('getUserSyncs/didError', { error: err }); |
There was a problem hiding this comment.
This seems like another opportunity to fallback from the record when telemetry is false
| bid.ad = `<script src="${creativeUrl}"></script> | ||
| <script> | ||
| const accountID = '${accountID}'; | ||
| window._aps = window._aps || new Map(); |
There was a problem hiding this comment.
This looks odd to me because the creative is (usually) rendered in a separate iframe and I don't expect this to be useful there. If you're just interested in recording when the render happens, onAdRenderSucceeded runs immediately after this.
There was a problem hiding this comment.
APS uses an event-driven architecture. The record statement here is not used for analytics but instead to trigger a callback in the APS creative render script (creativeUrl in the code snippet).
modules/apsBidAdapter.js
Outdated
| if (!doesHWExist && doesFormatExist) { | ||
| const { w, h } = imp.banner.format[0]; | ||
| if (typeof w !== 'number' || typeof h !== 'number') { | ||
| throw new Error( |
There was a problem hiding this comment.
Ignoring unsuitable format / imps is better than throwing, as you may still get something you can use somewhere else in the request. It would align better with how isBidRequestValid filters out only invalid requests - this sort of validation is meant to be done there, although it doesn't work on ortb imp objects which is probably why you're doing it here.
There was a problem hiding this comment.
Makes sense, we will fix this. Thanks!!
ChrisHuie
requested review from
gmcgrath11 and
robertrmartinez
and removed request for
jsnellbaker
| if (!imp.banner) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Hey sorry to point this out again, but just checking this short circuit is intentional. I didn't spot any related test context and it tosses out video bids.
Example request:
There was a problem hiding this comment.
Appreciate the feedback. The short circuit is intended so that we only add the w and h properties to banner objects. Additional test context is now included to clarify this scenario. The test I am referring to is: "when imp array contains banner object with invalid format..."
|
@codex review |
![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.
ℹ️ 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".
bjoberg-amzn
force-pushed
the
apsBidAdapter/v2.0.0
branch
from
31d2df7 to
ed90dc3
Compare
**Overview** ------------ APS (Amazon Publisher Services) bid adapter initial open source release. **Changes** ----------- - (feat) Banner ad support - (feat) Video ad support - (feat) iframe user sync support - (feat) Telemetry and analytics - (docs) Integration guide
bjoberg-amzn
force-pushed
the
apsBidAdapter/v2.0.0
branch
from
ed90dc3 to
38db9cf
Compare
|
@ChrisHuie, @patmmccann, @dgirardi, @robertrmartinez, @gmcgrath11, @ETNOL - Thank you for taking the time to review our pull request. I've addressed all the review comments that have been raised so far. Just wanted to check in to see if there are any remaining questions or concerns before we move forward with opening this PR for final review and approval. Also, I've noticed the Safari E2E tests are failing. From what I can tell in the debug output, these failures appear to be unrelated to the proposed changes in this PR. Please let me know if I'm misunderstanding these errors or if there's something specific I should address. Looking forward to your feedback. Thanks again for your time and guidance on this submission! |
|
@bjoberg-amzn I believe this is good to move out of draft status into an open pr 👍 |
|
Thanks @ChrisHuie I have moved it out of draft status. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 598fb2395a
ℹ️ 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".
| const bidResponse = buildBidResponse(bid, context); | ||
| if (bidResponse.mediaType === VIDEO) { | ||
| bidResponse.vastUrl = vastUrl; |
There was a problem hiding this comment.
For video bids that return a VAST URL in nurl (with no adm), buildBidResponse already sets bidResponse.vastUrl from nurl. This code unconditionally overwrites bidResponse.vastUrl with the local vastUrl, which is only populated from bid.adm, so in the nurl-only case it becomes undefined and the creative URL is lost. That breaks video rendering whenever APS responds with a VAST URL instead of inline VAST; only assign bidResponse.vastUrl when you actually derive a value or prefer bid.nurl.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@bjoberg-amzn any thoughts on this? Do you want to preserve your ability to return a vast url perhaps by only overwriting this if it is null?
There was a problem hiding this comment.
@patmmccann In our implementation, we never return nurl, we only use bid.adm to pass VAST URLs (not inline markup). Since buildBidResponse would only set vastUrl from nurl (which we don't use), there's no existing value to preserve.
However, we are revisiting this to support this in our upcoming releases.
6 participants
Overview
APS (Amazon Publisher Services) bid adapter initial open source release.
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