Page MenuHomePhabricator

Add a basic profiler to Herald transcripts
ClosedPublic

Authored by epriestley on Fri, May 31, 2:33 PM.

Details

Summary

Ref T13298. Add a simple profiler as a starting point to catch any egregiously expensive rules or conditions.

This doesn't profile rule actions, so if "Add subscriber" (or whatever) is outrageously expensive it won't show up on the profile. Right now, actions get evaluated inside the Adapter so they're hard to profile. A future change could likely dig them out without too much trouble. I generally expect actions to be less expensive than conditions.

This also can't pin down a specific condition being expensive, but if you see that H123 takes 20s to evaluate you can probably guess that the giant complicated regex is the expensive part.

Test Plan

Diff Detail

Repository
rP Phabricator
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

epriestley created this revision.Fri, May 31, 2:33 PM
epriestley requested review of this revision.Fri, May 31, 2:34 PM
amckinley accepted this revision.Tue, Jun 4, 9:26 PM
amckinley added inline comments.
src/applications/herald/engine/HeraldEngine.php
358

We don't expect to ever hit this; this is just to prevent the profiler from losing its mind if someone has a broken custom Herald rule or something, right?

This revision is now accepted and ready to land.Tue, Jun 4, 9:26 PM
epriestley added inline comments.Tue, Jun 4, 9:29 PM
src/applications/herald/engine/HeraldEngine.php
358

Right. There might be some crazy pathway where we can hit this upstream -- maybe "Another Herald Rule" causing initial evaluation of a rule with a "Content added" rule on a commit which hits a Conduit exception because of a bad repository cluster configuration -- but this isn't routine and the exception handling is so the profiler doesn't explode when it exits with a nonempty stack or report something wildly misleading that throws us off the scent of the actual issue.

This revision was automatically updated to reflect the committed changes.