added support for sampling in ga and base adapter, fixed up some tests #1011

snapwich · 2017-02-24T23:29:31Z

Type of change

Feature
Refactoring (no functional changes, no api changes)

Description of change

Added a new configuration property, sampling, to the google analytics adapter and the base analytics adapter which allows these adapters to only report a sampled percentage of visits. The sampling rate is set with a decimal value between zero and one, one (the default) being 100% of analytics sent, and zero being no analytics sent.

Also, while writing some unit tests for the base analytics adapter I was having issues with tests keeping state and affecting other tests (specifically my new tests), so I refactored the tests a bit to behave in a more isolated fashion. This also included adding a disableAnalytics method to the analytics adapter which allows for the cleanup of event listeners.

The sampling is used as an additional config like so:

pbjs.enableAnalytics({
    provider: 'ga',
    options: {
        global: 'ga',
        enableDistribution: false,
        sampling: .25
    }
});

That would allow for a 25% sampling rate in the analytics adapter.

~~Edit~~
~~Also added a new configuration property, pipe, to the base adapter. This option is a function you can provide to allow transformation of event data before it is logged.~~

snapwich · 2017-02-24T23:31:51Z

test/spec/unit/adapters/analytics/AnalyticsAdapter_spec.js

        });
-        window[config.global].restore();
+
+        it('SHOULD NOT call global again when adapter.enableAnalytics is called with previous timeout', () => {


This test originally said "WHEN adapter.enableAnalytics is called twice" and was testing that the event was present from the test that executed before it. I updated it to test what I think it was trying to test, but let me know if I'm mistaken.

snapwich · 2017-03-14T20:59:54Z

There was another feature we were hoping to add but it depended on a lot of this code that hasn't been merged yet, so I've updated this pull-request with that feature since it wasn't a big change. The original pull-request comment has been edited to describe the new "pipe" functionality.

…ging

mkendall07 · 2017-03-16T15:00:09Z

@snapwich
Thanks for the PR. Can you explain the use case for the "pipe" operation?

snapwich · 2017-03-16T16:00:37Z

@mkendall07 It allows publishers to transform data before being sent to an analytics adapter through runtime configuration. We'll be submitting our analytics adapter soon which more or less logs raw data, and allowing different data transformations for different accounts is a nice feature to have.

bretg · 2017-03-16T17:23:02Z

@mkendall07 - Rich came up with the feature as a general way for each publisher to customize the latency or CPM buckets for logging to their analytics server.

I have Prebid.org documentation on my list for this amongst other recent updates...

grevory · 2017-03-16T18:18:20Z

👍

bretg · 2017-03-20T22:37:43Z

@mjacobsonny or others - any opinions about whether sampling on BIDS_WON (renders) is a good idea in all cases? This PR currently samples everything at the same rate, but for publishers with high Direct sell-through, renders could be rare, so sampling aggressively may make the bids_won statistic log infrequently. Which could affect the ability to estimate revenue.

For other pubs, however, there may be many renders, so eliminating the sampling would cost them more in the analytics system. (GA has a monthly limit for the free account which would affect many pubs at 100% sampling).

One possible solution would be to pull bids_won out and allow it to have a separate sample rate. Doubt we really need to support a sample rate for every event, but bids_won is arguably different and may have different analytics requirements for different customers.

Don't want to over-complicate this, but still want to meet the goal of allowing pubs the option to control GA traffic.

bretg · 2017-03-22T19:02:22Z

Spoke with one of our guys in the field who agreed that A) sampling would be helpful for some pubs, and B) renders could be different for some pubs.

So I'm proposing that we optionally allow a separate sampling rate on renders. e.g.

pbjs.que.push(function() {
    pbjs.enableAnalytics({
        provider: 'ga',
        options: {
            global: 'ga',
            enableDistribution: false,
            sampling: 0.05,
            renderSampling: 1
        }
    });
});

If renderSampling isn't specified, BIDS_WON events default to the sampling rate.

protonate · 2017-03-27T23:08:43Z

I wonder if this change may be best scoped to individual analytics adapters that would work well with the sampling functionality. For example, current GA adapter doesn't extend AnalyticsAdapter, and the sampling and pipe changes are in AnalyticsAdapter. The AppNexus analytics adapter does extend AnalyticsAdapter but in this case sampling chance based on a coin flip is going to leave gaps in the analytics for an auction, as partial updates are packaged up with each Prebid event and then sent on a periodic basis.

bretg · 2017-03-29T21:23:54Z

@protonate - good call on the AnalyticsAdapter issue.

in this case sampling chance based on a coin flip is going to leave gaps in the analytics for an auction

Rich and I discussed the coin flip happening at the page level, so all events on the page are either logged or not. So there shouldn't be gaps in the tracking for a given auction. Not what's there?

