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

🐛 [amp-analytics] Pass the canonical url on every Parse.ly Analytics pageview request #19425

Conversation

cwisecarver
Copy link
Contributor

Passing the canonical url on pageview requests will allow us to properly associate self-hosted (non-CDN hosted) AMP traffic with the canonical URL of the content. We have had a customer manually add this change to their AMP configuration and verified that the traffic post-change was being correctly associated. I also tested this locally using gulp default and the analytics-vendors.amp.html page to confirm the HTTP request sent by the pageview event was correct.

@jeffjose jeffjose requested a review from zhouyx November 21, 2018 17:59
@jeffjose jeffjose added this to Needs triage in Analytics via automation Nov 21, 2018
@zhouyx
Copy link
Contributor

zhouyx commented Nov 21, 2018

@cwisecarver Please also update the test file. Thanks

@cwisecarver
Copy link
Contributor Author

Sorry @zhouyx . I'm a little out of practice. I ran gulp check-all and thought that also ran the tests. I updated the test file and it's passing locally now.

@cwisecarver cwisecarver changed the title 🐛 Pass the canonical url on every Parse.ly Analytics pageview request 🐛 [amp-analytics] Pass the canonical url on every Parse.ly Analytics pageview request Nov 26, 2018
@cwisecarver
Copy link
Contributor Author

Any update on this @zhouyx ? It's a pretty small configuration change and we have customers waiting on it.

@zhouyx
Copy link
Contributor

zhouyx commented Dec 11, 2018

@cwisecarver Sorry about the delay. LGTM.

One thing to mention is that the meta data object will be encoded to metadata={%22canonical_url%22:%22http%3A%2F%2Flocalhost%3A8000%2Fexamples%2Fanalytics.amp.html%22} here.
let me know if it doesn't work for you. Or i'll go ahead and merge the change.

@cwisecarver
Copy link
Contributor Author

@zhouyx No worries about the delay.

That does work. I'm expecting the JSON to be URL encoded. I just tested that exact snippet you posted and it parsed correctly on our side.

Thanks!

@zhouyx zhouyx merged commit 0ccf9ca into ampproject:master Dec 11, 2018
@zhouyx
Copy link
Contributor

zhouyx commented Dec 11, 2018

@cwisecarver Thank you for confirming! PR merged.

@cwisecarver
Copy link
Contributor Author

@zhouyx I imagine this will end up in the release on January 9th not December 18th, correct?

@zhouyx
Copy link
Contributor

zhouyx commented Dec 11, 2018

@cwisecarver The change will be in canary cut today. But since there will be no prod release next, the change will hit prod on January 2nd.

noranazmy pushed a commit to noranazmy/amphtml that referenced this pull request Mar 22, 2019
…pageview request (ampproject#19425)

* Include canonical URL in parsely pageview

* Remove errant double-quote

* Update test file to match current default pageview request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Analytics
  
Needs triage
Development

Successfully merging this pull request may close these issues.

None yet

3 participants