Page MenuHomePhabricator

MobileFrontend should use XAnalytics extension
Open, LowPublic

Description

The MobileFrontend extension should utilize the XAnalytics extension instead of handling X-Analytics header by itself:

The code is bit complex, it tries to read the XAnalytics header twice.
step 1: it listens to the RequestContextCreateSkin and reads to XAnalytics header to store all keys in MobileContext
step 2: listens to the BeforePageDisplay hook, and re-injects the header by calling MobileContext::getXAnalyticsHeader() which reads the XAnalytics header once again and then, applies all analytics keys found in the step 1.

The getXAnalyticsHeader() has to know how to build the header, this is not the MobileFrontend responsibility. There is already a XAnalytics extension that knows how to handle that.
The MobileFrontend code responsible for XAnalytics header handling is almost identical to the one in the XAnalytics extension. There is no need to keep the same logic (and code) in two places.

Please refactor the MobileFrontend code, use the XAnalytics extension, and remove all unnecessary/dead code:

Acceptance criteria

  • MobileFrontend uses XAnalyticsSetHeader hook to inject analytics items, Hook is defined in XAnalytics extension. Note: pass strings, not urlencoded as XAnalytics already does that.
  • Remove all XAnalytics header handling (getXAnalyticsHeader, addAnalyticsLogItemFromXAnalytics, getAnalyticsLogItems)
  • Remove XAnalytics handling from RequestContextCreateSkin and BeforePageDisplay
  • Update unit tests to reflect the new state. Please see developer notes
  • There is a MobileFrontend config variable: MFEnableXAnalyticsLogging. MobileFrontend should log in debug (maybe show a warning) when the x-analytics logging is enabled but the XAnalytics extension is not present.

Developer notes:

MobileFrontend has pretty good unit test coverage for x-analytics header handling, the XAnalytics header has no unit tests. Instead of removing unit tests, we should port those into XAnalytics extension.

Event Timeline

Jdlrobson triaged this task as Medium priority.Mar 15 2019, 10:10 PM

Looks clear to me! Thanks for taking the time to explain this one!

Thanks for looking at this. See also T190381: Figure out XAnalytics stuff, which is relevant.

Jdlrobson lowered the priority of this task from Medium to Low.Jul 31 2019, 6:50 PM

Adding analytics given conversation on referenced task.