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: Added support for extraUrlParamsReplaceMap defined in a trigger block. #14233

Open
dougscher opened this issue Mar 25, 2018 · 9 comments

Comments

@dougscher
Copy link

This will add efficiency and better organization of param replacements that are only associated with specific triggers.

Similar use case as with:
Added support for extraUrlParams in trigger blocks and URL vars. #3168

@dougscher dougscher changed the title Added support for extraUrlParamsReplaceMap defined in a trigger block. amp-analytics: Added support for extraUrlParamsReplaceMap defined in a trigger block. Mar 25, 2018
@aghassemi aghassemi added this to the Pending Triage milestone Apr 9, 2018
@aghassemi
Copy link
Contributor

/to @zhouyx

@zhouyx
Copy link
Contributor

zhouyx commented Jul 16, 2018

@dougscher You are suggesting adding extraUrlParamsReplaceMap to trigger config as well as the extraUrlParams. What to do with global extraUrlParamsReplaceMap and trigger level extraUrlParamsReplaceMap? Shall we combine the values and override config level value with trigger level value? Could you please share your detail use case?

@dougscher
Copy link
Author

Right now, we use extraUrlParamsReplaceMap to allow url params to be specified by the user
as a meaningful name:
articleId: 'artid',

So the user does not have to deal with the cryptic names that are actually used in the event url.
This could be done with variables, but then we would be sending a lot of empty url params if they are not used, same example:
&artid=""

Right now extraUrlParamsReplaceMap is limited to 16 entries for some reason. bumping that to maybe 40 would take us a long way.

Also allowing extraUrlParamsReplaceMap to be specified to a specific trigger would allow controlling these sets of parameters to only apply to specific events.

@zhouyx
Copy link
Contributor

zhouyx commented Sep 7, 2018

@rudygalfi Do you maybe have some context on why we put the 16 entries limit to extraUrlParamsReplaceMap. from #1932

@ampprojectbot
Copy link
Member

This issue hasn't been updated in awhile. @zhouyx Do you have any updates?

@stale
Copy link

stale bot commented Dec 24, 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 24, 2020
@dougscher
Copy link
Author

This issue hasn't been updated in awhile. @zhouyx Do you have any updates? This change would fix some challenges we have with our implementation.

@stale stale bot removed the Stale Inactive for one year or more label Dec 24, 2020
@zhouyx
Copy link
Contributor

zhouyx commented Dec 29, 2020

I think expanding the limit from 16 to 40 is reasonable.
I also think that extraUrlParamsReplaceMap should only be defined by analytics vendors. Maybe a good approach is to remove the count limit check, but disallow the configuration of extraUrlParamsReplaceMap inline.

cc @jeffjose

@zhouyx zhouyx assigned micajuine-ho and jeffjose and unassigned zhouyx Dec 29, 2020
@micajuine-ho
Copy link
Contributor

From a performance standpoint, increasing the limit from 16 to 40 doesn't seem like it will be immensely impactful.

However, removing inline support seems a little risky though since some pubs might have custom inline replace maps.

I would prefer to only add support for extraUrlParamsReplaceMap within a trigger block:

  • If an extraUrlParamsReplaceMap is defined in a trigger block, then we will use it to preprocess the extraUrlParams within the trigger block
  • Top level extraUrlParams will not be affected by trigger level extraUrlParamsReplaceMap

@dougscher Would this be sufficient for your use case?

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

No branches or pull requests

6 participants