Optimize AOL adapter and heavily reduce the size #653

marian-r · 2016-09-23T13:54:49Z

Type of change

Description of change

Refactoring of the AOL adapter, optimizing the logic.
Removing additional request for a JavaScript library, thus improving performance, reducing the delay and bandwidth usage.
AOL adapter now also supports deals.
Adding unit tests (100% code coverage).
Adding Number.isInterger() polyfill.

Other information

AOL adapter is now maintained by @aol/header-bidding team.

- no aditional request for a JavaScript library - also supports deals - unit tests - Number.isInterger() polyfill

CLKeenan · 2016-09-23T18:44:21Z

This is great news!! Really looking forward to this.

marian-r · 2016-09-27T08:59:38Z

I think the failed build was not caused by the changes introduced in this PR. It might be related to #642.

nanek · 2016-09-27T21:07:34Z

src/adapters/aol.js

+      ajax(pubapiUrl, (response) => {
+        if (!response && response.length <= 0) {
+          utils.logError('Empty bid response', BIDDER_CODE, bid);
+          return;


For an empty bid response, you will still want to add a bid response, likely with _addErrorBidResponse.

Thanks for the suggestion. This is now fixed.

nedstankus · 2016-09-27T22:02:57Z

I'm seeing issues with the window=window assignment that's in loaders/delimiterLoader.js. You can see this on the example pages; e.g., http://localhost:9999/integrationExamples/gpt/pbjs_example_gpt.html

marian-r · 2016-09-28T09:46:44Z

@nedstankus that is not part of this PR. You are not testing the correct git branch. Please use contrib/optimized-aol-adapter branch.

nedstankus · 2016-09-28T16:47:06Z

@marian-r You're correct--contrib/optimized-aol-adapter does not have that issue. I was on master.

marian-r · 2016-10-04T16:28:44Z

Are there still any issues/concerns regarding this PR? It has been marked as 'needs review' so do you have an estimate of when the PR could be reviewed?

protonate · 2016-10-04T17:02:10Z

I should be able to review today or tomorrow, thanks for your patience.

marian-r · 2016-10-07T12:51:53Z

These update of AOL adapter also changes the way how gross to net price calculations are handled for AOL bidder. Would it be possible to include a note about this change in the release notes once this is released? I would suggest something like this:

If you're using AOL as a bidder, please contact your Marketplace account team for a quick config fix (not doing gross to net config client side) as it's now done server side for AOL.

And do you have any estimate of when this could be released? Is it possible to include this in the 0.14.0 milestone?

mkendall07 · 2016-10-07T13:50:55Z

@marian-r
Thanks for the info. Yes we can include that info in the release notes. I've added to 0.14.0 milestone

protonate

This looks great, thanks for the submission. As noted an empty seatbid array in the response shouldn't throw an error as this is normal "no bid" response. Also if possible to sync the bid request/response IDs.

protonate · 2016-10-11T19:46:04Z

src/adapters/aol.js

-    if (!bid) {
-      utils.logError('mismatched bid: ' + context.placement, ADTECH_BIDDER_NAME, context);
+    try {
+      bidData = response.seatbid[0].bid[0];


An empty bid or "no bid" response causes this to fail which throws an error, though a response with an empty seatbid array appears to be common in my testing, so should be handled as other than Error.

When an error is thrown here, addErrorBidResponse() is called, which adds bid response to the bidmanager with status 2 (empty or error bid response). That, I believe, is correct behavior, but I removed error logging, so it won't appear as error in the console when debugging. That was done in 981fb75.

protonate · 2016-10-11T19:49:53Z

src/adapters/aol.js

-      var bidreq = _mapUnit(bid);
-      window.ADTECH.loadAd(bidreq);
-    });
+    var bidResponse = bidfactory.createBid(1);


Consider setting the bid response ID to match the bid request ID as described here: #509.

Doing so will position the adapter to take advantage of Prebid Roadmap features such as secure iframe support, improved analytics, Prebid Auctions and so on.

Thanks for this suggestion. Done in 38bd3ae.

protonate · 2016-10-11T19:50:35Z

src/adapters/aol.js


-    // add it to the bid manager
+  function _addErrorBidResponse(bid, response = {}) {
+    var bidResponse = bidfactory.createBid(2);


Consider setting the bid response ID to match the bid request ID as described here: #509.

Doing so will position the adapter to take advantage of Prebid Roadmap features such as secure iframe support, improved analytics, Prebid Auctions and so on.

Thanks for this suggestion. Done in 38bd3ae.

marian-r · 2016-10-12T13:19:29Z

All issues have been addressed.

mkendall07 · 2016-10-12T21:08:31Z

src/adapters/aol.js

-    if (cpm === null || isNaN(cpm)) {
-      return _addErrorBid(response, context);
+    if (!SERVER_MAP.hasOwnProperty(regionParam)) {
+      console.warn(`Unknown region '${regionParam}' for AOL bidder.`);


console.warn doesn't play nice in IE 9. Please use utils.logWarn instead for safety.

The reason for using console.warn() directly is that this warning is meant to be shown when a publisher is configuring a site and uses incorrect region. I want to show the warning even if the debug mode is turned off, so the publisher can see what's wrong. And I expect publisher to be configuring it in a browser other than IE9 🤔. After the site is configured correctly, the warning should go away, so I believe it is safe to use this console.warn(). If you feel otherwise, please let me know.

fair enough I suppose. Thanks

mkendall07 · 2016-10-12T21:26:15Z

@marian-r
Thanks for updating the adapter and adding tests. My last request for you is if you could provide us with test params that return bids for use in our integration testing. Thank you!

marian-r · 2016-10-13T16:36:28Z

@mkendall07 thanks for merging. I'm just following with the testing params:

This is for 728x90 placements:

{
  bidder: 'aol',
  params: {
    network: '9599.1',
    placement: 3675022 // 728x90
  }
}

This is for 300x250 placements:

{
  bidder: 'aol',
  params: {
    network: '9599.1',
    placement: 3675026 // 300x250
  }
}

Please let me know whether they work.

Optimize AOL adapter and heavily reduce the size

9c53431

- no aditional request for a JavaScript library - also supports deals - unit tests - Number.isInterger() polyfill

nanek reviewed Sep 27, 2016

View reviewed changes

Fix AOL adapter to add invalid bid in case of empty response

67c307a

mkendall07 added the needs review label Sep 30, 2016

mkendall07 assigned protonate Sep 30, 2016

mkendall07 added this to the Prebid 0.14.0 milestone Oct 7, 2016

protonate suggested changes Oct 11, 2016

View reviewed changes

protonate added needs update and removed needs review labels Oct 11, 2016

marian-r added 2 commits October 12, 2016 14:11

Set bid response ID to bid request ID for AOL

38bd3ae

Remove error logging for empty bids and refactor ES6

981fb75

mkendall07 approved these changes Oct 12, 2016

View reviewed changes

mkendall07 assigned mkendall07 and unassigned protonate Oct 12, 2016

mkendall07 merged commit 38a2d25 into prebid:master Oct 13, 2016

This was referenced Oct 19, 2016

Show warning if bidCpmAdjustment is set for AOL bidder #725

Closed

Show warning if bidCpmAdjustment is set for AOL bidder (closes #725) #728

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize AOL adapter and heavily reduce the size #653

Optimize AOL adapter and heavily reduce the size #653

marian-r commented Sep 23, 2016

CLKeenan commented Sep 23, 2016

marian-r commented Sep 27, 2016

nanek Sep 27, 2016

marian-r Sep 28, 2016

nedstankus commented Sep 27, 2016

marian-r commented Sep 28, 2016

nedstankus commented Sep 28, 2016

marian-r commented Oct 4, 2016

protonate commented Oct 4, 2016

marian-r commented Oct 7, 2016

mkendall07 commented Oct 7, 2016

protonate left a comment

protonate Oct 11, 2016

marian-r Oct 12, 2016

protonate Oct 11, 2016

marian-r Oct 12, 2016

protonate Oct 11, 2016

marian-r Oct 12, 2016

marian-r commented Oct 12, 2016

mkendall07 Oct 12, 2016

marian-r Oct 13, 2016

mkendall07 Oct 13, 2016

mkendall07 commented Oct 12, 2016

marian-r commented Oct 13, 2016

Optimize AOL adapter and heavily reduce the size #653

Optimize AOL adapter and heavily reduce the size #653

Conversation

marian-r commented Sep 23, 2016

Type of change

Description of change

Other information

CLKeenan commented Sep 23, 2016

marian-r commented Sep 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nedstankus commented Sep 27, 2016

marian-r commented Sep 28, 2016

nedstankus commented Sep 28, 2016

marian-r commented Oct 4, 2016

protonate commented Oct 4, 2016

marian-r commented Oct 7, 2016

mkendall07 commented Oct 7, 2016

protonate left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marian-r commented Oct 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkendall07 commented Oct 12, 2016

marian-r commented Oct 13, 2016