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

Pass along document.referrer as amp_referrer query parameter during mobile redirection #5776

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

westonruter
Copy link
Member

@westonruter westonruter commented Jan 11, 2021

Summary

When client-side (JS-based) mobile redirection is enabled (which is the default instead of server-side redirection), any amp-analytics that runs on the resulting page will no longer be able to see the the Referrer, resulting in erroneous reports of direct traffic. See support topic.

I think we can rectify this by passing along the referrer in a query parameter, like amp_referrer. With the original referrer then on the destination page, it is then available to be passed along to amp-analytics. I believe this can be done by explicitly declaring the referrer in the vars for an analytics config as follows:

"vars": {
    ...
    "referrer":"QUERY_PARAM(amp_referrer,${documentReferrer})"
    ...
}

Here it attempts to pass amp_referrer query parameter as the referrer, or if that is not available it falls back to the document referrer. I'm not sure if the above is correct or if it should be:

"vars": {
    ...
    "referrer":"QUERY_PARAM(amp_referrer,DOCUMENT_REFERRER)"
    ...
}

See amp-analytics variables and AMP HTML URL Variable Substitutions.

We could modify all amp-analytics on the page to inject this referrer when JS-based mobile redirection is enabled, or we could defer to the user to enable that instead such as via a plugin that filters amp_analytics_entries.

Checklist

  • My pull request is addressing an open issue (please create one otherwise).
  • My code is tested and passes existing tests.
  • My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

@westonruter westonruter marked this pull request as ready for review January 11, 2021 21:36
@github-actions
Copy link
Contributor

Plugin builds for 5e9f9f9 are ready 🛎️!

Base automatically changed from fix/client-side-mobile-redirection to develop January 12, 2021 01:22
Copy link

codecov bot commented Sep 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.14%. Comparing base (2861dff) to head (5e9f9f9).
Report is 6899 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             develop    #5776   +/-   ##
==========================================
  Coverage      74.14%   74.14%           
  Complexity      5498     5498           
==========================================
  Files            201      201           
  Lines          16653    16653           
==========================================
  Hits           12347    12347           
  Misses          4306     4306           
Flag Coverage Δ
javascript 73.34% <ø> (ø)
php 74.17% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant