LGTM.

cc @Enriqe and @gmajoulet
FYI: Based on our local testing, the two variables don't resolve correctly. Instead of registering a method to resolve the macro to global variable service, a service is registered instead.
We feel there's no need to fix the issue, because in any way, the two variables only make sense within stories. And the macro should be registered within the story analytics service. That's already the case today, so we are simply removing the two variables from the global service.

I'm not very familiar with the URL replacement mechanism, could you please point me to where we do this in the story analytics service (or any other file) please?

could you please point me to where we do this in the story analytics service

The url-replacement service gets the AmpStoryVariableService. Then it expects the variables to be in the service, when it should actually query the variables_ property in the service: storyVariables.variables_[property].

So that is why isn't returning anything in the analytics request.

Sounds good, thanks for explaining.

Yuxuan mentioned that "the macro should be registered within the story analytics service. That's already the case today, so we are simply removing the two variables from the global service" and I was wondering how this was working?
I understand that the STORY_PAGE_ID and STORY_PAGE_INDEX macros were broken but I'd like to know if it'll be working after this PR, or if we'll have to submit a fix.

Thanks!

@gmajoulet The AmpStoryVariableService contains analytic variables related to amp-story such as storyPageId, storyPageIndex, storyIsMuted, etc...

This variable service is picked up by amp-analytics so that it can expand those variables when a request contains them.

So by removing them from the global variable source, you always have to use the amp-analytics version of the variable (${storyPageId}) instead of the macro platform version (STORY_PAGE_ID). No fix will be needed

I'm still a bit confused as to why #11528 was needed in the first place then, since the variables service already existed when this PR was introduced.

@zhouyx any ideas why the global macros were needed?

I believe when #11528 was introduced, we haven't introduced any story events yet. So everything was aggregated to global service.

Then we introduced all the story related events, for example story-page-visible. And moved all the story analytics service to amp-story.js. It makes sense now that all story variables to be sent along with a story event variables. (I believe the macros worked before, but somehow was broken when we introduced the new code.., not worth to fix it IMO)

Examples,
The following won't work because it's not a story event.

{
  'on': 'visible',
  'request': 'https://example.com/spi=${storyPageId} //(or STORY_PAGE_ID)
}

This would work

{
  'on': 'story-page-visible',
  'request': 'https://example.com?spi=${storyPageId}
}