Skip to content
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

Improve emoteevBidAdapter #3673

Merged
merged 6 commits into from
Apr 22, 2019
Merged

Conversation

piotr-yuxuan
Copy link
Contributor

@piotr-yuxuan piotr-yuxuan commented Mar 25, 2019

Type of change

  • Bugfix
  • Feature
  • New bidder adapter
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • This change doesn't affect user-facing APIs or examples documented on http://prebid.org.
  • Other

Description of change

Squash several pending changes

Following the long-awaited PR #3391, here are some changes we can now release. We avoided to modify too much the previous PR to keep things easy for the reviewer.

  • Improve test coverage
  • Document important constants
  • Extreme programming: document all functions
  • Imperative shell, functional core
  • Send events onAdapterCalled, onBidWon, and onTimeout
  • Report GDPR relevance and consent

Please note that some event pixels might trigger a 422 Unprocessable Entity response from our side. This doesn't preclude bid requests to be responded to, or won bid to be displayed.

Test coverage

Tests pass with HeadlessChrome 73.0.3683 (Mac OS X 10.14.3), including webGL tests. ? However IE and Edge don't support these tests (cf. 2679693...ed0b9ac) so commenting them out to make this PR pass CI. As a result, downgrading test coverage to:

  • 94.5% Statements 103/109
  • 98.18% Branches 54/55
  • 100% Functions 28/28
  • 93.88% Lines 92/98

Integration tests

Tested against production endpoint with the possible combinations of these following parameters:

  • Browser:
    · Chrome Canary 75.0.3739.0
    · Firefox Developer Edition 67.0b3 (64-bit)
    · Safari version 12.0.3 (14606.4.5)
    · Tor Browser 8.0.6 (based on Mozilla Firefox 60.5.1esr)
  • Integration page:
    · integrationExamples/gpt/hello_world_emoteev.html
    · https://jsfiddle.net/8aqotw1k/6/

Test parameters for validating bids:

{
  bidder: 'emoteev',
  params: {
    adSpaceId: 5084
  }
}

Localhost test server launched with:

$ npm install && gulp serve

Contact

** Squash several pending changes

- Improve test coverage
- Extreme programming: 100% test coverage
- Document important constants
- Extreme programming: document all functions
- Imperative shell, functional core
- Send events onBidWon and onTimeout
- Report GDPR relevance and consent

Code documentation uses JSDoc tags wherever possible. See the
following link for general explanation of the notation used:
https://github.com/google/closure-compiler/wiki/Annotating-JavaScript-for-the-Closure-Compiler

** Test coverage

- 100% Statements 110/110
- 100% Branches 55/55
- 100% Functions 28/28
- 100% Lines 99/99

** Integration tests

Tested against production endpoint with the possible combinations of
these following parameters:

- Browser:
  · Chrome Canary 75.0.3739.0
  · Firefox Developer Edition 67.0b3 (64-bit)
  · Safari version 12.0.3 (14606.4.5)
  · Tor Browser 8.0.6 (based on Mozilla Firefox 60.5.1esr)
- Integration page:
  · integrationExamples/gpt/hello_world_emoteev.html
  · https://jsfiddle.net/8aqotw1k/6/

Localhost test server launched with:
$ npm install && gulp serve
Has anybody an idea to make it pass?

https://circleci.com/gh/prebid/Prebid.js/2056

New test coverage:

- 94.5% Statements 103/109
- 98.18% Branches 54/55
- 100% Functions 28/28
- 93.88% Lines 92/98
@piotr-yuxuan
Copy link
Contributor Author

piotr-yuxuan commented Mar 25, 2019

Let's be pragmatic and temporarily give up on webGL tests. Has anybody got an idea to make them pass on IE and Edge? See 2679693...ed0b9ac for faulty tests.

Copy link
Contributor

@mike-chowla mike-chowla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adapters can not fire pixels outside of getUserSyncs. The adapter guidelines should be more clear on this point but it's definitely not allowed

modules/emoteevBidAdapter.js Outdated Show resolved Hide resolved
modules/emoteevBidAdapter.js Show resolved Hide resolved
modules/emoteevBidAdapter.js Show resolved Hide resolved
@piotr-yuxuan
Copy link
Contributor Author

piotr-yuxuan commented Mar 29, 2019

Hi @mike-chowla, thanks for your swift review!

We did choose to use triggerPixel because it looked more disciplined, and more evolved than manipulating raw img.src. Moreover we did choose to trigger pixels out of UserSyncs (namely in onTimeout, onBidWon, and isBidRequestValid) after reading through adapters code doing the same, so we do feel it's fair and right to do so.

