Rubicon analytics #2278

snapwich · 2018-03-16T22:03:55Z

Type of change

Feature
Bugfixes

Description of change

This includes the new Rubicon analytics adapter. The endpoint will be changed, but for now can be configured with the endpoint option when calling pbjs.enableAnalytics

            pbjs.enableAnalytics({
                provider: 'rubicon',
                options: {
                    endpoint: 'http://localhost:9999/event',
                    accountId: 1001
                }
            });

Other information

The karma testing library was updated to use the mocha reporter. It includes more information, better debug information (such as object diffing), and is just overall a better reporter than the progress-reporter.

An additional BIDDER_DONE event was added that signals when a bid adapter has finished responding to bids (this is a solution to better determine no-bids).

The SET_TARGETING event was modified to also pass the targeting that was set as an event argument.

Also a few bug fixes, such as cleaning up handlers left around by other analytics adapters (was causing errors in unrelated tests).

…ver'

mkendall07

Overall LGTM. Just a few comments/questions.

mkendall07 · 2018-03-29T20:44:11Z

src/adaptermanager.js

@@ -112,6 +112,8 @@ function getAdUnitCopyForPrebidServer(adUnits) {
  let adUnitsCopy = utils.deepClone(adUnits);

  adUnitsCopy.forEach((adUnit) => {
+    // TODO: fix! this should probably happen in the adapter rather than here. now all the analytics adapters as well as
+    // `getBids` above need to deal w/ potentially two different size formats.


good callout

mkendall07 · 2018-03-29T20:45:24Z

src/AnalyticsAdapter.js

@@ -130,6 +137,7 @@ export default function AnalyticsAdapter({ url, analyticsType, global, handler }
    utils._each(_handlers, (handler, event) => {
      events.off(event, handler);
    });
+    this.enableAnalytics = this._oldEnable ? this._oldEnable : _enable;


i'm not clear on what this change is for.

enableAnalytics is overwritten w/ the error message function above when an analytics adapter is enabled. This new code I added sets enableAnalytics back to the correct enable function when analytics is disabled (can't just set it back to _enable as some analytics extend enalbeAnalytics with a new function).

Probably not a use case you'd encounter much in production (disabling analytics and then enabling again), but in test cases it was an issue. Many of the analytics adapters were causing issues with other test suites as they were not cleaning up their event handlers, probably in part because cleaning them up (using disableAnalytics) didn't allow you to listen again since enableAnalytics was overwritten; this fixes that.

mkendall07 · 2018-03-29T20:47:33Z

src/prebid.js

@@ -194,7 +194,7 @@ $$PREBID_GLOBAL$$.setTargetingForAst = function() {
  targeting.setTargetingForAst();

  // emit event
-  events.emit(SET_TARGETING);
+  events.emit(SET_TARGETING, targeting.getAllTargeting());


do we doc the event signatures anywhere? Probably needs to be done since we are adding params.

Nowhere that I know of. The events themselves are documented on the publisher API reference page, but it doesn't include details of what arguments the events pass to the event handlers.

I did add a new event bidderDone event though that should be added to that page. Just created a pull-request: prebid/prebid.github.io#695

TODO Later

jaiminpanchal27

@snapwich Looks good. Left some minor comments

jaiminpanchal27 · 2018-04-02T14:41:30Z

modules/rubiconAnalyticsAdapter.js

+          case GOOD:
+            bid.status = 'success';
+            break;
+          case NO_BID:


I think we do not have NO_BID case any more.
https://github.com/prebid/Prebid.js/blob/master/src/adapters/bidderFactory.js#L299

There's still some instances that can result in NO_BIDs such as in prebid server adapter, currency module, etc. I'll probably leave this here just in case; I can remove later if NO_BIDs end up being removed entirely from the prebid code base.

jaiminpanchal27 · 2018-04-02T14:44:31Z

modules/rubiconAnalyticsAdapter.js

+};
+
+// basically lodash#pick that also allows transformation functions and property renaming
+function _pick(obj, properties) {


Should we move this to utils ?

We could, but I don't think it's necessary. I'd rather leave it here until such time that someone else decides they need to heavily utilize a pick function. Plus I deviated a bit from lodash's implementation of pick, which could be confusing for others to use if they're not familiar with my intent.

# Conflicts: # src/adaptermanager.js

snapwich · 2018-04-04T15:55:53Z

resolved conflicts

harpere

LGTM

# Conflicts: # package-lock.json

bretg · 2018-04-12T15:20:22Z

@mjacobsonny -- Rich submitted a doc change for the new bidderDone event. Any others you were looking for? We don't want to put this on the analytics adapter list just yet.

rubicon analytics adapter

snapwich added 30 commits March 16, 2018 15:44

rubicon analytics adapter initial commit

6cf2d5a

fix some linting errors and allow endpoint option in rubicon analytics

aeddcb2

remove karma-expect and just use karma-chai

6cc3a8e

update mocha and use mocha-reporter

3cb0d2b

change adxcg analytics spec to clean up handlers after tests

83a78e3

AnalyticsAdapter doesn't throw error if config isn't object now

873aef6

added chai-subset to testing framework

2141518

no chai-subset. we'll just use deep object matches where we can

8469034

recieving successful analytics messages in rubicon analytics

c957ba2

fix typo in s2sConfig setting grabbing for rubicon analytics

1f8b5ce

more rubicon analytics tests

21ba8ff

added schema validation for rubicon analytics

52cf6a0

updates to base analytics message in rubicon

57ff041

make sure status and source are always set in rubicon analytics

c5ce70c

updated rubicon analytics to latest schema

e932114

fix typo in rubicon analytics schema

298e975

fix transactionId and bidResponse.mediaType in rubicon analytics

bfc16c9

add more adUnit stuff to bidsWon in rubicon analytics

1ad5e50

updated analytics mock and added adUnit.status in rubicon analytics

fd5e500

samplingFactor in rubicon analytics implemented

8ba03c1

add video adUnit fix bugs with mediaType in rubicon analytics

3a7bacd

more schema changes

97ea271

serverAccountId to accountId in rubicon analytics

84f467d

fix enums to all be snake-case

f1811e5

updated packages

27e097c

use application/json for rubicon analytics

7439c32

standardize no-bid in rubicon analytics

d89c4b2

adserverTargeting properties must be strings in rubicon analytics

e1f9257

add bidwonStatus to bidsWon in rubicon analytics

3d9871d

add bidwonStatus as ENUM

567c352

mjacobsonny added the needs docs label Mar 27, 2018

Added logic to change clientLatencyMillis prop name if source is 'ser…

99c68a1

…ver'

mkendall07 approved these changes Mar 29, 2018

View reviewed changes

mkendall07 added LGTM needs 2nd review Core module updates require two approvals from the core team labels Mar 29, 2018

mkendall07 requested a review from jaiminpanchal27 March 29, 2018 20:48

Isaac Dettman and others added 4 commits March 30, 2018 13:50

Renamed 'source' to 'bidSource'

74bf553

Renamed 'finalSource' value from 'source' to 'bidSource'

cfbf99c

Setting bidSource key to source

c4cb224

Reverting back clientLatency -> ServerLatency change

3f85df4

TODO Later

jaiminpanchal27 approved these changes Apr 2, 2018

View reviewed changes

mkendall07 added this to the 1.7 milestone Apr 2, 2018

snapwich mentioned this pull request Apr 4, 2018

added new 'bidderDone' event to publisher API page prebid/prebid.github.io#695

Merged

Merge branch 'master' into rubicon-analytics

7661711

# Conflicts: # src/adaptermanager.js

idettman added adapter support and removed adapter support labels Apr 4, 2018

rubicon analytics should always pass accountId as number

e99ca8a

harpere approved these changes Apr 10, 2018

View reviewed changes

harpere removed the needs 2nd review Core module updates require two approvals from the core team label Apr 10, 2018

snapwich added 5 commits April 10, 2018 10:27

Merge branch 'master' into rubicon-analytics

cacbdf3

# Conflicts: # package-lock.json

update package lock to solve conflict

254474c

update rubicon analytics endpoint

2f3a9f7

fix test with endpoint change

d7e6ee3

make endpoint required for rubicon analytics

2044796

snapwich merged commit 0bf6c7d into prebid:master Apr 10, 2018

mkendall07 removed the in progress label Apr 10, 2018

dluxemburg pushed a commit to Genius/Prebid.js that referenced this pull request Jul 17, 2018

Rubicon analytics (prebid#2278)

4df0fb0

rubicon analytics adapter

idettman deleted the rubicon-analytics branch February 29, 2020 06:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rubicon analytics #2278

Rubicon analytics #2278

snapwich commented Mar 16, 2018 •

edited

Loading

mkendall07 left a comment

mkendall07 Mar 29, 2018

mkendall07 Mar 29, 2018

snapwich Apr 4, 2018

mkendall07 Mar 29, 2018

snapwich Apr 4, 2018

jaiminpanchal27 left a comment

jaiminpanchal27 Apr 2, 2018

snapwich Apr 4, 2018

jaiminpanchal27 Apr 2, 2018

snapwich Apr 4, 2018

snapwich commented Apr 4, 2018

harpere left a comment

bretg commented Apr 12, 2018

Rubicon analytics #2278

Rubicon analytics #2278

Conversation

snapwich commented Mar 16, 2018 • edited Loading

Type of change

Description of change

Other information

mkendall07 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

jaiminpanchal27 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

snapwich commented Apr 4, 2018

harpere left a comment

Choose a reason for hiding this comment

bretg commented Apr 12, 2018

snapwich commented Mar 16, 2018 •

edited

Loading