Page MenuHomePhabricator

Phatality: Keep error message and trace closer together
Closed, ResolvedPublic

Description

The new error task template used by our Logstash Phatality plugin is somewhat disorienting for developers from what I have heard, and experienced myself.

In this task I'll focus on two aspects that I hope will enable developers to better understand, triage and respond to reports.

  1. The task description is inserted between the error message and the stack trace. These two pieces of information belong together, and due to being separated makes it hard to understand what is going on when needing to keep it all in our minds a the same time. In particular in our current largely uncoordinated/untriaged MW backlogs, where someone would perhaps spend a fraction of a second deciding whether a task seems relevant to them.
  1. The exception message missing the class prefix and file cause. As such, they do not feel real errors as seen in other contexts. Such error message that could be of any type and from any file. My guess is that people tend to default to a sense of "It's probably not in my code" given there is very little supporting signal to suggest otherwise, and (in fairness) why would someone triage a random error that could be caused by any of a hundred different components/teams/features. (Only I'm crazy enough to do that.)

Currently:

Error
MediaWiki version: 1.35.0-wmf.5

message
Unsaved old revision passed

Impact

Notes

TRACE
#0 /includes/Revision/RevisionStore.php(3482): MediaWiki\Revision\RevisionStore->assertRevisionParameter(string, integer, MediaWiki\Revision\MutableRevisionRecord)
#1 /includes/diff/DifferenceEngine.php(1525): MediaWiki\Revision\RevisionStore->countRevisionsBetween(integer, MediaWiki\Revision\MutableRevisionRecord, MediaWiki\Revision\RevisionStoreCacheRecord, integer)
#2 /includes/diff/DifferenceEngine.php(1052): DifferenceEngine->getMultiNotice()
#3 /includes/actions/McrUndoAction.php(242): DifferenceEngine->getDiff(string, string)
#4 /includes/actions/McrUndoAction.php(374): McrUndoAction->generateDiffOrPreview()
#5 /includes/htmlform/fields/HTMLInfoField.php(25): McrUndoAction->{closure}(array)
#6 /includes/htmlform/HTMLForm.php(1699): HTMLInfoField->getDefault()

Proposed:

Error

message
[{exception_id}] {exception_url}   InvalidArgumentException from line 3272 of /srv/mediawiki/php-1.35.0-wmf.5/includes/Revision/RevisionStore.php: Unsaved old revision passed
exception.trace
#0 /includes/Revision/RevisionStore.php(3482): MediaWiki\Revision\RevisionStore->assertRevisionParameter(string, integer, MediaWiki\Revision\MutableRevisionRecord)
#1 /includes/diff/DifferenceEngine.php(1525): MediaWiki\Revision\RevisionStore->countRevisionsBetween(integer, MediaWiki\Revision\MutableRevisionRecord, MediaWiki\Revision\RevisionStoreCacheRecord, integer)
#2 /includes/diff/DifferenceEngine.php(1052): DifferenceEngine->getMultiNotice()
#3 /includes/actions/McrUndoAction.php(242): DifferenceEngine->getDiff(string, string)
#4 /includes/actions/McrUndoAction.php(374): McrUndoAction->generateDiffOrPreview()
#5 /includes/htmlform/fields/HTMLInfoField.php(25): McrUndoAction->{closure}(array)
#6 /includes/htmlform/HTMLForm.php(1699): HTMLInfoField->getDefault()

Impact

Notes

Basically:

  • Prefer normalized_message (and fallback to message) instead of exception.message. The exception.message field is an internal field of the exception object which is only useful if it is prefixed by exception.class with exception.file mentioned on the same line as well. This is in line with the default text formatting from PHP for Exception objects, which MediaWiki already sends as part of normalized_message.
  • Remove separation of the "TRACE" field and bring it directly into the description.

Event Timeline

At first glance this seems trivial but it's actually going to require a deployment of phatality which currently doesn't work because the deployment breaks logstash.

Change 668202 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[releng/phatality@master] Various updates and alignment with MediaWiki/PHP semantics

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

Change 668202 merged by 20after4:
[releng/phatality@master] Various updates and alignment with MediaWiki/PHP semantics

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

mmodell reassigned this task from mmodell to Krinkle.
mmodell subscribed.