Page MenuHomePhabricator
Create Task
Maniphest T230798

GrowthExperiments should log exceptions on the exception channel
Closed, ResolvedPublic
Actions

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

SubjectRepoBranchLines +/-
Standardize error handlingmediawiki/extensions/GrowthExperimentsmaster+88 -70
Use more conventional API error handlingmediawiki/extensions/GrowthExperimentsmaster+10 -15
Log exceptions to exception channelmediawiki/extensions/GrowthExperimentsmaster+3 -3
Customize query in gerrit

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.

kostajh subscribed.Aug 20 2019, 12:54 PM
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.

JTannerWMF edited projects, added Growth-Team (Sprint 0 (Growth Team)); removed Growth-Team.Aug 21 2019, 6:12 PM
JTannerWMF moved this task from Incoming to Ready for Development on the Growth-Team (Sprint 0 (Growth Team)) board.
gerritbot added a comment.Sep 6 2019, 10:01 AM

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

gerritbot added a project: Patch-For-Review.Sep 6 2019, 10:01 AM
kostajh claimed this task.Sep 6 2019, 10:02 AM
kostajh moved this task from Ready for Development to Code Review on the Growth-Team (Sprint 0 (Growth Team)) board.

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.

• Catrope moved this task from Code Review to QA on the Growth-Team (Sprint 0 (Growth Team)) board.Sep 6 2019, 9:03 PM
gerritbot added a comment.Sep 6 2019, 9:27 PM

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

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

ReleaseTaggerBot added a project: MW-1.34-notes (1.34.0-wmf.22; 2019-09-10).Sep 6 2019, 10:00 PM
Maintenance_bot removed a project: Patch-For-Review.Sep 6 2019, 10:10 PM
gerritbot added a comment.Sep 12 2019, 6:22 PM

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

gerritbot added a project: Patch-For-Review.Sep 12 2019, 6:22 PM

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

Tgr moved this task from QA to Code Review on the Growth-Team (Sprint 0 (Growth Team)) board.Sep 12 2019, 7:47 PM
kostajh reassigned this task from kostajh to Tgr.Sep 13 2019, 8:23 AM
gerritbot added a comment.Sep 14 2019, 12:38 AM

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

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

ReleaseTaggerBot edited projects, added MW-1.34-notes (1.34.0-wmf.23; 2019-09-17); removed MW-1.34-notes (1.34.0-wmf.22; 2019-09-10).Sep 14 2019, 1:00 AM
gerritbot added a comment.Sep 17 2019, 12:54 PM

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

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

Maintenance_bot removed a project: Patch-For-Review.Sep 17 2019, 1:10 PM
kostajh moved this task from Code Review to QA on the Growth-Team (Sprint 0 (Growth Team)) board.Sep 17 2019, 2:21 PM
Etonkovidova closed this task as Resolved.EditedSep 24 2019, 4:51 PM
Etonkovidova subscribed.

@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.

Etonkovidova added a comment.Sep 25 2019, 12:25 AM
In T230798#5520688, @Tgr wrote:

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!

Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL · Credits