✨ Add support for data, plid, pvid request properties in Parse.ly analytics

[joshuarrrr]

Fixes https://github.com/Parsely/engineering/issues/912

Note that this adds the first default variable to the Parse.ly configuration, which makes the data match the generation logic of the standard tracker (it sends an empty {} by default). I also kept the order of properties as similar as possible to the standard tracker to aid in manual debugging.

The easiest way to use the new data property is to set it to a quote-escaped JSON string. For example:

<amp-analytics type="parsely">
<script type="application/json">
    {
        "vars": {
            "apikey": "example.com",
            "data": "{\"viewID\":\"${timestamp}-${pageViewId}\"}"
        }
    }
</script>
</amp-analytics>

There may be other better ways of passing arbitrary key/value pairs, or actually encoding the JSON strings, but this will likely work for now and was easy to implement.

[emmettbutler]

@joshuarrrr is your intent to have this reviewed by the Parsely team before pull requesting it against the mainline AMP codebase?

[joshuarrrr]

Note that by using the built-in ${pageViewId} variable substitution here, AMP pageviews will only send 4-digit plid values (such as 8726), as opposed to the 8-digit values sent by the Parse.ly tracker (such as 54412420). Other than the integer length, both systems use the same approach:

• Parse.ly tracker: https://github.com/Parsely/parsely-js/blob/352ea7d554d821ddc75a079022b261f871ab232a/js-api/src/tracker/pixel.js#L142-L147
• AMP: https://github.com/ampproject/amphtml/blob/45d40823de8476b3caaa69a04f96c8d18d1f6d7a/src/service/document-info-impl.js#L131-L133

After a GitHub search across Parsely projects, I think this will be OK - we do need a value that can be cast to an int, due to our DPL schema: https://github.com/Parsely/parsely_raw_data/blob/303ae23349a888c1c821e4b9779eca245e42404b/parsely_raw_data/schema.py#L67

[joshuarrrr]

Note that this change explicitly assumes that all AMP pageloads can only send a single pageview. Furthermore, it means the plid and pvid for any AMP event will always be the same. While that's not how the normal Parse.ly tracker works, I don't think it should cause any problems. @rachelannelise does that approach seem OK to you?

While customers could add custom triggers to send multiple pageviews or bind to other events, I'd say that's a non-supported usage.

[cwisecarver]

I don't remember how the plid and pvid work in the traditional tracker but because you're adding them to the template that's used for both the heartbeats and the pageview those, along with data, will be sent on pageviews and heartbeats. If that's not the desired behavior you can move any of them down into the pageview or heartbeat templates.

[joshuarrrr]

Yep, the intent here was to include those values on all events (both pageviews and heartbeats), as the values are used to group those events together in DPL queries: https://www.parse.ly/help/rawdata/schema#id-fields

[joshuarrrr]

OK, this PR is ready for re-review, and I've tried to make explicit the assumptions that we're making with these changes in the code comments above.

[cwisecarver]

IIRC you'll need to update this file for the tests to pass.

[cwisecarver]

I don't remember how the plid and pvid work in the traditional tracker but because you're adding them to the template that's used for both the heartbeats and the pageview those, along with data, will be sent on pageviews and heartbeats. If that's not the desired behavior you can move any of them down into the pageview or heartbeat templates.

[cwisecarver]

Do you still need to add data here? This is just the generic template for parsely integration so it will be sent for every customer who doesn't have a custom implementation. Customers can choose to override these variables, requests, pageview, and heartbeat in their amp configuration in their implementation.

[joshuarrrr]

I'm not sure it's strictly necessary, but the intent here was to make the default AMP requests mimic the behavior of the standard tracker. With the standard Parse.ly tracker, the data property is always sent on all events, and it's default value is an empty object ({}) in cases where integrators don't pass any custom data. Without this variable definition, the AMP tracker equivalent would send an empty string instead. I figured consistency would make it easier to reason about in downstream systems, but if you think that adds too much overhead to the default template, we can remove.

[cwisecarver]

Here's what I did to get it working.

nvm install --lts # install node 12
git co master 
git remote add upstream [email protected]:ampproject/amphtml.git # set upstream to amphtml
git pull upstream master # pull from upstream into local master
git co - # checkout your branch again
git rebase master # rebase your changes onto master
yarn # install new node modules
gulp unit --local-changes # run the tests that would be impacted by local changes (except it actually seems to run everything)

FYI amp tests fail if you're behind a pi-hole