Add extraUrlParams and extraUrlParamsReplaceMap to amp-analytics #1932

jmadler · 2016-02-11T03:39:29Z

Implements #1881

jmadler · 2016-02-11T03:40:00Z

cc @rudygalfi @avimehta @btownsend @cramforce

cramforce · 2016-02-11T04:05:17Z

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

+      var i = 0;
+      for (const keyrule in this.config_['customVarsMap']) {
+        var replacerule = this.config_['customVarsMap'][keyrule];
+        alert ("keyrule = " + keyrule + ";replacerule = " + replacerule);


I'd recommend using console.log :)

Will remove - that was for debugging

cramforce · 2016-02-11T04:10:10Z

Looks great. Just some simple questions and nits.

Run gulp lint and gulp presubmit to fix the lint issues.

avimehta · 2016-02-11T05:00:42Z

who are we building this for? can you point to some examples on who would use/need these?

jmadler · 2016-02-11T06:02:37Z

This is for Adobe Analytics

avimehta · 2016-02-11T06:33:43Z

I see. a few general concerns:

Having variables defined like this and then the vendor renaming them in the config looks like a weird contract between two parties. I think @cramforce already asked this and you gave an answer but why wouldn't a publisher simply specify the parameter name in their config? Isn't specifying just the parameter name and value easier, more flexible?

Taking a cursory look at adobe's site, it looks like the variables they use in the request are literally of the format:
```
prop31:en-us
prop32:en-us:adobe.com:marketing-cloud:web-analytics
prop34:9
eVar37:D=oid
eVar38:D=pid
```
Given that, I am not sure if the customVarsMap part of this PR is needed. Won't just the vendor config part of your PR suffice?
Custom Variables is actually used by multiple products and it means slightly different thing in each different product. So GA users might try to use this but will not see the results they would like. Same for piwik and some other products out there.
I will prefer a solution where the hit building/parameter config is more descriptive. For example, if we decide to do Support POSTing JSON with amp-analytics #1798 and Provide ability to conditionally append key-value pair in amp-analytics #1701, will this work with that? To some extent, I feel that solving Provide ability to conditionally append key-value pair in amp-analytics #1701 is a superset of this feature.

cramforce · 2016-02-11T16:51:21Z

Maybe customParams is a better name, as in: It literally (with the exception of the replace) goes into the URL.

I second Avi's concern about the replace being really weird, since people could just use the output instead. But I'm happy to make things easy for Adobe customers.

jmadler · 2016-02-11T17:11:52Z

In the case of Adobe, the key names in these key/val pairs are different in different contexts. For example, s.evar0 is sometimes called v0. Adobe (and other analytics providers) should be able to have that sort of flexibility in AMP Analytics. I understand that this additional ease of use comes at a cost of reduced performance, but note that the performance impact for analytics configs not using it is a one-time check of two vars, and the entire routine is skipped if the config doesn't exist. In terms of performance impact for those using analytics vendors that enable this routine, this O(n * m) routine runs once per page load and has a maximum "m" of 8, and an unlimited "n". I'm happy to add a limit to "n" (customVars) to minimize the potential upper bound of performance impact in these cases. I'd suggest 128.
We can rename to customParams or extraParams or whatever you like best. I have no opinion. Just let me know which is best for AMP Analytics.
Provide ability to conditionally append key-value pair in amp-analytics #1701 and Support POSTing JSON with amp-analytics #1798 seems to me to be orthogonal to this PR. Based on this PR, Provide ability to conditionally append key-value pair in amp-analytics #1701 could be a config variable to omit vars and/or extraParams if the value is null; Support POSTing JSON with amp-analytics #1798 would be a config variable to specify the verb to use (POST, GET, HEAD, et al). I'm happy to implement Provide ability to conditionally append key-value pair in amp-analytics #1701, btw.

cramforce · 2016-02-11T17:18:15Z

I'm not worried about runtime performance. Code size and complexity is the only worry.

Lets call them extraUrlParams

jmadler · 2016-02-11T18:14:02Z

I did:

renamed to extraUrlParams / extraUrlParamsReplaceMap
removed debugging shims
moved regex outside of the inner loop to the outer loop to reduce duplication of effort
rename variables and set scope appropriately throughout (var => let/const)
Ran gulp lint and gulp presubmit and corrected issues

I did not:

Remove regex sanitation

PTAL

avimehta · 2016-02-11T18:38:40Z

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

+        }
+
+        // only allow word chars
+        replaceMapKey = extraUrlParamsReplaceMapKey.replace(/(\W)/g, '\$1');


not sure what extraUrlParamsReplaceMapKey is.

good catch. corrected and tested.

avimehta · 2016-02-11T18:46:42Z

agreed with @cramforce on code complexity. Plus I still feel that this is too specific a solution and that #1701 is a more generic replacement of this.

Having said that, I am still not completely sure about how this is going to be used. I am not sure there will be multiple vendor contexts. Is adobe going to create multiple types in vendors.js?

Since I don't understand the usecase, I think I am okay with committing this and keeping an eye on how this will be used.

jmadler · 2016-02-11T19:20:49Z

I've given this some thought and I think, more broadly, the need for the extraUrlParams is introduced because the request URLs have the variables built into the string.

Rather than push this one through, and adding that as technical debt for later, let's grab some time to brainstorm the right approach and move forward appropriately.

cramforce · 2016-02-11T19:23:49Z

I have the same questions as @avimehta, but I also want to get this resolved quickly. Main question is whether the replace list itself needs to be configured by clients or is there a single one.

I think the extraUrlParams feature is super useful and should definitely land.

avimehta · 2016-02-11T19:35:35Z

talked offline. Iets continue with this PR. I don't think we are painting ourselves is a bad corner with this. Overall, this is a positive this for analytics and should be done.

