Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mechanism for making analytics calls on next page #560

Merged
merged 2 commits into from Mar 20, 2015
Merged

Conversation

@fofr
Copy link
Contributor

@fofr fofr commented Mar 17, 2015

  • Based on an existing technique that sets a GOVUK cookie but which needed classic GA parameters to work
  • Instead use callOnNextPage which sets a method to call on the analytics object with an array of parameters to pass to it
  • Use JSON stringify and parse to save method name and parameters
  • Call the requested method before the initial pageview, allowing for custom variables to be set from a prior page (as is the current use case)

Example of usage replacing old approach in frontend:
https://github.com/alphagov/frontend/compare/next-page-analytics

(Once the frontend change is merged and deployed, the remaining shims within static can be deleted)

* Based on an existing technique that sets a GOVUK cookie but needed
specific classic GA parameters set
* Instead set the method to call on the `analytics` object and any
parameters to pass in
* Split array based on `|` so that commas in values don’t affect calls
* Call the requested method before the initial pageview, allowing for
custom variables to be set from a prior page (as is the current use
case)

StaticTracker.prototype.callMethodRequestedByPreviousPage = function() {
if (GOVUK.cookie && GOVUK.cookie('analytics_next_page_call') !== null) {
var params = GOVUK.cookie('analytics_next_page_call').split('|'),

This comment has been minimized.

@rboulton

rboulton Mar 17, 2015
Contributor

This means that none of the parameters passed to the method can contain a '|' character, which isn't obvious and seems likely to trip us up in future. Parameters will also be silently converted to strings, which might trip people up (though, this being javascript, often won't be noticed).

Might it be better to limit the split to split only once, and pass the full remaining cookie data to the provided method, so the method can parse it as it wants. Particularly since our only current use just wants a single string parameter.

This comment has been minimized.

@edds

edds Mar 17, 2015
Contributor

I don't know how long these strings are likely to be and how much we can put into a cookie but could we use JSON.stringify and JSON.parse to manage the encoding and decoding of the data?

This comment has been minimized.

@rboulton

rboulton Mar 17, 2015
Contributor

JSON.* sounds like a reasonable approach if we want to pass multiple parameters.

This comment has been minimized.

@fofr

fofr Mar 19, 2015
Author Contributor

Updated to use JSON in cc1292d

@rboulton
Copy link
Contributor

@rboulton rboulton commented Mar 17, 2015

This seems mostly reasonable, but I'm wondering if there's actually a need (at present, or anticipated) for the flexibility of allowing a method name to be specified in the cookie? The method needs to be defined on all pages on our site - is there an easy way to ensure this is done right? In the example of usage on frontend, I can't see where the method is defined (but it couldn't be defined in frontend, anyway).

One problem we've seen with the existing implementation of this is that sometimes the cookie data gets loaded on the wrong page (presumably due to the target page failing to load, etc). I've wondered if it would be good to include the expected target page url (or a hash of it, for brevity) in the cookie data, and only perform the action on the target page if the url matches, to strip out this confusing data.

Finally, don't forget to update https://www.gov.uk/help/cookies with the new cookie name.

@fofr
Copy link
Contributor Author

@fofr fofr commented Mar 17, 2015

@rboulton Thank you for the review, some excellent points. The method provided needs to be a named method on the analytics object itself. In the frontend example it would call GOVUK.analytics.setSearchPositionDimension.

Because this is only being used once, and because of the problems with | delimiting and string coercion, I'm tempted to make this a specific solution for setting just the search result custom dimension.

One problem we've seen with the existing implementation of this is that sometimes the cookie data gets loaded on the wrong page.

I did wonder if that was happening. Even with this bad data, is the technique and custom dimension still useful?

Finally, don't forget to update https://www.gov.uk/help/cookies with the new cookie name.

Ah yes, good point.

@rboulton
Copy link
Contributor

@rboulton rboulton commented Mar 19, 2015

Even with the bad data, this dimension is invaluable for analysing search results. I previously estimated the bad data was occurring at most 5% of the time, probably less, but I can't now remember the methodology I used to get that estimate.

Making this be a specific solution for setting a custom dimension would be reasonable, I think. The current implementation isn't specific to setting any particular custom dimension, and I think it would be useful to preserve the ability to set other custom dimensions (or even expand that to allow setting multiple such dimensions at a time) - but I don't think we need more flexibility than being able to set a list of custom dimensions to particular values. I think @edds suggestion of using JSON should make it easy to avoid the problems with delimiters and coercion.

Finally, if you do include a hash of the target URL in the cookie, I'm not sure whether the best thing to do is to drop data where the URL doesn't match the expected value, or whether we should be including that in the custom dimensions so we can do some analysis of how often the URL will mismatch. There's lots of edge cases to think about involving redirections, etc. That probably merits a wider discussion than fits easily into a PR.

@rboulton
Copy link
Contributor

@rboulton rboulton commented Mar 19, 2015

Apologies for the duplicate/partial responses (now deleted) earlier - github went weird on me.

* Splitting with a delimiter would break if the delimiter was included
in the params
* Joining the array of params also inadvertently cast params to strings
* Use JSON stringify/parse to avoid these problems
* Catch JSON parse errors and continue, so that the troublesome cookie
gets deleted
* govuk-template provides a JSON shim for older browsers
* Remove old spec helper regarding JSON.parse, all tests pass correctly
with the latest PhantomJS it seems.
@fofr
Copy link
Contributor Author

@fofr fofr commented Mar 19, 2015

Is this in a state good to merge? I think the improvements @rboulton mentioned are worth doing but should be done separately to this. cc @wryobservations

@edds
Copy link
Contributor

@edds edds commented Mar 20, 2015

This LGTM.

@rboulton
Copy link
Contributor

@rboulton rboulton commented Mar 20, 2015

It does look good, and I'm happy to leave improvements for later.

fofr added a commit that referenced this pull request Mar 20, 2015
Mechanism for making analytics calls on next page
@fofr fofr merged commit 8fc1ea7 into master Mar 20, 2015
1 check passed
1 check passed
default "Build #431 succeeded on Jenkins"
Details
@fofr fofr deleted the next-page-analytics branch Mar 20, 2015
fofr added a commit that referenced this pull request Mar 20, 2015
The technique for setting analytics calls for a following page was
updated in #560 and uses of the
old technique updated in alphagov/frontend#777.

* Remove shim
* Simplify tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.