Page MenuHomePhabricator

GrowthExperiments should log exceptions on the exception channel
Closed, ResolvedPublic

Description

GrowthExperiments catches all exceptions for some calls (specifically, homepage modules) and then logs them on the GrowthExperiments channel (see SpecialHomepage::logModuleRenderIssue). That reduces the visibility of serious issues (e.g. no one will watch the GrowthExperiments channel during a train deploy) and is confusing for developers. They should be logged to the exception channel (or both if that's preferable).

Details

Related Gerrit Patches:
mediawiki/extensions/GrowthExperiments : masterStandardize error handling
mediawiki/extensions/GrowthExperiments : masterUse more conventional API error handling
mediawiki/extensions/GrowthExperiments : masterLog exceptions to exception channel

Event Timeline

Tgr created this task.Aug 20 2019, 12:48 PM
Restricted Application added a project: Growth-Team. · View Herald TranscriptAug 20 2019, 12:48 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
Tgr added a comment.Aug 20 2019, 12:49 PM

As an aside, the code catches Exception and Error subclasses; it would be slightly more correct to catch Exception and Throwable. Currently the only throwables are Exception and Error but PHP makes no promise that this will be the case.

Tgr updated the task description. (Show Details)Aug 20 2019, 1:02 PM
kostajh moved this task from Inbox to Upcoming Work on the Growth-Team board.Aug 20 2019, 1:17 PM

I would say we do this in an upcoming sprint. Probably exception channel on its own is sufficient.

Change 534779 had a related patch set uploaded (by Kosta Harlan; owner: Kosta Harlan):
[mediawiki/extensions/GrowthExperiments@master] Log exceptions to exception channel

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

kostajh claimed this task.Sep 6 2019, 10:02 AM

As an aside, the code catches Exception and Error subclasses; it would be slightly more correct to catch Exception and Throwable. Currently the only throwables are Exception and Error but PHP makes no promise that this will be the case.

I think this can be done later, if at all. In the attached patch I've changed to log to the exception channel.

Change 534779 merged by jenkins-bot:
[mediawiki/extensions/GrowthExperiments@master] Log exceptions to exception channel

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

Change 536310 had a related patch set uploaded (by Gergő Tisza; owner: Gergő Tisza):
[mediawiki/extensions/GrowthExperiments@master] Use more conventional API error handling

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

Change 536311 had a related patch set uploaded (by Gergő Tisza; owner: Gergő Tisza):
[mediawiki/extensions/GrowthExperiments@master] Standardize error handling

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

kostajh reassigned this task from kostajh to Tgr.Sep 13 2019, 8:23 AM

Change 536310 merged by jenkins-bot:
[mediawiki/extensions/GrowthExperiments@master] Use more conventional API error handling

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

Change 536311 merged by jenkins-bot:
[mediawiki/extensions/GrowthExperiments@master] Standardize error handling

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

Etonkovidova closed this task as Resolved.EditedSep 24 2019, 4:51 PM
Etonkovidova added a subscriber: Etonkovidova.

@Tgr (or @kostajh ) - a questions (not related to the task):

  • Growth Team dashboard - why it has only ar.wikipedia.org and not all target wikis?
Tgr added a comment.Sep 24 2019, 8:07 PM

Probably no errors on any of the other target wikis right now?

Btw I added a

{
  "match_phrase": {
    "origin": "GrowthExperiments"
  }
},

to the dashboard filter so everything that was on the dashboard before the patch is kept there.

Probably no errors on any of the other target wikis right now?
Btw I added a

{
  "match_phrase": {
    "origin": "GrowthExperiments"
  }
},

to the dashboard filter so everything that was on the dashboard before the patch is kept there.

Thanks!