[1.x] Add average metrics

[aryehraber]

Hi again!

It was awesome to hear from @timacdonald that you were open to adding averages to the time-related metrics (#281).

I've gone ahead and implemented it for "Slow Jobs", and would love to get your feedback before I continue with the others — just to make sure I'm doing it in the way you would like.

Thanks!

[github-actions]

Thanks for submitting a PR!

Note that draft PR's are not reviewed. If you would like a review, please mark your pull request as ready for review in the GitHub user interface.

Pull requests that are abandoned in draft may be closed due to inactivity.

[jessarcher]

The only thing to note here is that the average will only be from when the job was over the threshold (e.g. 1,000ms). I'm not sure whether or not this is what people would want/expect.

If we wanted to show the average from every time the job runs, we'd need to capture a lot more data.

What are your thoughts?

[aryehraber]

@jessarcher That is such a good point... 🤦‍♂️ Completely didn't consider the threshold limiting the data. I would definitely expect the average to be the "real" average and not just from the metrics over the threshold.

I can see how the Recorders would need to capture a lot more data than they currently do for this to work. Perhaps we can make this metric configurable inside each Recorder's config? The config option could then be used to check whether it should capture the average or to ignore it. Additionally, we could then conditionally show/hide the "Average" column to the Livewire component based on the config.

I'll continue working on this idea using the single "Slow Jobs" recorder to see if we like it or not.

[abaldwinls]

@jessarcher first, I love Pulse, thanks so much for all the work to make this happen!

@aryehraber I really like the idea of having the average, and also wouldn't have initially thought of the avg being "off" and only for slow jobs. For me, I wouldn't mind this being an average of only the slow jobs, since that's what is being tracked. Maybe its just a clearly defined option within the Recorders config for slow_( job | request )_average? With this, if people really wanted to see stats for all jobs/requests, they could make the slow threshold 0. (Not a perfect solution, but it'd work for my needs).

Thanks!
Adam

[aryehraber]

Apologies for the delay on this, we're preparing to launch our first product so it's been all hands on deck. I carved out some time this morning to think about this more deeply and try out a few things.

I added an additional config option to record averages before the underThreshold check. This works, but requires 2 separate record() calls (https://github.com/laravel/pulse/pull/284/files#diff-5e3bbf8abb7ea480bdaebfd2fe9ddd42580ca4099aecd092a078285c3fe04abdR82-R89).

However, after further testing with values below the threshold only, I quickly realised that the card starts showing the average data without having any recordings for count & max.

To counter this, I added a reject to the Livewire component to remove rows without a "max" value.

The changes work as expected but admittedly the solution is far from clean imho.

Potentially, it makes more sense to be taking averages of the slow jobs themselves, as @abaldwinls mentioned above. I initially found this idea a bit confusing but have come around that this could actually be fine.

Curious to hear your thoughts!

[aryehraber]

Going to go ahead and close this as there's not been much further discussion and it honestly doesn't feel like the "right" way to go anyways. Happy to re-open or make changes if you want to come back to this!

[FeBe95]

It has been almost 2 years since this was first discussed, but I really like the idea. I was looking for exactly this feature today and stumbled upon this closed issue.

Here are my two cents about the implementation:

Question: Should Laravel Pulse display the average of slow jobs only (option A) or the actual average of all jobs (option B)?

In my opinion, option A can be highly misleading. I am thinking of the following situation: A job normally takes about 1 second to execute. Let’s say it has already run 1,000 times in the last hour, taking 1 second each. Now, due to e.g. temporary high database load or a third-party outage, one single job execution takes 100 seconds to execute. When this happens, the job is then listed in Pulse under "Slow Jobs." It shows with a count of 1 and a slowest execution of 100,000 ms.
If we were to choose option A here, Pulse would show the average as 100,000 ms as well. This can be misleading because one might think that the job only executed once, taking 100 seconds to finish, while the reality is very different. That's why I think option B is the better solution.

To make things even clearer, I would adjust how "count" is recorded and displayed: Instead of only calling record() whenever the job is actually slow, we should always record the execution time (similar to what @aryehraber is already doing for the avg stat). If we look at my example again, instead of showing count: 1 we could show count: 1/1001, or even slow count: 1 | total count: 1001.

This way, when choosing option B, it becomes more straightforward to understand what the average execution time actually means.

Note

One additional idea: Adding this to both the Slow Jobs and Slow Queries recorder would make a lot of sense to me.