Skip to content

feat(track): Introducing easy event tracking #207

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Jan 15, 2019
Merged

feat(track): Introducing easy event tracking #207

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b847a65
Easy Event Tracking
Jan 9, 2019
b6072e2
Update license header years to 2019
Jan 10, 2019
9e7adca
Merge branch 'master' into mcarroll/easy-event-tracking
Jan 11, 2019
1b87b08
Merge branch 'master' into mcarroll/easy-event-tracking
Jan 14, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 3 additions & 16 deletions packages/optimizely-sdk/lib/core/event_builder/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/**
* Copyright 2016-2018, Optimizely
* Copyright 2016-2019, Optimizely
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -57,6 +57,7 @@ function getCommonEventParams(options) {
client_name: options.clientEngine,
client_version: options.clientVersion,
anonymize_ip: anonymize_ip,
enrich_decisions: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

How did we come to the conclusion of using a config in data payload.

I'm guess I'm fine with it, it just seems kind of the wrong place to put it. This seems like something "meta" to the event, like it should go in a header or query param. I get that putting it in a header or query param is probably tough given the architecture of EventDispatcher.

Wondering the thoughts on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For me it was only about the current EventDispatcher not supporting headers or query params, as you said. I agree about using a header being more appropriate and would support that going forward.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 I agree header or QS is more appropriate. We'd need the logtier to be able to handle it though. Have we discussed these alternatives with em?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mikeng13 Yea, we have discussed with logtier, and they are on board with this.

};

// Omit attribute values that are not supported by the log endpoint.
Expand Down Expand Up @@ -115,26 +116,14 @@ function getImpressionEventParams(configObj, experimentId, variationId) {
* @param {Object} configObj Object representing project configuration
* @param {string} eventKey Event key representing the event which needs to be recorded
* @param {Object} eventTags Values associated with the event.
* @param {Array} experimentsToVariationMap Map of experiment IDs to bucketed variation IDs
* @param {Object} logger Logger object
* @return {Object} Conversion event params
*/
function getVisitorSnapshot(configObj, eventKey, eventTags, experimentsToVariationMap, logger) {
function getVisitorSnapshot(configObj, eventKey, eventTags, logger) {
var snapshot = {
decisions: [],
Copy link
Contributor

Choose a reason for hiding this comment

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

is it okay to completely remove decisions vs keeping it as an empty array.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea, the API will treat no decisions the same as an empty decisions array.

events: []
};

fns.forOwn(experimentsToVariationMap, function(variationId, experimentId) {
var decision = {
campaign_id: projectConfig.getLayerId(configObj, experimentId),
experiment_id: experimentId,
variation_id: variationId,
};

snapshot.decisions.push(decision);
});

var eventDict = {
entity_id: projectConfig.getEventId(configObj, eventKey),
timestamp: fns.currentTimestamp(),
Expand Down Expand Up @@ -199,7 +188,6 @@ module.exports = {
* @param {Object} options.configObj Object representing project configuration, including datafile information and mappings for quick lookup
* @param {string} options.eventKey Event key representing the event which needs to be recorded
* @param {Object} options.eventTags Object with event-specific tags
* @param {Object} options.experimentsToVariationMap Map of experiment IDs to bucketed variation IDs
* @param {Object} options.logger Logger object
* @param {string} options.userId ID for user
* @return {Object} Params to be used in conversion event logging endpoint call
Expand All @@ -215,7 +203,6 @@ module.exports = {
var snapshot = getVisitorSnapshot(options.configObj,
options.eventKey,
options.eventTags,
options.experimentsToVariationMap,
options.logger);

commonParams.visitors[0].snapshots = [snapshot];
Expand Down
Loading