Also, I discussed the render-sampling idea with other people and am letting that one go.

snapwich · 2017-03-30T15:33:14Z

@protonate - with regards to sampling, the changes in this pull-request are included in both the GA adapter and the AnalyticsAdapter. With regards to the piping functionality, the change was just in the AnalyticsAdapter as the GA adapter handled each event separately. If you want, we could scope the piping functionality to our individual analytics adapter.

snapwich · 2017-03-30T17:14:30Z

I updated this pull-request to remove the piping functionality. We'll implement the desired interactions directly in our Rubicon analytics adapter. The sampling functionality should work in both GA and any adapter directed from the AnalyticsAdapter.

mkendall07 · 2017-03-30T19:45:20Z

src/adapters/analytics/AnalyticsAdapter.js

-        _enqueue.call(_this, { eventType, args });
-      }
-    });
+    if (_sampled) {


Edit: ignore this - I see how this work now.

protonate

Looks good, confirmed functionality, tests are green, reviewed.

protonate · 2017-04-06T23:35:12Z

@snapwich @bretg this is reviewed so need one more from your end and we'll merge.

grevory · 2017-04-07T15:17:44Z

src/adapters/analytics/ga.js

-      sendBidResponseToGa(bid);
+    utils._each(existingEvents, function (eventObj) {
+      var args = eventObj.args;
+      if (!eventObj) {


If this validation is necessary then line 52 will throw a TypeError.

it should be noted that these line changes were only from moving existing code around to be inside the if(_sampled) { block above. However, this is a bug; and even though it existed before this pull-request, now is as good as time as any to fix it. Thanks!

grevory

One things needs review but otherwise +1

grevory · 2017-04-07T15:21:20Z

test/spec/unit/adapters/analytics/AnalyticsAdapter_spec.js

@@ -29,7 +29,6 @@ FEATURE: Analytics Adapters API
        const adapter = new AnalyticsAdapter(config);
        var spyTestGlobal = sinon.spy(window, config.global);

-        adaptermanager.registerAnalyticsAdapter(adapter, 'adapter');


FYI: I don't understand the consequences of removing this line or others like it.

There is no consequence, which is why I removed them. All the unit tests were using the adapter instance directly rather than getting it from the adapters registered with Prebid.js. That's fine, but makes these registerAnalyticsAdapter lines irrelevant.

I'm sure it's all good. I was just pointing out my lack of extensive Prebid knowledge to know much about that is happening here.

grevory · 2017-04-07T15:22:27Z

src/adapters/analytics/ga.js

-      bid = args;
-      sendBidResponseToGa(bid);
+    utils._each(existingEvents, function (eventObj) {
+      var args = eventObj.args;


Could throw a TypeError. See comment below.

snapwich

@grevory made updates!

snapwich · 2017-04-07T20:08:15Z

test/spec/unit/adapters/analytics/AnalyticsAdapter_spec.js

@@ -29,7 +29,6 @@ FEATURE: Analytics Adapters API
        const adapter = new AnalyticsAdapter(config);
        var spyTestGlobal = sinon.spy(window, config.global);

-        adaptermanager.registerAnalyticsAdapter(adapter, 'adapter');


There is no consequence, which is why I removed them. All the unit tests were using the adapter instance directly rather than getting it from the adapters registered with Prebid.js. That's fine, but makes these registerAnalyticsAdapter lines irrelevant.

snapwich · 2017-04-07T20:11:30Z

src/adapters/analytics/ga.js

-      sendBidResponseToGa(bid);
+    utils._each(existingEvents, function (eventObj) {
+      var args = eventObj.args;
+      if (!eventObj) {


it should be noted that these line changes were only from moving existing code around to be inside the if(_sampled) { block above. However, this is a bug; and even though it existed before this pull-request, now is as good as time as any to fix it. Thanks!

…built * 'master' of https://github.com/prebid/Prebid.js: (38 commits) Add optional domain parameter to AdButler adapter (prebid#1078) Send transactionID to Criteo Services (prebid#1113) Fix `buildMasterVideoTagFromAdserverTag()` not selecting winning bid (prebid#1106) Remove placement size selection and filtering (prebid#1107) revert `srcdoc` change (prebid#1130) Add new Adapter- Beachfront Media (prebid#1062) Fixes SpringServe adapter (prebid#1101) Update Widespace request param (prebid#1098) - New Adapter: Innity (prebid#1074) Update Roxot prebid analytic adapter (prebid#1034) Yarn Package Manager (prebid#1109) allow writing into current document if prebid is loaded inside an iframe (prebid#1066) Adapter bug fix (prebid#1096) fix typo added pr review process and governance model (prebid#1103) added support for sampling in ga and base adapter, fixed up some tests (prebid#1011) Add Inneractive adapter (prebid#1048) Add alias freewheel-ssp to stickyadstv bidder adapter (prebid#1043) Add Facebook Audience Network adapter (prebid#1068) Add Atomx support (prebid#1056) ...

prebid#1011) * added support for sampling in ga and base adapter, fixed some tests * added ability to pipe analytics events to customize values before logging * don't mutate original args in analytics adapter pipe function * remove pipe functionality in favor of custom adapter implementation * fixed bug in GA adapter looking for property on potential non-object

added support for sampling in ga and base adapter, fixed some tests

25cc847

mkendall07 added the in progress label Feb 24, 2017

snapwich commented Feb 24, 2017

View reviewed changes

mkendall07 assigned protonate Mar 1, 2017

added ability to pipe analytics events to customize values before log…

407d024

…ging

snapwich force-pushed the analytics-sampling branch from c377321 to 407d024 Compare March 15, 2017 20:37

protonate added needs update and removed in progress labels Mar 27, 2017

don't mutate original args in analytics adapter pipe function

8fdfe47

snapwich force-pushed the analytics-sampling branch from eaece7b to 8fdfe47 Compare March 30, 2017 15:56

snapwich added 2 commits March 30, 2017 10:03

Merge branch 'master' into analytics-sampling

3c7044a

remove pipe functionality in favor of custom adapter implementation

d8578f0

mkendall07 reviewed Mar 30, 2017

View reviewed changes

protonate approved these changes Apr 6, 2017

View reviewed changes

protonate added LGTM and removed needs update labels Apr 6, 2017

grevory reviewed Apr 7, 2017

View reviewed changes

grevory suggested changes Apr 7, 2017

View reviewed changes

fixed bug in GA adapter looking for property on potential non-object

8dc55a7

snapwich commented Apr 7, 2017

View reviewed changes

grevory approved these changes Apr 7, 2017

View reviewed changes

protonate merged commit 2ace03f into prebid:master Apr 7, 2017

patmmccann mentioned this pull request Nov 16, 2020

Missing or difficult to find function documentation prebid/prebid.github.io#2504

Closed

bretg mentioned this pull request Jan 12, 2021

added sampling option to enableAnalytics function prebid/prebid.github.io#2621

Merged

robertrmartinez deleted the analytics-sampling branch July 5, 2023 19:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added support for sampling in ga and base adapter, fixed up some tests #1011

added support for sampling in ga and base adapter, fixed up some tests #1011

snapwich commented Feb 24, 2017 •

edited

Loading

snapwich Feb 24, 2017

snapwich commented Mar 14, 2017

mkendall07 commented Mar 16, 2017

snapwich commented Mar 16, 2017

bretg commented Mar 16, 2017

grevory commented Mar 16, 2017

bretg commented Mar 20, 2017

bretg commented Mar 22, 2017

protonate commented Mar 27, 2017 •

edited

Loading

bretg commented Mar 29, 2017

snapwich commented Mar 30, 2017

snapwich commented Mar 30, 2017

mkendall07 Mar 30, 2017 •

edited

Loading

protonate left a comment

protonate commented Apr 6, 2017

grevory Apr 7, 2017 •

edited

Loading

snapwich Apr 7, 2017

grevory left a comment

grevory Apr 7, 2017

snapwich Apr 7, 2017

grevory Apr 7, 2017

grevory Apr 7, 2017

snapwich left a comment

snapwich Apr 7, 2017

snapwich Apr 7, 2017

added support for sampling in ga and base adapter, fixed up some tests #1011

added support for sampling in ga and base adapter, fixed up some tests #1011

Conversation

snapwich commented Feb 24, 2017 • edited Loading

Type of change

Description of change

Choose a reason for hiding this comment

snapwich commented Mar 14, 2017

mkendall07 commented Mar 16, 2017

snapwich commented Mar 16, 2017

bretg commented Mar 16, 2017

grevory commented Mar 16, 2017

bretg commented Mar 20, 2017

bretg commented Mar 22, 2017

protonate commented Mar 27, 2017 • edited Loading

bretg commented Mar 29, 2017

snapwich commented Mar 30, 2017

snapwich commented Mar 30, 2017

mkendall07 Mar 30, 2017 • edited Loading

Choose a reason for hiding this comment

protonate left a comment

Choose a reason for hiding this comment

protonate commented Apr 6, 2017

grevory Apr 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grevory 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 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snapwich commented Feb 24, 2017 •

edited

Loading

protonate commented Mar 27, 2017 •

edited

Loading

mkendall07 Mar 30, 2017 •

edited

Loading

grevory Apr 7, 2017 •

edited

Loading