These pixels would merely send us datapoints needed to monitor our adapter behaviour across the internet. The intent here is to get metrics as per the principles of three pillars of observability. I say it in good faith, we don't do that to track users, fiddle with their cookies, or to do nasty things. By the way, as suggested by these lines we are planning to be GDPR compliant as fast as possible.

TL;DR

The rest of this message examplifies how we intend to use data from these pixels to improve our adapter observability, then we look at what the documentation says about pixels, and finally we skim through merged, released code and find some examples of code similar to our intended use.

Because it's currently something accepted from other reviewers with no problem, and because it's currently used for fair purposes in other adapters, might I ask the team of Prebid.js to contemplate a second review?

Improve emoteevBidAdapter observability

on_adapter_called

  • This event would allow us to keep an eye on the average time between when isBidRequestValid returns true and when we actually display a creative. As I read our code again I acknowledge we should actually send an event every time isBidRequestValid is called so we can even give our partner websites the percentage of valid request and report misconfigured ad units.
  • Another interesting metric to observe is the time difference between on_adapter_called and our bid endpoint actually get the matching request. This could be helpful to the publishers to figure out if they call too much adapters, so that auctions become heavy and at the end they loose inventory.

on_bid_won

  • This event enables us to monitor the discrepencies between bid actually won and creative actually displayed.
  • Should a publisher grant us a bid, we want to make absolutely sure we are aware of it so we can pay the publisher transparently. An adapter pixel is a clean way of doing so. It's much more reliable than a pixel baked in the creative itself. Also, with this event we can efficiently spot problems in creative without penalizing the publisher.
  • We can monitor and compare the bids we've won against the creative we've chosen and the actually displayed creative. We want to live up to our commitment to spend efficiently each and every dollar an advertiser entrusts us with.
  • Finally it would enable us to provide fair metrics to our partners on the advertising side to honestly give them our win rate.

on_timeout

  • When a publisher activates our adapter on their site, they want us to reply and send back bids to their requests. This event enables us to make sure we keep being fast enough for them so we get things done under the timeout they set.
  • Also, should a publisher make a mistake and set a value too low, this would make it possible for us to debug and pro-actively advice them to reconsider their settings.

Adapter documentation

On 16 Apr 2018 @mkendall07 approved prebid/prebid.github.io#678 on the documentation about changes to onTimeout made by @vedantseta. It's still live, and can be read in the current documentation:

The onTimeout function will be called when an adpater timed out for an auction. Adapter can fire a ajax or pixel call to register a timeout at thier end.

In my humble opinion, the doc here shows a good idea: reacting to an event (timeout) with a side-effect (pixel call). We merely want to implement this idea and react to a timeout, but also a handful of other events: when our adapter is called, and when a bid is won.

Current pixels outside of UserSyncs

Our current review says: « adapters are prohibited from firing their own pixels. If you want fire pixels, you can return them from get getUserSyncs ». However there are a few examples of recently approved and merged code which fire pixels out of UserSyncs. So we feel it's fair to do the same and gather data to monitor and improve our adapter.

Mgid triggers a raw pixel in onBidWon. This commit has been created on 22 Fev 2019 and approved the same day by @idettman. It's just a couple of weeks ago. It looks like the current usage.

Zedo uses utils.triggerPixels to fire pixels in onTimeout and onBidWon. These changes got in PR #3240 and where approved by @jaiminpanchal27 on 6 Nov 2018, a mere four months ago.

Medianet uses utils.triggerPixels to fire pixels in onTimeout and onBidWon. These changes got in PR #2954 and where approved by @harpere on 11 Sep 2018, that's to say six months ago.

Moreover JustPremium uses raw img.src to fire pixels in onTimeout and this was approved by @jsnellbaker on 30 Oct 2018 with no remarks.

Mantis uses raw img.src to fire pixels on master since it's been approved on 9 Jun 2018 by @idettman.

@mike-chowla
Copy link
Contributor

Looks like some pixels have slipped when they should not have been approved. Prebid.js' policy doesn't allow them. The only way I can allow them is if @mkendall07 OKs them.

@piotr-yuxuan
Copy link
Contributor Author

piotr-yuxuan commented Apr 8, 2019

Hi @mike-chowla,

Thanks for your reply. In my humble opinion, the paragraph Adapter documentation of my previous message might suggest pixels are actually allowed in event handlers like onTimeout. However I gonna await @mkendall07's response 🙂

