Feat: show what a queued job promises when it goes wrong - #120
Conversation
Benchmarkbase What the scan detectsDeterministic: both arms scan the same generated application on the same PHP version, and every repetition is checked to produce the same figures. A difference here is a change in what Brain detects, not machine noise — which is worth looking at rather than automatically worth fixing, since detecting more legitimately moves these.
No count changed on either corpus, including the per-node-type breakdown. TimingMedian wall clock on a shared CI runner. Noise is how far the base arm's own repetitions sat from its median, discounting the single worst on each side, with their full spread beside it; a delta inside the noise is not a measurable effect.
Phase split — Full scan — 1,188 filesA phase carries at least as much runner noise as the scan it is part of, so each one gets its own noise floor from the base arm's repetitions of that phase. Useful for locating a large move; a phase that is a few percent of the scan cannot be read at a few percent of accuracy.
Scans a synthetic Laravel application generated by |
SanderMuller
left a comment
SanderMuller
left a comment
There was a problem hiding this comment.
Reviewed at aa537823. The facts it reads are the right ones and the reading is careful. Approving, with two small things.
What it reads, run on shapes rather than read
class MethodFactsJob {
public $tries = 5;
public $timeout = 120;
public function backoff(): int { return 60; }
public function retryUntil(): \DateTime { return now()->addMinutes(5); }
public function uniqueId(): string { return 'x'; }
}comes out as tries=5 timeout=120 backoff=60 dynamic=[retryUntil, uniqueId]. The literal return becomes a number, the two that cannot be reduced say so instead of guessing, and neither is silently dropped. That is the distinction the description rests on and it holds.
On real job classes across three applications: 26, 40 and 29 described, of which 6, 15 and 9 declare anything at all. So the panel appears on roughly a quarter to a third of jobs, which is the right shape for a section people are meant to read rather than scroll past.
::class inside a middleware entry is read as a middleware
public function middleware(): array
{
return [
(new WithoutOverlapping(Order::class))->releaseAfter(60)->expireAfter(180),
new RateLimited('reports'),
SomeBareMiddleware::class,
];
}reports [WithoutOverlapping, RateLimited, Order, SomeBareMiddleware]. Order is the key handed to the overlap guard, not a middleware.
The new scan does what the docblock says. The ::class scan beside it walks the whole method body rather than the top level of each returned entry, so any class constant used as an argument comes back as a name. The bare SomeMiddleware::class form it exists for is the top-level case, so restricting it to entries of the returned array keeps that and drops this.
It is narrow: of the 16 middleware() methods across three applications, none passes a ::class as an argument, so this is a shape I constructed rather than one I found. Worth a follow-up rather than a block.
The superseded entry chunk is left behind
The branch adds index-BwWD5GvV.js and does not delete the one it replaces, so both ship and only the new one is referenced. #117, #118 and #119 each deleted their predecessor; this is the one that does not. One git rm and the count stays at one.
Everything else
Gate: node and edge ids are identical on all three applications and the null arms came out identical, so the enrichment adds data to job nodes and moves nothing else. That is what an enrichment should look like.
The tests bite, each on its own axis. Dropping the literal-return read fails it takes the value from a method whose body is just a return. Taking the outer node of a middleware chain instead of the new fails it names the middleware class, not the chained configuration around it. Making isInteresting() always true fails it says nothing at all about a job that declares nothing. One test each, nothing collateral.
Loading classes is guarded properly. class_exists() inside a try/catch (Throwable) covers the case that matters: a job whose parent or interface is missing raises an Error rather than a warning, and that is caught. Asking the loaded class for ShouldBeUnique and the trait is the right call given both are as often inherited as written.
413 passed (1132 assertions), PHPStan clean, Pint clean, CI green across 15 jobs with per-job results. The committed bundle reproduces from source.
Also worth saying: catching your own fixture for using syntax only 8.4 parses, in a second commit, is the kind of thing the matrix exists to find and usually finds later.
Verdict
Approving. The ::class case and the leftover chunk are both a line each, and neither is a reason to hold a change that turns "this runs on a queue" into something you can act on at three in the morning.
2bd57cd to
506ff56
Compare
A job node carried its name, its file and its flow, so "this runs on a queue" was the whole story — not whether a failure is retried, not whether a second dispatch is dropped on the floor, not whether the job refuses to overlap with itself. Those are the differences that matter at three in the morning. Two sources, for two kinds of fact. Interfaces are asked of the loaded class, because ShouldBeUnique is as often inherited from a base job as written on the job itself. Everything else is read from the source: a declared `$tries = 5` is a fact the file states, and instantiating a job to ask it is not something an analyzer should do — a constructor can take models, open connections, or throw. Reading properties alone would have found a third of it. Measured on an application of 113 jobs: 44 declare ShouldBeUnique, 32 a uniqueId(), 14 a middleware(), and the retry envelope is spread across 8 `$timeout`, 7 `$tries`, 8 retryUntil() and 4 backoff(). So a fact expressed as a method is reported too: as its value where the body is a single `return 60;`, and otherwise as decided-at-runtime. A computed backoff cannot be reduced to a number by reading the source, and printing a guess would be worse than printing nothing. Middleware names come from the `new` at the root of each entry, because middleware is habitually configured by chaining and the outermost node of `new WithoutOverlapping($key)->releaseAfter(60)` is a method call, not the class anyone means. A job that declares none of this gets no panel section at all — six nulls say less than nothing.
`new X()->method()` is PHP 8.4. The analyzer never runs it — PhpParser reads both spellings into the same MethodCall(var: New_), so the test still covers the chained-configuration shape it was written for — but Pint lints the fixtures, and on the 8.2 jobs that is a parse error. It went unnoticed locally because this machine runs 8.5, where the file parses fine.
506ff56 to
9173d23
Compare
3 participants
A job node carried its name, its file and its flow — so "this runs on a queue" was the whole story. Not whether a failure is retried, not whether a second dispatch is dropped on the floor, not whether the job refuses to overlap with itself. Those are the differences that matter at three in the morning.
The section appears only when the job declares something. A job that declares none of it gets no section at all — six nulls say less than nothing.
Two sources, for two kinds of fact
Interfaces and the
Batchabletrait are asked of the loaded class, becauseShouldBeUniqueis as often inherited from a base job as written on the job itself.Everything else is read from the source. A declared
$tries = 5is a fact the file states, and instantiating a job to ask it is not something an analyzer should do — a constructor can take models, open connections, or throw.A fact expressed as a method is still a fact
Reading properties alone would have found a third of it. Measured on an application of 113 jobs:
ShouldBeUniqueuniqueId()middleware()retryUntil()$timeout$triesbackoff()So a method is reported too — as its value where the body is a single
return 60;, and otherwise as decided at runtime. A computed backoff cannot be reduced to a number by reading the source, and printing a guess would be worse than printing nothing.Middleware names come from the
newat the root of each entry. Middleware is habitually configured by chaining, and the outermost node ofis a method call, not the class anyone means. A bare
SomeMiddleware::classentry is read too.Scope
This enriches the job nodes a graph already has; it does not discover jobs. On the application measured that is 27 of 113 — the rest are not reachable from any traced call chain, which is a separate question from this one.
Bus::chain()andBus::batch()are deliberately left out. They are call-site facts rather than class facts, and showing a chain properly means grouping its members and drawing the order between them — the region machinery from #117. Once that is in, a chain can reuse it rather than grow a second copy. The class-level half of it, theBatchabletrait, is included here.Verification
pint,phpstanand the suite are green — 413 passing, 8 new. Three rules were mutation-checked: dropping the literal-return read, taking the outer node of a middleware chain instead of its root, and letting a job that declares nothing through to the panel each turn a test red.Batchablereported zero across the measured graph, so rather than trust the zero it was checked directly against a job known to use the trait, which reportsbatchable: true. None of the reachable job nodes happens to be one of the four that use it.