feat(track): Introducing easy event tracking by mjc1283 · Pull Request #207 · optimizely/javascript-sdk

mjc1283 · 2019-01-09T19:37:31Z

Summary

Updates track method and event builder to support easy event tracking.

Test plan

Unit tests: updated, and and added new ones
Compatibility suite tests
Manual testing

coveralls · 2019-01-09T19:48:08Z

Coverage decreased (-0.06%) to 97.198% when pulling b847a65 on mcarroll/easy-event-tracking into 7fe2e0e on master.

coveralls · 2019-01-09T19:48:08Z

Coverage decreased (-0.06%) to 97.198% when pulling b847a65 on mcarroll/easy-event-tracking into 7fe2e0e on master.

coveralls · 2019-01-09T19:48:08Z

Coverage decreased (-0.06%) to 97.217% when pulling 1b87b08 on mcarroll/easy-event-tracking into 9812af8 on master.

jordangarcia

Nothing blocking here, just some questions.

jordangarcia · 2019-01-09T19:44:12Z

packages/optimizely-sdk/lib/core/project_config/index.js

+   * @param {string} eventKey
+   * @returns {boolean}
+   */
+  eventWithKeyExists: function(projectConfig, eventKey) {


This is more of a question. Why did we decide to start passing projectConfig to an instance of projectConfig.

Is doing this.eventKeyMap bad?

Also do you think it makes more sense to use the existing projectConfig.getEventId and check the existence vs adding a new API method.

FWIW, I re-wrote projectConfig and use the API method getEventByKey and that's enough for event creation

That's a fair question. Maybe there should be a project config class instead of having all these functions accept a projectConfig. In other SDKs it's done this way. I was following convention here and didn't think much about it.

I think using getEventId is a little less clear than adding a new method, but don't feel strongly about it.

Yeah not married to this pattern. I'd support refactoring this class to house those functions eventually.

jordangarcia · 2019-01-09T19:45:48Z

packages/optimizely-sdk/lib/optimizely/index.js

-          eventKey));
-        return;
+      if (!projectConfig.eventWithKeyExists(this.configObj, eventKey)) {
+        throw new Error(sprintf(ERROR_MESSAGES.INVALID_EVENT_KEY, MODULE_NAME, eventKey));


Another question: is giving an invalid eventKey actually an error?

Another fair question that I wasn't considering here. It was being treated as an error previously, and I don't want to change that behavior as part of this PR.

jordangarcia · 2019-01-09T19:47:50Z

packages/optimizely-sdk/lib/core/event_builder/index.js

    client_name: options.clientEngine,
    client_version: options.clientVersion,
    anonymize_ip: anonymize_ip,
+    enrich_decisions: true,


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.

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.

👍 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?

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

jordangarcia · 2019-01-09T19:48:04Z

packages/optimizely-sdk/lib/core/event_builder/index.js

-function getVisitorSnapshot(configObj, eventKey, eventTags, experimentsToVariationMap, logger) {  
+function getVisitorSnapshot(configObj, eventKey, eventTags, logger) {
  var snapshot = {
-    decisions: [],


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

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

aliabbasrizvi

Changes look good. @jordangarcia and @mikeng13 should take a look as well.

aliabbasrizvi · 2019-01-09T20:56:04Z

nit. You should update copyright year in every file that you are updating.

mikeproeng37 · 2019-01-10T18:27:10Z

packages/optimizely-sdk/lib/core/event_builder/index.js

    client_name: options.clientEngine,
    client_version: options.clientVersion,
    anonymize_ip: anonymize_ip,
+    enrich_decisions: true,


👍 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?

mikeproeng37 · 2019-01-10T18:29:35Z

packages/optimizely-sdk/lib/core/project_config/index.js

+   * @param {string} eventKey
+   * @returns {boolean}
+   */
+  eventWithKeyExists: function(projectConfig, eventKey) {


Yeah not married to this pattern. I'd support refactoring this class to house those functions eventually.

mikeproeng37 · 2019-01-10T18:29:57Z

packages/optimizely-sdk/lib/utils/fns/index.js

  forEach: require('lodash/forEach'),
  forOwn: require('lodash/forOwn'),
  map: require('lodash/map'),
-  reduce: require('lodash/reduce'),


Good riddance!

mikeproeng37

lgtm

aliabbasrizvi · 2019-01-14T21:44:54Z

Triggered tests for this branch: https://travis-ci.com/optimizely/fullstack-sdk-compatibility-suite/builds/97304511
Looks like we need to look at 2 tests.

aliabbasrizvi · 2019-01-15T01:22:06Z

All tests passed: https://travis-ci.com/optimizely/fullstack-sdk-compatibility-suite/builds/97304511
Merging change

Easy Event Tracking

b847a65

mjc1283 requested review from aliabbasrizvi, jordangarcia and mikeproeng37 January 9, 2019 19:37

jordangarcia reviewed Jan 9, 2019

View reviewed changes

aliabbasrizvi approved these changes Jan 9, 2019

View reviewed changes

Update license header years to 2019

b6072e2

mikeproeng37 reviewed Jan 10, 2019

View reviewed changes

Merge branch 'master' into mcarroll/easy-event-tracking

9e7adca

mikeproeng37 approved these changes Jan 14, 2019

View reviewed changes

Merge branch 'master' into mcarroll/easy-event-tracking

1b87b08

aliabbasrizvi merged commit fe6916f into master Jan 15, 2019

aliabbasrizvi deleted the mcarroll/easy-event-tracking branch January 15, 2019 01:41

Conversation

mjc1283 commented Jan 9, 2019

Summary

Test plan

Uh oh!

coveralls commented Jan 9, 2019

Uh oh!

coveralls commented Jan 9, 2019

Uh oh!

coveralls commented Jan 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jordangarcia left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aliabbasrizvi left a comment

Choose a reason for hiding this comment

Uh oh!

aliabbasrizvi commented Jan 9, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikeproeng37 left a comment

Choose a reason for hiding this comment

Uh oh!

aliabbasrizvi commented Jan 14, 2019

Uh oh!

aliabbasrizvi commented Jan 15, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

coveralls commented Jan 9, 2019 •

edited

Loading