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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃搱 AmpStoryEventTracker & Integration tests #24548

Merged
merged 11 commits into from Sep 18, 2019
2 changes: 1 addition & 1 deletion extensions/amp-analytics/0.1/analytics-root.js
Expand Up @@ -201,7 +201,7 @@ export class AnalyticsRoot {
* has not been requested before, it will be created.
*
* @param {string} name
* @param {function(new:./events.CustomEventTracker, !AnalyticsRoot)|function(new:./events.ClickEventTracker, !AnalyticsRoot)|function(new:./events.ScrollEventTracker, !AnalyticsRoot)|function(new:./events.SignalTracker, !AnalyticsRoot)|function(new:./events.IniLoadTracker, !AnalyticsRoot)|function(new:./events.VideoEventTracker, !AnalyticsRoot)|function(new:./events.VideoEventTracker, !AnalyticsRoot)|function(new:./events.VisibilityTracker, !AnalyticsRoot)} klass
* @param {function(new:./events.CustomEventTracker, !AnalyticsRoot)|function(new:./events.ClickEventTracker, !AnalyticsRoot)|function(new:./events.ScrollEventTracker, !AnalyticsRoot)|function(new:./events.SignalTracker, !AnalyticsRoot)|function(new:./events.IniLoadTracker, !AnalyticsRoot)|function(new:./events.VideoEventTracker, !AnalyticsRoot)|function(new:./events.VideoEventTracker, !AnalyticsRoot)|function(new:./events.VisibilityTracker, !AnalyticsRoot)|function(new:./events.AmpStoryEventTracker, !AnalyticsRoot)} klass
* @return {!./events.EventTracker}
*/
getTracker(name, klass) {
Expand Down
93 changes: 93 additions & 0 deletions extensions/amp-analytics/0.1/events.js
Expand Up @@ -50,6 +50,7 @@ export const AnalyticsEventType = {
INI_LOAD: 'ini-load',
RENDER_START: 'render-start',
SCROLL: 'scroll',
STORY: 'story',
TIMER: 'timer',
VIDEO: 'video',
VISIBLE: 'visible',
Expand Down Expand Up @@ -109,6 +110,13 @@ const TRACKER_TYPE = Object.freeze({
return new ScrollEventTracker(root);
},
},
[AnalyticsEventType.STORY]: {
name: AnalyticsEventType.STORY,
allowedFor: ALLOWED_FOR_ALL_ROOT_TYPES,
klass: function(root) {
return new AmpStoryEventTracker(root);
},
},
[AnalyticsEventType.TIMER]: {
name: AnalyticsEventType.TIMER,
allowedFor: ALLOWED_FOR_ALL_ROOT_TYPES,
Expand All @@ -135,6 +143,14 @@ const TRACKER_TYPE = Object.freeze({
/** @visibleForTesting */
export const trackerTypeForTesting = TRACKER_TYPE;

/**
* @param {string} triggerType
* @return {boolean}
*/
function isAmpStoryTriggerType(triggerType) {
return startsWith(triggerType, 'story');
}

/**
* @param {string} triggerType
* @return {boolean}
Expand All @@ -159,6 +175,9 @@ export function getTrackerKeyName(eventType) {
if (isVideoTriggerType(eventType)) {
return AnalyticsEventType.VIDEO;
}
if (isAmpStoryTriggerType(eventType)) {
return AnalyticsEventType.STORY;
}
if (!isReservedTriggerType(eventType)) {
return AnalyticsEventType.CUSTOM;
}
Expand Down Expand Up @@ -386,6 +405,80 @@ export class CustomEventTracker extends EventTracker {
}
}

// TODO(Enriqe): If needed, add support for sandbox story event.
// (e.g. sandbox-story-xxx).
export class AmpStoryEventTracker extends CustomEventTracker {
/**
* @param {!./analytics-root.AnalyticsRoot} root
*/
constructor(root) {
super(root);
}

/** @override */
add(context, eventType, config, listener) {
// TODO(Enriqe): add support for storySpec.
const rootTarget = this.root.getRootElement();

// Fire buffered events if any.
const buffer = this.buffer_ && this.buffer_[eventType];
if (buffer) {
const bufferLength = buffer.length;

for (let i = 0; i < bufferLength; i++) {
const event = buffer[i];
this.fireListener_(event, rootTarget, config, listener);
}
}

let observables = this.observables_[eventType];
if (!observables) {
observables = new Observable();
this.observables_[eventType] = observables;
}

return this.observables_[eventType].add(event => {
this.fireListener_(event, rootTarget, config, listener);
});
}

/**
* Fires listener given the specified configuration.
* @param {!AnalyticsEvent} event
* @param {!Element} rootTarget
* @param {!JsonObject} config

* @param {function(!AnalyticsEvent)} listener
*/
fireListener_(event, rootTarget, config, listener) {
Copy link
Contributor

Choose a reason for hiding this comment

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

To make sure I understand: we're adding a "fireListener" method to be able to maybe not fire duplicate events depending on the configuration?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, that's right. This is going to make it easier for when we introduce the configurable options and control when we fire events or not.

const type = event['type'];
const vars = event['vars'];

listener(new AnalyticsEvent(rootTarget, type, vars));
}

/**
* Triggers a custom event for the associated root, or buffers them if the
* observables aren't present yet.
* @param {!AnalyticsEvent} event
*/
trigger(event) {
const eventType = event['type'];
const observables = this.observables_[eventType];

// If listeners already present - trigger right away.
if (observables) {
observables.fire(event);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

sandboxEvent is added if an analytics event is added by an AMP component dynamically, in the format of sandbox-xxx.
In those case, the event won't be recognized as a story event because the name doesn't start with story-. Would that be an issue?
Code wise, it's fine extending the CustomEventTracker class, as no event would ever fall into the this.sandboxBuffer_ here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To make sure I understand: sandboxEvent is emitted by an arbitrary AMP component?

If that's the case then it should still go to the CustomEventTracker, right? So not intercepting sandbox-* events here seems like the right behavior.

Let me know if I'm missing something.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's right. All sandbox events will be handled by the CustomEventTracker.
It won't be an issue for now, but it could be an issue if a component trigger a sandbox story event in the future. sandbox-story-xxx. It would be great to keep that in mind by adding a comment.

// Create buffer and enqueue event if needed.
if (this.buffer_) {
this.buffer_[eventType] = this.buffer_[eventType] || [];
this.buffer_[eventType].push(event);
}
}
}

/**
* Tracks click events.
*/
Expand Down
28 changes: 24 additions & 4 deletions extensions/amp-analytics/0.1/instrumentation.js
Expand Up @@ -14,8 +14,14 @@
* limitations under the License.
*/

import {
AmpStoryEventTracker,
AnalyticsEvent,
AnalyticsEventType,
CustomEventTracker,
getTrackerKeyName,
} from './events';
import {AmpdocAnalyticsRoot, EmbedAnalyticsRoot} from './analytics-root';
import {AnalyticsEvent, AnalyticsEventType, CustomEventTracker} from './events';
import {AnalyticsGroup} from './analytics-group';
import {Services} from '../../../src/services';
import {getFriendlyIframeEmbedOptional} from '../../../src/iframe-helper';
Expand Down Expand Up @@ -67,6 +73,19 @@ export class InstrumentationService {
return new AnalyticsGroup(root, analyticsElement);
}

/**
* @param {string} trackerName
* @private
*/
getTrackerClass_(trackerName) {
switch (trackerName) {
case AnalyticsEventType.STORY:
return AmpStoryEventTracker;
default:
return CustomEventTracker;
}
}

/**
* Triggers the analytics event with the specified type.
*
Expand All @@ -77,9 +96,10 @@ export class InstrumentationService {
triggerEventForTarget(target, eventType, opt_vars) {
const event = new AnalyticsEvent(target, eventType, opt_vars);
const root = this.findRoot_(target);
const tracker = /** @type {!CustomEventTracker} */ (root.getTracker(
AnalyticsEventType.CUSTOM,
CustomEventTracker
const trackerName = getTrackerKeyName(eventType);
const tracker = /** @type {!CustomEventTracker|!AmpStoryEventTracker} */ (root.getTracker(
trackerName,
this.getTrackerClass_(trackerName)
));
tracker.trigger(event);
}
Expand Down
149 changes: 148 additions & 1 deletion extensions/amp-analytics/0.1/test/test-events.js
Expand Up @@ -15,8 +15,8 @@
*/

import * as lolex from 'lolex';
import {AmpdocAnalyticsRoot} from '../analytics-root';
import {
AmpStoryEventTracker,
AnalyticsEvent,
AnalyticsEventType,
ClickEventTracker,
Expand All @@ -28,6 +28,7 @@ import {
VisibilityTracker,
trackerTypeForTesting,
} from '../events';
import {AmpdocAnalyticsRoot} from '../analytics-root';
import {Deferred} from '../../../../src/utils/promise';
import {Signals} from '../../../../src/utils/signals';
import {macroTask} from '../../../../testing/yield';
Expand Down Expand Up @@ -657,6 +658,152 @@ describes.realWin('Events', {amp: 1}, env => {
});
});

describe('AmpStoryEventTracker', () => {
let tracker;
let clock;
let getRootElementSpy;
let rootTarget;

beforeEach(() => {
clock = sandbox.useFakeTimers();
tracker = root.getTracker(AnalyticsEventType.STORY, AmpStoryEventTracker);
rootTarget = root.getRootElement();
getRootElementSpy = sandbox.spy(root, 'getRootElement');
});

it('should initalize, add listeners, and dispose', () => {
expect(tracker.root).to.equal(root);
expect(tracker.buffer_).to.exist;

tracker.dispose();
expect(tracker.buffer_).to.not.exist;
});

it('should listen on story events', () => {
const handler2 = sandbox.spy();
tracker.add(analyticsElement, 'story-event-1', {}, handler);
tracker.add(analyticsElement, 'story-event-2', {}, handler2);
tracker.trigger(new AnalyticsEvent(target, 'story-event-1'));
expect(getRootElementSpy).to.be.calledTwice;

expect(handler).to.be.calledOnce;
expect(handler2).to.have.not.been.called;
tracker.trigger(new AnalyticsEvent(target, 'story-event-2'));

expect(handler).to.be.calledOnce;
expect(handler2).to.be.calledOnce;
tracker.trigger(new AnalyticsEvent(target, 'story-event-1'));

expect(handler).to.have.callCount(2);
expect(handler2).to.be.calledOnce;
});

it('should buffer story events early on', () => {
// Events before listeners added.
tracker.trigger(new AnalyticsEvent(target, 'story-event-1'));
tracker.trigger(new AnalyticsEvent(target, 'story-event-2'));
tracker.trigger(new AnalyticsEvent(target, 'story-event-2'));
expect(tracker.buffer_['story-event-1']).to.have.length(1);
expect(tracker.buffer_['story-event-2']).to.have.length(2);

// Listeners added: immediate events fired.
const handler2 = sandbox.spy();
const handler3 = sandbox.spy();
tracker.add(analyticsElement, 'story-event-1', {}, handler);
tracker.add(analyticsElement, 'story-event-2', {}, handler2);
tracker.add(
analyticsElement,
'story-event-3',
{on: 'story-event-3'},
handler3
);

expect(handler).to.be.calledOnce;
expect(handler2).to.have.callCount(2);
expect(handler3).to.have.not.been.called;
expect(tracker.buffer_['story-event-1']).to.have.length(1);
expect(tracker.buffer_['story-event-2']).to.have.length(2);
expect(tracker.buffer_['story-event-3']).to.be.undefined;

// Second round of events.
tracker.trigger(new AnalyticsEvent(target, 'story-event-1'));
tracker.trigger(new AnalyticsEvent(target, 'story-event-2'));
tracker.trigger(new AnalyticsEvent(target, 'story-event-3'));
expect(getRootElementSpy).to.have.callCount(3);

expect(handler).to.have.callCount(2);
expect(handler2).to.have.callCount(3);
expect(handler3).to.be.calledOnce;
expect(tracker.buffer_['story-event-1']).to.have.length(2);
expect(tracker.buffer_['story-event-2']).to.have.length(3);
expect(tracker.buffer_['story-event-3']).to.have.length(1);

// Buffering time expires.
clock.tick(10001);
expect(tracker.buffer_).to.be.undefined;

// Post-buffering round of events.
tracker.trigger(new AnalyticsEvent(target, 'story-event-1'));
tracker.trigger(new AnalyticsEvent(target, 'story-event-2'));
tracker.trigger(new AnalyticsEvent(target, 'story-event-3'));

expect(handler).to.have.callCount(3);
expect(handler2).to.have.callCount(4);
expect(handler3).to.have.callCount(2);
expect(tracker.buffer_).to.be.undefined;
});

it('should not fire twice from observable and buffer', () => {
tracker.trigger(
new AnalyticsEvent(target, 'story-event-1', {'order': '1'})
);
tracker.add(target, 'story-event-1', {}, handler);

tracker.trigger(
new AnalyticsEvent(target, 'story-event-1', {'order': '2'})
);

expect(handler).to.have.callCount(2);
expect(handler.firstCall).to.be.calledWith(
new AnalyticsEvent(rootTarget, 'story-event-1', {'order': '1'})
);
expect(handler.secondCall).to.be.calledWith(
new AnalyticsEvent(rootTarget, 'story-event-1', {'order': '2'})
);
});

it('should handle all events without duplicate trigger', () => {
tracker.trigger(
new AnalyticsEvent(target, 'story-event-1', {'order': '1'})
);
tracker.trigger(
new AnalyticsEvent(target, 'story-event-1', {'order': '2'})
);
tracker.add(analyticsElement, 'story-event-1', {}, handler);

tracker.trigger(
new AnalyticsEvent(target, 'story-event-1', {'order': '3'})
);
tracker.trigger(
new AnalyticsEvent(target, 'story-event-1', {'order': '4'})
);

expect(handler).to.have.callCount(4);
expect(handler.firstCall).to.be.calledWith(
new AnalyticsEvent(rootTarget, 'story-event-1', {'order': '1'})
);
expect(handler.secondCall).to.be.calledWith(
new AnalyticsEvent(rootTarget, 'story-event-1', {'order': '2'})
);
expect(handler.thirdCall).to.be.calledWith(
new AnalyticsEvent(rootTarget, 'story-event-1', {'order': '3'})
);
expect(handler.lastCall).to.be.calledWith(
new AnalyticsEvent(rootTarget, 'story-event-1', {'order': '4'})
);
});
});

describe('SignalTracker', () => {
let tracker;
let targetSignals;
Expand Down