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

Support required vars in AMP analytics #5386

Closed
cramforce opened this issue Oct 4, 2016 · 5 comments
Closed

Support required vars in AMP analytics #5386

cramforce opened this issue Oct 4, 2016 · 5 comments

Comments

@cramforce
Copy link
Member

I recently saw a case where a Google Analytics user failed to correctly send their account id.

From reading the code, this is currently a silent failure which is very hard to debug.

I think we should introduce a concept of required vars where a trigger fails with a clear error message in the dev tools console if a variable that is absolutely required was not set.

CC @rudygalfi @avimehta

@rudygalfi
Copy link
Contributor

Could this just be a vendor config feature? I don't think it makes sense in any other context because there's not an expectation of inherited behavior.

@cramforce
Copy link
Member Author

Yeah, I think vendors.js would be a good place for this.

@ericlindley-g ericlindley-g added this to the Pending Triage milestone Oct 5, 2016
@avimehta
Copy link
Contributor

avimehta commented Dec 6, 2016

ok, here are two possible solutions:

1 Have a new array in the config that lists all the required variables and then Ensure that none of those are empty(-ish) when final url creation is being done.

2 Create a variable called REQUIRED in url-replacements and set the default value for all required variables to REQUIRED. Whenever we try to expand this variable, we'll throw a user error and say that this is required.

Both of them are easy enough to do. 1. is contained within mp-analytics package whereas 2 results in code changes in platform. 2 might be slightly simpler to read/understand.

@rudygalfi rudygalfi added this to Backlog in Analytics Feb 8, 2017
@jeffjose jeffjose added this to TODO in Ads & Analytics Fixit - July 2018 via automation Jul 16, 2018
@calebcordry calebcordry assigned calebcordry and unassigned avimehta Jul 16, 2018
@zhouyx
Copy link
Contributor

zhouyx commented Jun 22, 2019

Given we've already supported all the operator macros. We can also introduce a REQUIRED macro for this use case. However the error message itself won't be very specific because it can't relate to the original variable name.

I would vote for approach #1 instead, where we can introduce a requestedVars field;

requiredVars: ['fake']

The con is that no error will occur if ${fake} resolve to empty. But I don't expect that to be common.

@zhouyx zhouyx added this to To do in Ads & Analytics Fixit - August 2019 via automation Jul 2, 2019
@stale
Copy link

stale bot commented Dec 13, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Stale Inactive for one year or more label Dec 13, 2020
@stale stale bot closed this as completed Dec 20, 2020
Analytics automation moved this from Feature Backlog to Done Dec 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Analytics
  
Done
Development

No branches or pull requests

6 participants