Jordan has some great ideas about how to make requests more flexible. Lets talk about those outside of this PR.

jmadler · 2016-02-11T23:29:26Z

Final revisions made. PTAL @cramforce before merge

cramforce · 2016-02-11T23:33:15Z

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

+      // of params to String.replace to allow aliasing of the keys in
+      // extraUrlParams.
+      let count = 0;
+      const MAX_REPLACES = 16;


This should be defined near the top of the file and come with an explanation.

cramforce · 2016-02-11T23:33:32Z

One comment. Please rebase.

jmadler · 2016-02-11T23:42:53Z

fixed & rebased

cramforce · 2016-02-11T23:45:43Z

Rebasing appears to not have been successful. Maybe you forgot to pull down master?

jridgewell · 2016-02-11T23:45:58Z

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

+        }
+
+        // only allow word chars
+        replaceMapKey = replaceMapKey.replace(/(\W)/g, '\$1');


What is this preventing?

early iterations treated the key as a regex. It's now just a string, though. I'll remove the line as it's irrelevant now.

cramforce · 2016-02-12T01:12:11Z

LGTM

Add extraUrlParams and extraUrlParamsReplaceMap to amp-analytics

jmadler changed the title ~~Add customVars and customVarsKeyReplace to amp-analytics~~ Add customVars and customVarsMap to amp-analytics Feb 11, 2016

cramforce reviewed Feb 11, 2016
View reviewed changes

jmadler force-pushed the analytics-customvars branch from c337838 to 66337b9 Compare February 11, 2016 18:13

jmadler changed the title ~~Add customVars and customVarsMap to amp-analytics~~ Add extraUrlParams and extraUrlParamsReplaceMap to amp-analytics Feb 11, 2016

avimehta reviewed Feb 11, 2016
View reviewed changes

jmadler force-pushed the analytics-customvars branch from 66337b9 to d572fe2 Compare February 11, 2016 23:28

cramforce reviewed Feb 11, 2016
View reviewed changes

jmadler force-pushed the analytics-customvars branch from d572fe2 to 27339f7 Compare February 11, 2016 23:42

jridgewell reviewed Feb 11, 2016
View reviewed changes

jmadler force-pushed the analytics-customvars branch 2 times, most recently from 8864386 to 409bb62 Compare February 12, 2016 00:47

Add extraUrlParams and extraUrlParamsReplaceMap to amp-analytics

42726fc

jmadler force-pushed the analytics-customvars branch from 409bb62 to 42726fc Compare February 12, 2016 01:01

cramforce added a commit that referenced this pull request Feb 12, 2016

Merge pull request #1932 from jmadler/analytics-customvars

7353601

Add extraUrlParams and extraUrlParamsReplaceMap to amp-analytics

cramforce merged commit 7353601 into ampproject:master Feb 12, 2016

jmadler deleted the analytics-customvars branch February 12, 2016 03:10

zhouyx mentioned this pull request Sep 7, 2018

amp-analytics: Added support for extraUrlParamsReplaceMap defined in a trigger block. #14233

Open

mmmicedcoffee mentioned this pull request Oct 13, 2021

SwG Release 0.1.22.188 #36346

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add extraUrlParams and extraUrlParamsReplaceMap to amp-analytics #1932

Add extraUrlParams and extraUrlParamsReplaceMap to amp-analytics #1932

jmadler commented Feb 11, 2016

jmadler commented Feb 11, 2016

cramforce Feb 11, 2016

jmadler Feb 11, 2016

cramforce commented Feb 11, 2016

avimehta commented Feb 11, 2016

jmadler commented Feb 11, 2016

avimehta commented Feb 11, 2016

cramforce commented Feb 11, 2016

jmadler commented Feb 11, 2016

cramforce commented Feb 11, 2016

jmadler commented Feb 11, 2016

avimehta Feb 11, 2016

jmadler Feb 11, 2016

avimehta commented Feb 11, 2016

jmadler commented Feb 11, 2016

cramforce commented Feb 11, 2016

avimehta commented Feb 11, 2016

jmadler commented Feb 11, 2016

cramforce Feb 11, 2016

cramforce commented Feb 11, 2016

jmadler commented Feb 11, 2016

cramforce commented Feb 11, 2016

jridgewell Feb 11, 2016

jmadler Feb 11, 2016

cramforce commented Feb 12, 2016

Add extraUrlParams and extraUrlParamsReplaceMap to amp-analytics #1932

Add extraUrlParams and extraUrlParamsReplaceMap to amp-analytics #1932

Conversation

jmadler commented Feb 11, 2016

jmadler commented Feb 11, 2016

cramforce Feb 11, 2016

Choose a reason for hiding this comment

jmadler Feb 11, 2016

Choose a reason for hiding this comment

cramforce commented Feb 11, 2016

avimehta commented Feb 11, 2016

jmadler commented Feb 11, 2016

avimehta commented Feb 11, 2016

cramforce commented Feb 11, 2016

jmadler commented Feb 11, 2016

cramforce commented Feb 11, 2016

jmadler commented Feb 11, 2016

avimehta Feb 11, 2016

Choose a reason for hiding this comment

jmadler Feb 11, 2016

Choose a reason for hiding this comment

avimehta commented Feb 11, 2016

jmadler commented Feb 11, 2016

cramforce commented Feb 11, 2016

avimehta commented Feb 11, 2016

jmadler commented Feb 11, 2016

cramforce Feb 11, 2016

Choose a reason for hiding this comment

cramforce commented Feb 11, 2016

jmadler commented Feb 11, 2016

cramforce commented Feb 11, 2016

jridgewell Feb 11, 2016

Choose a reason for hiding this comment

jmadler Feb 11, 2016

Choose a reason for hiding this comment

cramforce commented Feb 12, 2016