[1.x] Allow thresholds of slow queries, jobs and requests to be customised by regex pattern #340
Conversation
|
Many checks are broken but I can't understand the reason, even with a fresh clone of |
|
Thanks for the PR! I've fixed the failing test. The Livewire payload in the test was incomplete as it was previously inconsequential, but with these changes, the payload needs to be parsed to determine the path for the threshold. I think this could be a useful feature, but at the moment, it seems like a breaking change for existing users. To move forward and consider this feature, it would need to support the existing config file format where the threshold is a single value rather than an associate array. I'm also not sure whether this new format should be the default in the config file or just something we document. |
|
Hi @jessarcher, Thank you for fixing the tests! About the problem with the config file, what do you think about creating a previous check? We could test if the value is a array to continue this way, in case of negative we could fallback to the current way, just using the single value. For this change I would need to change the two methods If you think it's viable I could submit the changes. |
|
@matheus-carvalho Sounds reasonable. Our other concern is introducing a concept that ties cards to individual recorders with the |
|
Thank you for your attention @jessarcher. Alright, this would really be a problem, I'll implement the changes to fix these points and submit the commit. Please let me know if there are something else worrying you. |
|
Thanks @matheus-carvalho! I haven't run it locally yet to check out the UI additions, but the only other thing I'm wondering about is whether the "Top 10 users experiencing slow endpoints" view in the Usage card should have something to indicate when there are path-specific thresholds in play. I'm imagining a scenario where a user complains about something being "slow", but it doesn't show up on that card even though it's over the default threshold because it was under the path-specific threshold. The person viewing the dashboard may not be aware of the additional configuration. It's also up to Taylor whether we want to take this feature on, but it looks useful to me! |
|
Hello @jessarcher, I have pushed some improvements to take care of the pointed concerns.
Here are some images from my local tests so you can take a look at UI additions. Information icon |
|
Hello, excuse me but I just want to know if there are something else I could do for this PR. |
|
We'll review this one shortly and see if we can move it along. |
timacdonald
changed the title
timacdonald
force-pushed
the
customise-thresholds
branch
from
9f4e0de to
90ae7b9
Compare
| // @phpstan-ignore argument.templateType, argument.templateType | ||
| $custom = collect($config) | ||
| ->except(['default']) | ||
| ->first(fn ($threshold, $pattern) => preg_match($pattern, $value)); | ||
|
|
||
| return $custom ?? $config['default'] ?? 1_000; |
There was a problem hiding this comment.
Notice that we have a hard-coded 1_000ms here as the default threshold when no default is specified; this matches our config defaults.
When a user changes from a single threshold to an array, if they forget the default key that the app continues to work with this default.
Not sure if we wanna keep this or just document that they need to have the default key, but this felt okay to me.
There was a problem hiding this comment.
I'm not sure if you want my opinion about this, but don't breaking the app even without a default key sounds good for me. Although, if you want help documenting the default key's mandatory, count on me.
timacdonald
force-pushed
the
customise-thresholds
branch
10 times, most recently
from
63ea9f7 to
62bee6c
Compare
timacdonald
force-pushed
the
customise-thresholds
branch
from
62bee6c to
71e1590
Compare
| <button title="{{ $message }}" @click="alert('{{ str_replace("\n", '\n', $message) }}')"> | ||
| <button title="{{ $message }}" @click="alert(@js($message))"> |
There was a problem hiding this comment.
Standardising use of the @js directive, which I've used in some of the changes in this PR.
|
Some UI shots here. I've decided to keep this pretty simple and only change the UI based on the threshold config type. Array = per item threshold reporting. Integer = Global type threshold. We could have done fancy stuff checking if there was more than one threshold value for the all the listed items - but I didn't like the idea that the UI would change if an entry came into the table that had a different threshold while you were watching it. Slow requests'threshold' => 1000,
'threshold' => [
'default' => 1000,
'#pattern#' => 10,
],
Application usage card'threshold' => 1000,
'threshold' => [
'default' => 1000,
'#pattern#' => 10,
],
Slow queries'threshold' => 1000,
'threshold' => [
'default' => 1000,
'#pattern#' => 10,
],
Outgoing requests'threshold' => 1000,
'threshold' => [
'default' => 1000,
'#pattern#' => 10,
],
Slow Jobs'threshold' => 1000,
'threshold' => [
'default' => 1000,
'#pattern#' => 10,
],
|
I'm facing a problem with slow jobs and requests I've already know is slower than others, so it would be useful I could customise the thresholds individually, using regex patterns.
I think it would be useful to anyone who have resources known to be slow, like report jobs, huge-data endpoints or something else, and want to monitor them with a higher threshold, not just ignore them.
I guess this changes will not break any existing feature because we still with a 'default' threshold, which will be used until some individual customisation.