Make some analytics call conditional by zikas · Pull Request #9061 · ampproject/amphtml

zikas · 2017-05-01T06:54:31Z

Added the ability to fire analytics triggers conditional based on enabled property that can be specified in individual triggers and at the root level.

FIXES #4752

…7983

…onditions

avimehta · 2017-05-02T17:24:15Z

extensions/amp-analytics/0.1/amp-analytics.js

-        this.sendRequest_(request, trigger);
-        return request;
-      });
+    return this.checkTriggerEnabled_(trigger, event)


would it make more sense to check for enabled before we do the url expansion. That way we prevent some work that might not be used.

avimehta · 2017-05-02T17:25:18Z

extensions/amp-analytics/0.1/amp-analytics.js

  }

+  /**
+   * Checks if request for a trigger is enabled.


s/sampling/if the trigger is enabled

avimehta · 2017-05-02T17:30:05Z

extensions/amp-analytics/0.1/amp-analytics.js

+  }
+
+  /**
+   * Checks result of 'enabled' spec evaluation. Returns false if spec is provided and value


it returns false for all falsey values. right? (eg: 0, null and so on)

Method itself always returns Promise, so it's real true/false all the time.

In current implementation false is returned only if the result of a spec expansion is an empty string (assuming that url expansion always returns string, the only falsey value is '').
Should I change this and treat all falsey-looking strings ('0', 'false', 'undefined', 'null, 'NaN'') as false?

I think we should go ahead and change this, yes. The only more desirable alternative is to change var/url-replacements expansion with a new mode that will take false-y values and instead of outputting them via String(x) would instead output them as an empty string. However, that'd be a lot of changes for a relatively small feature. So, in the meantime, using falsy strings is a good alternative, I believe.

avimehta · 2017-05-02T17:31:50Z

extensions/amp-analytics/0.1/test/test-amp-analytics.js

+      };
+    }
+
+    it('allows a request through', () => {


allows a request through for undefined `enabled` property

avimehta · 2017-05-02T17:33:52Z

extensions/amp-analytics/0.1/test/test-amp-analytics.js

+      const analytics = getAnalyticsTag(config);
+
+      const urlReplacements = urlReplacementsForDoc(analytics.element);
+      sandbox.stub(urlReplacements.getVariableSource(), 'get').returns(0);


this is interesting. Does the 0 get converted to a string? how does the tigger get enabled for this case?

The result of expandUrlAsync is always a string because it gets encoded every time, that's the reason 0 (that gets translated to '0') is treated as true

Knowing the fact that URL replacements does conversion to strings, we still can fairly easily standardize on what strings lead to false, including 0 and false.

avimehta · 2017-05-02T17:35:24Z

extensions/amp-analytics/0.1/test/test-amp-analytics.js

+      });
+    });
+  });
+


Can you add a test or two for the case where both tag level and trigger level properties are specified?

zikas · 2017-05-02T22:07:54Z

ptal

dvoytenko · 2017-05-03T17:14:19Z

extensions/amp-analytics/0.1/amp-analytics.js

+          if (!enabled) {
+            return;
+          }
+          return this.expandExtraUrlParams_(trigger, event)


Could you move this into a separate method, e.g. expandAndSendRequest_?

dvoytenko · 2017-05-03T17:38:40Z

extensions/amp-analytics/0.1/amp-analytics.js

+  }
+
+  /**
+   * Checks result of 'enabled' spec evaluation. Returns false if spec is provided and value


I think we should go ahead and change this, yes. The only more desirable alternative is to change var/url-replacements expansion with a new mode that will take false-y values and instead of outputting them via String(x) would instead output them as an empty string. However, that'd be a lot of changes for a relatively small feature. So, in the meantime, using falsy strings is a good alternative, I believe.

dvoytenko · 2017-05-03T17:39:58Z

@zikas Thanks a lot for the PR! This looks very good. I just added a couple of comments, most importantly around the point you've made on falsy strings.

zikas

@dvoytenko No problem, all done. Thank you for the review!

zikas · 2017-05-03T21:34:14Z

extensions/amp-analytics/0.1/test/test-amp-analytics.js

+      });
+    });
+  });
+


avimehta

just a few small changes. Mostly lgtm from my side.

avimehta · 2017-05-04T20:14:06Z

extensions/amp-analytics/0.1/amp-analytics.js

+   * @param {string} request The request to process.
+   * @param {!JSONType} trigger JSON config block that resulted in this event.
+   * @param {!Object} event Object with details about the event.
+   * @return {!Promise<string|undefined>} The request that was sent out.


I believe type is always !Promise<string> here. no?

avimehta · 2017-05-04T20:14:39Z

extensions/amp-analytics/0.1/amp-analytics.js

-    InstrumentationService,
-    instrumentationServicePromiseForDoc,
-    AnalyticsEventType,
+  InstrumentationService,


nit: were the indentation changes automatically generated?

Yeah, I guess this is my ide. Lint accepts either way. I rolled back unrelated indentation changes.

dvoytenko · 2017-05-05T16:02:28Z

extensions/amp-analytics/0.1/amp-analytics.js

+      const expansionOptions = this.expansionOptions_({}, trigger);
+      return this.expandTemplateWithUrlParams_(sampleOn, expansionOptions)
          .then(key => this.cryptoService_.uniform(key))
          .then(digest => digest * 100 < spec['threshold']);


Could you please change here spec['threshold'] to threshold from above const threshold?

dvoytenko

One small request and LGTM. Thanks!

…onditions

dvoytenko · 2017-05-05T18:53:12Z

Thanks again! Looks like everything is green. Merging...

zikas added 6 commits April 26, 2017 14:15

AMP Analytics: Make the aip parameter changeable to false ampproject#…

1a466fa

…7983

Added tests

105d35b

Ability to disable trigger

5aaa68e

Merge branch 'master' of https://github.com/ampproject/amphtml into c…

b498de3

…onditions

rolled back indentation changes

be1c54e

Renamed some tests

66d5a20

zikas changed the title ~~Conditions~~ Make some analytics call conditional May 1, 2017

avimehta self-requested a review May 1, 2017 17:25

avimehta added the WG: analytics label May 1, 2017

avimehta assigned zikas May 1, 2017

avimehta requested a review from dvoytenko May 1, 2017 17:25

zikas added 4 commits May 1, 2017 10:37

lint fixes

e4c3177

lint errors

3027c58

lint fixes

a4841cd

fixed type declarations

c9592b2

avimehta reviewed May 2, 2017

View reviewed changes

avimehta suggested changes May 2, 2017

View reviewed changes

zikas added 4 commits May 2, 2017 12:54

fixed comments

cde8d7e

updated jsdoc

9c2813d

updated jsdoc

e35545c

lint

18d4fea

googlebot added the cla: yes label May 2, 2017

dvoytenko mentioned this pull request May 3, 2017

Make some analytics call conditional #4752

Closed

dvoytenko suggested changes May 3, 2017

View reviewed changes

zikas added 2 commits May 3, 2017 12:30

Falsey values

f4a091f

jsdoc updated

2ee4252

zikas commented May 3, 2017

View reviewed changes

extensions/amp-analytics/0.1/test/test-amp-analytics.js

});

});

});

Copy link
Copy Markdown

Contributor Author

zikas May 3, 2017

Choose a reason for hiding this comment

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

Done

avimehta suggested changes May 4, 2017

View reviewed changes

dvoytenko reviewed May 5, 2017

View reviewed changes

dvoytenko approved these changes May 5, 2017

View reviewed changes

zikas added 5 commits May 5, 2017 11:12

code review changes

ef6cf80

Merge branch 'master' of https://github.com/ampproject/amphtml into c…

4493be4

…onditions

code review changes

998ac7a

rolled back indentation

95c971c

rolled back indentation

fc7677d

avimehta approved these changes May 5, 2017

View reviewed changes

dvoytenko merged commit 9ad4aa5 into ampproject:master May 5, 2017

ampprojectbot added the PR use: In Beta / Experimental label May 11, 2017

Conversation

zikas commented May 1, 2017 • edited by avimehta Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

zikas commented May 2, 2017

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!

dvoytenko commented May 3, 2017

Uh oh!

zikas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

avimehta 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!

dvoytenko left a comment

Choose a reason for hiding this comment

Uh oh!

dvoytenko commented May 5, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

zikas commented May 1, 2017 •

edited by avimehta

Loading