Add support for Parsely analytics #1595

msukmanowsky · 2016-01-26T20:37:27Z

See #1500 for additional information. This PR adds vendor configuration support for Parse.ly analytics.

Outstanding issues are the additional configuration variables we're requesting and the open discussions on client-side sessionization and engaged time implementations.

googlebot · 2016-01-26T20:37:29Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

msukmanowsky · 2016-01-26T20:41:04Z

@amontalenti has signed the CLA.

cramforce · 2016-01-26T21:58:54Z

CC @rudygalfi

rudygalfi · 2016-01-26T22:02:51Z

@msukmanowsky I think the CLA bot will require you to say "I signed it!"

cc @jmadler

avimehta · 2016-01-27T18:37:07Z

extensions/amp-analytics/0.1/vendors.js

@@ -41,6 +41,11 @@ export const ANALYTICS_CONFIG = {
      'scrollHeight': 'SCROLL_HEIGHT',
      'screenWidth': 'SCREEN_WIDTH',
      'screenHeight': 'SCREEN_HEIGHT'
+      // TODO: Requested by Parse.ly
+      //   screenAvailWidth (screen.availWidth)


I think you can remove the first three items here. @rudygalfi has opened #1503.

cc @cramforce for the last item. I am not sure if/how we'll do the session stuff. I think it requires some kind of storage on the client side.

FYI: #1668 adds the these three items. Not in yet, but close.

These vars should be available now.

avimehta · 2016-01-27T18:52:00Z

Forgot to mention:

Also, if you guys would like, you can add an example of parsely to examples/analytics.amp.html

rudygalfi · 2016-01-29T19:47:34Z

@msukmanowsky Three of the variables you requested should make it in soon, so you may want to update this once those are in.

rudygalfi · 2016-01-30T01:11:28Z

cc @andreban

avimehta · 2016-02-03T22:18:43Z

Any updates on this? Please sync with HEAD and rebase. I think we are almost there.

rudygalfi · 2016-02-05T02:53:51Z

@msukmanowsky I think this is close. Could you take a fresh look at the comments, resolve and rebase?

msukmanowsky · 2016-02-08T19:22:12Z

Apologies for the wait, going to have a look today @rudygalfi.

msukmanowsky · 2016-02-08T20:21:04Z

@googlebot I signed it!

avimehta · 2016-02-08T21:22:54Z

extensions/amp-analytics/amp-analytics.md

@@ -60,6 +60,7 @@ when the document is first loaded, and each time an `<a>` tag is clicked:
 ## <a name="attributes"></a>Attributes

  - `type` This optional attribute can be specified to use one of the built-in analytics providers. Currently supported values for type are:
+    - `parsely`: Adds support for Parsely. Configuration details can be found at [parsely.com/docs](http://parsely.com/docs/integration/partners/google-amp.html).


This list should be sorted alphabetically.

avimehta · 2016-02-08T21:26:48Z

Also, do you mind squashing the commits into a single commit to prepare for merging?

Thanks!

msukmanowsky · 2016-02-08T21:44:03Z

@avimehta will do on squashing.

Quick question for you and clientId usage. As far as I understand from https://github.com/ampproject/amphtml/blob/master/spec/amp-var-substitutions.md, if an AMP page is served off the publisher's site and not the AMP CDN, it will effectively use the value of whatever is stored in the cookie amp-${cid scope}$.

Parse.ly stores a visitor ID in a _parsely_visitor cookie on publisher web sites whose value is currently a JSON-encoded string like the following:

'"{"id":"0765d266-80eb-45b3-bd50-81db037498a0","session_count":1,"last_session_ts":1454967564476}'

The id attribute of that JSON object contains the current visitor ID assigned by Parse.ly upon a visitor's first visit. Is it possible for clientId to fall back on this JSON attribute instead of creating a new cookie like amp-parsely which would invariably lead to unique visitor duplication?

Apologies as I may have misunderstood something with respect to CLIENT_ID usage.

rudygalfi · 2016-02-08T23:22:40Z

Publisher site case: cid-scope resolves to the cookie name that's used. It shouldn't have an "amp-" prefix if you don't specify it that way. In other words cid-scope=_parsely_visitor should read cookie _parsely_visitor. If there is no cookie called that, then AMP will set one as “amp-” followed by base64 encoded random string.

AMP CDN case: AMP manages get/set of CID and will generate one. This CID will be unique per a combination of publisher origin (e.g. the nytimes.com part of cdn.ampproject.org/c/nytimes.com/...) and the cid-scope.

You asked:

Is it possible for clientId to fall back on this JSON attribute instead of creating a new cookie like amp-parsely which would invariably lead to unique visitor duplication?

Would there be any issue with transmitting the whole JSON back and extracting the ID server-side?

msukmanowsky · 2016-02-09T00:55:34Z

@rudygalfi perfect, thanks for the explanation. We should be able to work just fine with that.

rudygalfi · 2016-02-09T00:57:27Z

Meant to cc @cramforce on the above to get his take, but happy to hear it should work for you.

@cramforce can you read over #1595 (comment) and let us know if it seems reasonable to you or if you have additional suggestions?

cramforce · 2016-02-09T01:35:06Z

That is exactly right.

msukmanowsky · 2016-02-09T01:44:13Z

Regarding testing, I've created a local build with gulp clean build extensions serve but viewing http://localhost:8000/examples.build/analytics.amp.html#development=1 doesn't show any pixel requests being sent to srv.pixel.parsely.com on load which is expected given the trigger we have in place.

Anything we missed in our integration?

cramforce · 2016-02-09T01:51:43Z

Just running gulp should be enough. It looks good from a quick glance.

msukmanowsky · 2016-02-09T01:53:59Z

@cramforce you're seeing pixel requests to srv.pixel.parsely.com? I'm still not seeing anything here.

cramforce · 2016-02-09T01:55:43Z

I wasn't checking your PR directly yet :) Just looked at the source.

msukmanowsky · 2016-02-09T01:59:05Z

Gotcha. I'm not seeing any requests for chartbeat either fwiw. Only Google Analytics and comScore seem to be working.

msukmanowsky · 2016-02-09T02:11:34Z

@rudygalfi @cramforce I think this is ready to go but feel a little uneasy about not being able to test/see actual pixels firing on that aforementioned example page.

cramforce · 2016-02-09T03:43:41Z

extensions/amp-analytics/0.1/vendors.js

+  'parsely': {
+    'requests': {
+      'host': 'https://srv.pixel.parsely.com',
+      'basePrefix': '${host}$/plogger/?' +


There is an extra $ after ${host}

cramforce · 2016-02-09T03:45:09Z

I checked out your PR and discovered you had a typo in the URL. See above. With that fixed it looks good. Chartbeat also appears to work fine from here.

msukmanowsky · 2016-02-09T04:09:10Z

Thanks @cramforce, fixed the string interpolation issues (there were more than the one, dumb on my part). I can now see the chartbeat pixel firing, but still not ours on http://localhost:8000/examples.build/analytics.amp.html.

avimehta · 2016-02-09T05:43:32Z

Strange. I patched your commit and here is the request I see:

https://srv.pixel.parsely.com/plogger/?rand=1454996518355&idsite=example.com&url=http%3A%2F%2Flocalhost%3A8000%2Fexamples.build%2Fanalytics.amp.max.html&urlref=&screen=1280x850%7C1231x826%7C24&title=AMP%20Analytics&date=1454996518357&ampid=amp-ytFgMnGyzcUXOPhVCdh05N5cWSBqB2FPHZqKbPJBrBltljtRnykJ0QFEu7fy2RtL&action=pageview

avimehta · 2016-02-09T05:44:26Z

extensions/amp-analytics/0.1/vendors.js

+      'xhrpost': false,
+      'image': true
+    }
+


nit: remove this empty line?

msukmanowsky · 2016-02-09T16:25:59Z

Thanks @avimehta. Turns out I didn't have the Chrome extension installed and so requests for AMP JS files weren't being redirected to localhost.

Having a final look and will then ping you.

msukmanowsky · 2016-02-09T20:54:58Z

Alright @avimehta @rudygalfi @cramforce, we're ready to go here, apologies for the delays!

rudygalfi · 2016-02-09T21:00:05Z

👍 LGTM

avimehta · 2016-02-10T18:24:58Z

@msukmanowsky there seems to be a lint error? Can you fix that? @rudygalfi will merge as soon as that is done. Thanks for the patience so far!

lgtm otherwise.

msukmanowsky · 2016-02-11T01:04:21Z

@avimehta good to go.

Add support for Parsely analytics

cramforce assigned avimehta Jan 26, 2016

avimehta reviewed Jan 27, 2016
View reviewed changes

cramforce added the NEEDS REVIEW label Feb 4, 2016

msukmanowsky force-pushed the master branch from 865d6c4 to f2a0387 Compare February 8, 2016 21:15

avimehta reviewed Feb 8, 2016
View reviewed changes

msukmanowsky force-pushed the master branch from 7f3f755 to 11b01c6 Compare February 9, 2016 01:06

msukmanowsky force-pushed the master branch from 11b01c6 to b3c0fc2 Compare February 9, 2016 01:41

cramforce reviewed Feb 9, 2016
View reviewed changes

msukmanowsky force-pushed the master branch from b3c0fc2 to ae32ac5 Compare February 9, 2016 04:03

avimehta reviewed Feb 9, 2016
View reviewed changes

extensions/amp-analytics/0.1/vendors.js

'xhrpost': false,

'image': true

}

Copy link

Contributor

avimehta Feb 9, 2016

Choose a reason for hiding this comment

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

nit: remove this empty line?

msukmanowsky force-pushed the master branch from ae32ac5 to 883614e Compare February 9, 2016 14:05

Add support for Parsely analytics

5147d61

msukmanowsky force-pushed the master branch from 883614e to 5147d61 Compare February 11, 2016 01:03

rudygalfi added a commit that referenced this pull request Feb 11, 2016

Merge pull request #1595 from Parsely/master

df0a119

Add support for Parsely analytics

rudygalfi merged commit df0a119 into ampproject:master Feb 11, 2016

rudygalfi mentioned this pull request Feb 11, 2016

Add built-in vendor configuration for Parsely analytics #1500

Closed

This was referenced Apr 7, 2021

SwG Release 0.1.22.155 #33680

Closed

SwG Release 0.1.22.155 #33684

Closed

SwG Release 0.1.22.155 #33685

Merged

Add support for Parsely analytics #1595

Add support for Parsely analytics #1595

Conversation

msukmanowsky commented Jan 26, 2016

googlebot commented Jan 26, 2016

msukmanowsky commented Jan 26, 2016

cramforce commented Jan 26, 2016

rudygalfi commented Jan 26, 2016

avimehta Jan 27, 2016

Choose a reason for hiding this comment

rudygalfi Jan 29, 2016

Choose a reason for hiding this comment

rudygalfi Feb 5, 2016

Choose a reason for hiding this comment

avimehta commented Jan 27, 2016

rudygalfi commented Jan 29, 2016

rudygalfi commented Jan 30, 2016

avimehta commented Feb 3, 2016

rudygalfi commented Feb 5, 2016

msukmanowsky commented Feb 8, 2016

msukmanowsky commented Feb 8, 2016

avimehta Feb 8, 2016

Choose a reason for hiding this comment

avimehta commented Feb 8, 2016

msukmanowsky commented Feb 8, 2016

rudygalfi commented Feb 8, 2016

msukmanowsky commented Feb 9, 2016

rudygalfi commented Feb 9, 2016

cramforce commented Feb 9, 2016

msukmanowsky commented Feb 9, 2016

cramforce commented Feb 9, 2016

msukmanowsky commented Feb 9, 2016

cramforce commented Feb 9, 2016

msukmanowsky commented Feb 9, 2016

msukmanowsky commented Feb 9, 2016

cramforce Feb 9, 2016

Choose a reason for hiding this comment

cramforce commented Feb 9, 2016

msukmanowsky commented Feb 9, 2016

avimehta commented Feb 9, 2016

avimehta Feb 9, 2016

Choose a reason for hiding this comment

msukmanowsky commented Feb 9, 2016

msukmanowsky commented Feb 9, 2016

rudygalfi commented Feb 9, 2016

avimehta commented Feb 10, 2016

msukmanowsky commented Feb 11, 2016