✨ [amp-analytics] AT Internet - add default vars + new var pixelPath #19446
Conversation
|
Oh yeah, got the build #50000!! |
BenDz
changed the title
|
Assigning to @zhouyx As I think you reviewed this last? 😄 |
| @@ -24,8 +24,8 @@ | |||
| "atinternet": { | |||
| "base": "https://!log!domain/hit.xiti?s=!site&ts=_timestamp_&r=_screen_width_x_screen_height_x_screen_color_depth_&re=_available_screen_width_x_available_screen_height_", | |||
| "suffix": "&medium=amp&&ref=_document_referrer_", | |||
| "pageview": "https://!log!domain/hit.xiti?s=!site&ts=_timestamp_&r=_screen_width_x_screen_height_x_screen_color_depth_&re=_available_screen_width_x_available_screen_height_&p=_title_&s2=!level2&medium=amp&&ref=_document_referrer_", | |||
| "click": "https://!log!domain/hit.xiti?s=!site&ts=_timestamp_&r=_screen_width_x_screen_height_x_screen_color_depth_&re=_available_screen_width_x_available_screen_height_&pclick=_title_&s2click=!level2&p=!label&s2=!level2Click&type=click&click=!type&medium=amp&&ref=_document_referrer_" | |||
| "pageview": "https://!log!domain/!pixelPath?s=!site&ts=_timestamp_&r=_screen_width_x_screen_height_x_screen_color_depth_&re=_available_screen_width_x_available_screen_height_&p=_title_&s2=!level2&medium=amp&&ref=_document_referrer_", | |||
There was a problem hiding this comment.
since you've added the default value to ${pixelPath} and ${domain}, these two variables can be replaced now. You'll need to change the !domain to the default replaced value to fix tests, same applies to !pixelPath. Thanks.
| 'requests': { | ||
| 'base': 'https://${log}${domain}/hit.xiti?s=${site}&ts=${timestamp}&r=${screenWidth}x${screenHeight}x${screenColorDepth}&re=${availableScreenWidth}x${availableScreenHeight}', | ||
| 'base': 'https://${log}${domain}/${pixelPath}?s=${site}&ts=${timestamp}&r=${screenWidth}x${screenHeight}x${screenColorDepth}&re=${availableScreenWidth}x${availableScreenHeight}', |
There was a problem hiding this comment.
If I understand this correctly, you want all the requests send to your server? Is there any reason to override the default domain? If not, why don't we encode it to the base request instead.
There was a problem hiding this comment.
The goal was to have a default value for the ${domain} variable, since it can be customized for some clients, but most of them will use our default domain (xiti.com).
I'm not sure I understood correctly how those default vars work though... Is it for this kind of use case? Same goes for ${pixelPath}. Thanks.
There was a problem hiding this comment.
@zhouyx just checking if you saw my answer. Thanks!
There was a problem hiding this comment.
@BenDz There is an issue with replacing the hostname. As all variables will by default be decoded. We are currently working on a fix to this issue.
There was a problem hiding this comment.
BTW: There's #19759 to track this issue.
There was a problem hiding this comment.
@BenDz Sorry for the delay here. The team had some discussion on encoding the request domain. We think putting variables in the request domain is risky.
Your example with domain=.xiti.com works, because encodeURIComponent('.xiti.com') = 'xiti.com'. But the request will break if one put domain=.xiti.com/productPage because the request is encoded to xiti.com%2FproductPage. This type of issue will be very difficult to debug if happen.
#19759 is our attempt to fix this issue.
I understand that you have the best knowledge on the expecting value to the ${log} and ${domain}. If you think our concern won't be an issue with your use case and you want to continue with the current approach. We can definitely unblock this PR. But please document (a comment?) that the value for ${log} and ${domain} must equal itself after encoding.
Let me know if this sounds good, Thank you!
There was a problem hiding this comment.
@zhouyx Thanks for the update. We do have the new ${pixelPath} that will take care of the path. Then, ${domain} should never contain encoded characters.
I can also add this to our documentation.
By the way, I will update the tests to fix it.
Thank you!
|
LGTM. PR merged. |
…mpproject#19446) * [amp-analytics] AT Internet - add default vars + new var pixelPath * Fix atinternet tests
AT Internet improvements: