Properly handle the networking errors and aborted requests
Open, NormalPublic

Description

We should not log the aborted requests as PagePreviewsAPIFailure because that makes API Failures chart unusable. After deployment of T197700, the chart went from <10 events per minute to 200-400 events per minute and the graph doesn't present API failures. The noise generated by aborted requests is too big. The abort action should not increment the PagePreviewsApiFailure because it is not a request failure.

Developer notes

The catch logic in actions.js is incorrect. result parameter will be always undefined as the only place that causes catch() statement to be executed is this this throw. Current logic assumes that every time we land in catch((err, result) => { ... }) scope is not a network issue (because of the fact that result is always not defined) and it shows the generic page preview. Every fetch exception causes FETCH_COMPLETE state, which triggers the generic preview state.

Because of that, we cannot:

  • determine if this is an offline issue and not show the page preview
  • it's not possible to not call FETCH_COMPLETE when the request was aborted
Bonus points:

Rest gateway shouldn't know how to handle not found issues

Acceptance criteria:
  • If I go to a page with Page Previews on, change to offline mode in the networking tab, hover a link - it should not show the Page Preview
  • When a request abort happens, Page Previews should not switch into FETCH_COMPLETE state
  • When a request is aborted, the counter.PagePreviewsApiFailure should not be incremented
  • when the server (both Mediawiki and Restbase) returns 4xx or 5xx error the Page Preview should be visible (the generic preview)
  • POST Deployment: API Failures graph should get back to normal (less than 10 events per minute)

QA

  • Test on staging.
  • Drop connection, no page preview should show on any link
  • Restore connection, page previews should show on all links
  • When I hover over "List of D. C. Thomson & Co. Ltd publications" on http://reading-web-staging.wmflabs.org/wiki/Special:AllPages I should see an error (no lead section)
  • If page doesn't exist, a broken page preview will show e.g. Timed test 2
  • If a page preview summary request results in a 500 error then a broken page preview will show (not sure how to test this one without changing code, but JR checked this manually)
pmiazga created this task.Thu, Jul 12, 9:59 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptThu, Jul 12, 9:59 PM

Change 445543 had a related patch set uploaded (by Pmiazga; owner: Pmiazga):
[mediawiki/extensions/Popups@master] Properly handle catch() when calling gateway fetch.

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

pmiazga updated the task description. (Show Details)Thu, Jul 12, 10:20 PM

Change 445545 had a related patch set uploaded (by Pmiazga; owner: Pmiazga):
[mediawiki/extensions/Popups@master] When request gets aborted it shouldn't finish with FETCH_FAILED

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

Change 445543 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] Properly handle catch() when calling gateway fetch.

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

Jdlrobson triaged this task as Normal priority.
Jdlrobson moved this task from To Do to Needs More Work on the Readers-Web-Kanbanana-Board board.
pmiazga claimed this task.Fri, Jul 13, 12:54 PM
pmiazga updated the task description. (Show Details)Fri, Jul 13, 4:44 PM
Jdlrobson updated the task description. (Show Details)Fri, Jul 13, 5:33 PM
Jdlrobson reassigned this task from pmiazga to ABorbaWMF.
Jdlrobson added subscribers: ABorbaWMF, Ryasmeen, Jdlrobson.

Skipping design review. @Ryasmeen @ABorbaWMF could we do a generic page previews QA for error handling? We'll want to check that page previews do not show when the user's connection drops and continue to show when the connection is live. I've added some QA notes, let me know if I can help with anything more.

Change 445545 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] When request gets aborted it shouldn't finish with FETCH_FAILED

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

Niedzielski reassigned this task from ABorbaWMF to Ryasmeen.Mon, Jul 16, 6:23 PM