Page MenuHomePhabricator

Change VisualEditor's error logging to not assume all VRS errors are Parsoid errors
Closed, ResolvedPublic1 Story Points

Description

Not sure how…

Event Timeline

Jdforrester-WMF updated the task description. (Show Details)
Jdforrester-WMF raised the priority of this task from to Normal.
Jdforrester-WMF assigned this task to Krenair.
ssastry moved this task from Backlog to Non-Parsoid Tasks on the Parsoid board.Sep 11 2015, 10:14 PM
Jdforrester-WMF renamed this task from Change error logging to not assume all VRS errors are Parsoid errors to Change VisualEditor's error logging to not assume all VRS errors are Parsoid errors.Sep 11 2015, 10:50 PM
Jdforrester-WMF set Security to None.

I left a bunch of comments on the CX extension code .. https://gerrit.wikimedia.org/r/#/c/237421/

Looking at the VE code here .. https://gerrit.wikimedia.org/r/#/c/217995/9/ApiVisualEditor.php .. I think many of those comments are applicable to this code as well.

In addition, also worth checking why the error reporting on lines 111-115 didn't report the RESTbase error raised in https://github.com/gwicke/restbase/commit/51bbb98aa0dd1a784c63a137f0478ba71f0acfb2 ... @GWicke, @cscott could also weigh in on where that error report got lost and transformed into the generic 'Something went wrong' error message that clients saw.

Change 238177 had a related patch set uploaded (by Alex Monk):
Change a bunch of parsoid references to acknowledge that RESTBase is sometimes in the middle

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

Jdforrester-WMF edited a custom field.

In addition, it makes sense for the VE extension to log any 4xx errors it gets on the save / review wikitext requests. Right now, they are being lost.

In the interim, I've asked RESTBase folks to enable logging for 4xx errors (they aren't logged right now because they are info level and production logging level is warn). We'll do the same in Parsoid. We need this extra info so we can figure out the remaining 'something went wrong' errors that are being reported.

In the longer term, without logging these 4xx reports, we won't be able to figure out what exactly (beyond 'something') went wrong.

PS 240315 enables logging of such client-error requests in Parsoid, while PR 351 does the same for RESTBase. It would probably make sense to deploy them more or less at the same time.

... while PR 351 does the same for RESTBase.

This has been deployed and RESTBase is logging all 4xx occurrences in production.

PS 240315 enables logging of such client-error requests in Parsoid,

This is now deployed as well.

Change 238177 merged by jenkins-bot:
Change a bunch of parsoid references to acknowledge that RESTBase is sometimes in the middle

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

Jdforrester-WMF closed this task as Resolved.Oct 1 2015, 4:27 PM
Jdforrester-WMF removed a project: Patch-For-Review.