Page MenuHomePhabricator

In current VRS deployment scenarios, error logging should not assume Parsoid is the source of all errors.
Closed, ResolvedPublic

Description

T112286 (VE) and T111897 (CX) both reported errors in Parsoid which, after additional investigation, turned out to be errors in RESTBase. The reason why we all assumed it was a parsoid error is because both error-reporting code in both CX and VE/VRS still assume that these clients are directly talking to Parsoid. However, this is no longer the case. All client requests are now proxied through RESTBase. So, please update your error reporting code to pay attention to errors and dump the actual error messages and information about the error source. My concern is not that Parsoid is being blamed, but instead that we could be wasting time hunting down phantom errors.

Event Timeline

ssastry created this task.Sep 11 2015, 8:55 PM
ssastry updated the task description. (Show Details)
ssastry raised the priority of this task from to Normal.
ssastry added a subscriber: ssastry.
Restricted Application added a project: Collaboration-Team-Triage. · View Herald TranscriptSep 11 2015, 8:55 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
ssastry updated the task description. (Show Details)Sep 11 2015, 8:56 PM
ssastry set Security to None.

For example, for T112286, VRS/VE-extension/VE should have received and reported the error in https://github.com/gwicke/restbase/commit/51bbb98aa0dd1a784c63a137f0478ba71f0acfb2 but that error got lost somewhere in the layers and it was reported as: "Something went wrong. parsoidserver-http: HTTP 400." even though the request never even got to Parsoid.

ssastry moved this task from Backlog to Non-Parsoid Tasks on the Parsoid board.Sep 11 2015, 10:14 PM

@ssastry https://gerrit.wikimedia.org/r/#/c/237421/ is the first attempt to fix this.

I left a bunch of comments on that patch -- there are still hardcoded references to restbase/parsoid there.

cscott added a subscriber: cscott.Sep 14 2015, 4:24 PM

Note that CX and VisualEditor share very similar code for creating the VRS object; tweaks for better error reporting probably can/should be kept in parallel as well.

The VRS object perhaps should grow a method to return a user-friendly name for the service (if it doesn't already have one).

Also/alternatively, the error messages are passing back the "error" string returned in the JSON object; we should perhaps add a mechanism to the Parsoid logger to ensure that the error string always contains a "parsoid" prefix. RESTBase should similarly ensure that any error string it returns is prefixed by "restbase".

Also: I suspect the error message seen by the user is deliberately sanitized (ie, does not contain the error message provide by the service). However, the this->dieUsage call in the server-side API should end up in a log somewhere, shouldn't it? Maybe we are just looking in the wrong place?

The VRS object perhaps should grow a method to return a user-friendly name for the service

It shouldn't really care what the backend is (that's a large part of the point of having a VRS abstraction in the first place), but we have a standard path that can be communicated.

Change 238209 had a related patch set uploaded (by Cscott):
Provide VRS objects with a name for more informative debugging/logging

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

Change 238209 merged by jenkins-bot:
Provide VRS objects with a name for more informative debugging/logging

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

Jay8g added a subscriber: Jay8g.Oct 11 2015, 1:09 AM
GWicke closed this task as Resolved.Jul 11 2017, 8:12 PM
GWicke claimed this task.

Based on the patches & inactivity since 2015, I believe this has been resolved. Please reopen if this is not the case.