Cheers

@jeremyvdw
Copy link

@mkendall07 Do you mind having a look at this? We are kind of stuck with this PR and it's now getting pretty urgent. Thank you

@mkendall07
Copy link
Member

@piotr-yuxuan @jeremyvdw
I've reviewed and put a few comments. We'll allow the pixels from the event handlers since it was previously allowed. Thanks.

@piotr-yuxuan
Copy link
Contributor Author

piotr-yuxuan commented Apr 22, 2019

@mkendall07 @mike-chowla thanks for your patience and your reviews. To the best of my knowledge this code should now follow your reviews guidelines and be compliant with Prebid.js pixel policy. Please let me know any further needed changes.

@mike-chowla mike-chowla merged commit 253cbf4 into prebid:master Apr 22, 2019
jacekburys-quantcast pushed a commit to jacekburys-quantcast/Prebid.js that referenced this pull request May 15, 2019
* Improve emoteevBidAdapter

** Squash several pending changes

- Improve test coverage
- Extreme programming: 100% test coverage
- Document important constants
- Extreme programming: document all functions
- Imperative shell, functional core
- Send events onBidWon and onTimeout
- Report GDPR relevance and consent

Code documentation uses JSDoc tags wherever possible. See the
following link for general explanation of the notation used:
https://github.com/google/closure-compiler/wiki/Annotating-JavaScript-for-the-Closure-Compiler

** Test coverage

- 100% Statements 110/110
- 100% Branches 55/55
- 100% Functions 28/28
- 100% Lines 99/99

** Integration tests

Tested against production endpoint with the possible combinations of
these following parameters:

- Browser:
  · Chrome Canary 75.0.3739.0
  · Firefox Developer Edition 67.0b3 (64-bit)
  · Safari version 12.0.3 (14606.4.5)
  · Tor Browser 8.0.6 (based on Mozilla Firefox 60.5.1esr)
- Integration page:
  · integrationExamples/gpt/hello_world_emoteev.html
  · https://jsfiddle.net/8aqotw1k/6/

Localhost test server launched with:
$ npm install && gulp serve

* Tentative CI fix for IE and Edge webGL test

https://circleci.com/gh/prebid/Prebid.js/2052

* Tentative CI fix for IE and Edge webGL test prebid#2

* Give up on webGL tests

Has anybody an idea to make it pass?

https://circleci.com/gh/prebid/Prebid.js/2056

New test coverage:

- 94.5% Statements 103/109
- 98.18% Branches 54/55
- 100% Functions 28/28
- 93.88% Lines 92/98

* Avoid useless noice in diff

* Remove unallowed metric pixel ON_ADAPTER_CALLED
VideoReach pushed a commit to VideoReach/Prebid.js that referenced this pull request Aug 1, 2019
* Improve emoteevBidAdapter

** Squash several pending changes

- Improve test coverage
- Extreme programming: 100% test coverage
- Document important constants
- Extreme programming: document all functions
- Imperative shell, functional core
- Send events onBidWon and onTimeout
- Report GDPR relevance and consent

Code documentation uses JSDoc tags wherever possible. See the
following link for general explanation of the notation used:
https://github.com/google/closure-compiler/wiki/Annotating-JavaScript-for-the-Closure-Compiler

** Test coverage

- 100% Statements 110/110
- 100% Branches 55/55
- 100% Functions 28/28
- 100% Lines 99/99

** Integration tests

Tested against production endpoint with the possible combinations of
these following parameters:

- Browser:
  · Chrome Canary 75.0.3739.0
  · Firefox Developer Edition 67.0b3 (64-bit)
  · Safari version 12.0.3 (14606.4.5)
  · Tor Browser 8.0.6 (based on Mozilla Firefox 60.5.1esr)
- Integration page:
  · integrationExamples/gpt/hello_world_emoteev.html
  · https://jsfiddle.net/8aqotw1k/6/

Localhost test server launched with:
$ npm install && gulp serve

* Tentative CI fix for IE and Edge webGL test

https://circleci.com/gh/prebid/Prebid.js/2052

* Tentative CI fix for IE and Edge webGL test prebid#2

* Give up on webGL tests

Has anybody an idea to make it pass?

https://circleci.com/gh/prebid/Prebid.js/2056

New test coverage:

- 94.5% Statements 103/109
- 98.18% Branches 54/55
- 100% Functions 28/28
- 93.88% Lines 92/98

* Avoid useless noice in diff

* Remove unallowed metric pixel ON_ADAPTER_CALLED
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants