Page MenuHomePhabricator

AMC: Fix MobileWebMainMenuClickTracking schema for Minerva desktop to avoid errors
Closed, ResolvedPublic2 Story Points

Description

The MobileWebMainMenuClickTracking schema does not work on desktop Minerva. It throws a client error.

Change the skin to MinervaNeue (e.g. by adding useskin=minerva as a URL parameter) and then use a menu item. The following error would be displayed in the Console of your web browser's Developer tools:

[MobileWebMainMenuClickTracking] Value null is the wrong type for property "mobileMode" (string expected)

This adds noise to the console and for completeness should take into the consideration that Minerva can be used on desktop.

Acceptance criteria

Please delete based on whether we decide to keep or throw away the schema and move to "triaged but future" for estimation.

  • Update schema version of MobileWebMainMenuClickTracking to revision 18203509
  • Update MobileContext::getMode (resources/mobile.startup/context.js) to return "desktop" if return mw.config.get( 'wgMFMode' ) is undefined
  • Rename mobileMode field to "mode" (per diff)

Replication steps

  • Use a desktop browser with Minerva set via the query string
  • Open main menu

*make sure your mediawiki instance is configured with the following in LocalSettings.php:

$wgMinervaSchemaMainMenuClickTrackingSampleRate = 1;
  • Open a menu link in a new tab

Note: Sampling rate is set for 50% on the beta cluster. To replicate the issue there, try opening new tabs until the error presents itself.

Bug looks like this:

QA steps

Note: On beta cluster this schema logs at 50% so this should happen on 1 in 2 pages.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMay 1 2018, 4:52 PM
Jdlrobson added a subscriber: JKatzWMF.

@JKatzWMF are we still using the MobileWebMainMenuClickTracking table/data? The code hasn't been touched in years and apparently is not working on desktop Minerva.

Jdlrobson renamed this task from [wmf.1] MinervaNeue desktop - 'Value null is the wrong type for property "mobileMode"' for logging out to MobileWebMainMenuClickTracking schema does not work on desktop Minerva, throws client error.May 1 2018, 4:58 PM

@ovasileva is a better judge of current use than me. I can't speak to the original intent, but I used it to gauge the popularity of the various features on the site, which seems like a valid thing to know. It doesn't change much though, so its not a very interesting dashboard. Its funny that you mention it though, because I was just complaining that we don't have a similar metric on desktop and therefore don't know how to trim the infamously lengthy left-hand nac.

@ovasileva can we get rid of this schema?

Vvjjkkii renamed this task from MobileWebMainMenuClickTracking schema does not work on desktop Minerva, throws client error to 6vdaaaaaaa.Jul 1 2018, 1:13 AM
Vvjjkkii removed ovasileva as the assignee of this task.
Vvjjkkii triaged this task as High priority.
Vvjjkkii updated the task description. (Show Details)
Vvjjkkii removed a subscriber: Aklapper.
CommunityTechBot renamed this task from 6vdaaaaaaa to MobileWebMainMenuClickTracking schema does not work on desktop Minerva, throws client error.Jul 1 2018, 1:39 PM
CommunityTechBot assigned this task to ovasileva.
CommunityTechBot updated the task description. (Show Details)
CommunityTechBot added a subscriber: Aklapper.
CommunityTechBot raised the priority of this task from High to Needs Triage.Jul 3 2018, 1:54 AM

@Jdlrobson - I can't find the actual task, but didn't we identify some issues with this schema when we were looking at the settings changes?

That was a different schema (MobileWebSettings). This schema in MobileWebMainMenuClickTracking - which has been neglected, probably doesn't work and appears to be unused.

Unless you or @Tbayer have used this schema in the last 6 months, I propose we remove this code from mobile.

Jdlrobson renamed this task from MobileWebMainMenuClickTracking schema does not work on desktop Minerva, throws client error to Remove MobileWebMainMenuClickTracking schema from mobile.Jul 11 2018, 5:19 PM
Jdlrobson updated the task description. (Show Details)
Tbayer closed this task as Declined.Jul 12 2018, 12:54 AM
Tbayer reopened this task as Open.
Tbayer triaged this task as Low priority.
Tbayer updated the task description. (Show Details)
Tbayer added a subscriber: phuedx.

CCing @phuedx as the other maintainer of this schema (see schema page)

Tbayer raised the priority of this task from Low to Normal.Jul 12 2018, 12:56 AM

That was a different schema (MobileWebSettings). This schema in MobileWebMainMenuClickTracking - which has been neglected, probably doesn't work and appears to be unused.

A schema named MobileWebSettings doesn't seem to exist, did you mean something else?

Unless you or @Tbayer have used this schema in the last 6 months, I propose we remove this code from mobile.

Thanks for the ping! I don't recall having used it so far. That said, an EL schema doesn't have to be used regularly to be valuable. It's often very useful to be able to resort to an existing schema for a new data question instead of having to build a new instrumentation. How likely is it that the instrumentation is broken at this point?

As far as I recall, usage of Minerva on desktop is still comparatively very low, does anyone have recent numbers? (I might have some soonish in the context of T189554.) So in principle I would be skeptical of removing instrumentations or features just because they don't work there. I agree though that data integrity/code rot could be an argument.

How likely is it that the instrumentation is broken at this point?

I dont seem to have access to it but we would be able to determine if it's still working quite quickly by checking if new entries are coming in for every single item in the main menu.

A quick (totally non-exhaustive) check of the current data:

SELECT event.name, COUNT(*) AS events 
FROM event.MobileWebMainMenuClickTracking 
WHERE year = 2018 AND month = 7 AND day =11 
GROUP BY event.name ORDER BY events DESC LIMIT 10000;

event.name	events
home	34902
random	34304
settings	15970
login	5767
nearby	4215
watchlist	1771
profile	1001
contributions	901
logout	341
Jdlrobson updated the task description. (Show Details)Jul 12 2018, 2:31 PM
Jdlrobson updated the task description. (Show Details)

Thanks for checking @Tbayer ! So it looks like this is working as it's returning results for all the things I'd expect it to be. Note, schema is quite flexible so it could also be used in future for logging any new items we add to the menu.

I've updated the two options we have here. @ovasileva I don't mind what we do here, but we should do one of the options. Please delete as appropriate and move it to estimation!

Ping @ovasileva and @Tbayer we are in the process of modernising this code in T204584. Several patches will be needed.
I'd appreciate a speedy decision here - whether to remove it or keep it - with reasoning. It will help us think about how to think of this code going forward. Option 1 would also simplify T204584 a lot if we decide to do this...

Thank you!

@Jdlrobson - I feel like it will be very useful because we'll want to do similar instrumentation for the new navigation so I would say let's go for option 2 (unless reusing this for the new navigation ends up being harder than building it out again).

Jdlrobson renamed this task from Remove MobileWebMainMenuClickTracking schema from mobile to Fix MobileWebMainMenuClickTracking schema for Minerva desktop to avoid errors.Sep 26 2018, 10:03 PM
Jdlrobson updated the task description. (Show Details)
Jdlrobson updated the task description. (Show Details)

Thanks.
I've pulled out "Consider lowering the sample rate to reduce the number of events we see." - we can deal with that later.

ovasileva lowered the priority of this task from Normal to Low.Oct 2 2018, 4:20 PM
ovasileva set the point value for this task to 2.

Dropping back down to low priority for the time being but do plan on fixing this as a part of T198313: [GOAL] Advanced mobile contributions. Added that as a parent so we don't lose this task

phuedx removed a subscriber: phuedx.Oct 2 2018, 5:08 PM
Aklapper updated the task description. (Show Details)Oct 7 2018, 6:06 PM

@Jdlrobson added projects: patch-welcome, Google-Code-in-2018.

@Jdlrobson: Testing for GCI, I fail to reproduce this. I go to https://www.mediawiki.org/wiki/Phabricator?useskin=minerva&debug=true and clicked the hamburger menu, notifications bell, and language selector but no such output in the console of Firefox 62 on desktop Linux. Am I doing something wrong? Is this still an issue?

Jdlrobson updated the task description. (Show Details)Oct 9 2018, 6:09 PM
ovasileva raised the priority of this task from Low to Normal.Oct 10 2018, 9:57 PM

@Aklapper did the description change help?

Jdlrobson renamed this task from Fix MobileWebMainMenuClickTracking schema for Minerva desktop to avoid errors to AMC: Fix MobileWebMainMenuClickTracking schema for Minerva desktop to avoid errors.Nov 15 2018, 6:58 PM

Unassigning from @ovasileva. I think this was a mistake.

pmiazga claimed this task.Nov 26 2018, 5:41 PM

Change 475917 had a related patch set uploaded (by Pmiazga; owner: Pmiazga):
[mediawiki/extensions/MobileFrontend@master] Bump the MobileWebMainMenuClickTracking schema revision

https://gerrit.wikimedia.org/r/475917

Change 475922 had a related patch set uploaded (by Pmiazga; owner: Pmiazga):
[mediawiki/skins/MinervaNeue@master] Pass mode property to MobileWebMainMenuClickTracking schema

https://gerrit.wikimedia.org/r/475922

Change 475917 merged by Jdlrobson:
[mediawiki/extensions/MobileFrontend@master] Bump the MobileWebMainMenuClickTracking schema revision

https://gerrit.wikimedia.org/r/475917

Change 475922 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Pass mode property to MobileWebMainMenuClickTracking schema

https://gerrit.wikimedia.org/r/475922

pmiazga removed pmiazga as the assignee of this task.Nov 29 2018, 6:06 PM
pmiazga added a subscriber: pmiazga.
Jdlrobson closed this task as Resolved.Nov 29 2018, 7:14 PM
Jdlrobson updated the task description. (Show Details)

QAed. This is working great!