-
Notifications
You must be signed in to change notification settings - Fork 659
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make the recent failed jobs metric period configurable #637
Comments
Ping @JayBizzle @halaei @mfn @shawnhind. Can you guys please provide more insight into why you preferred option 1 vs option 3 from the issue linked above? |
I've no strong opinion and not opposing 3️⃣ ; to me the additional "complexity" didn't seem worth it but as said, no strong attachment to 1. |
@driesvints I'm in the same boat as @mfn. I oppose #2 because failed jobs in the last hour is too small of a time period to be useful. I liked option 1 because it was simple and just implemented the behavior that was already expected. I have nothing specifically against #3 other than extra complexity to the configuration but as long as defaults are sane then that's fine. |
@ghobaty feel free to attempt a PR. Please do keep in mind not to re-introduce the original problem. |
I've rebased my option 3 branch on the master branch and reverted the Vue template changes made in #633 for the tagged 3.2.6 release. Ignoring the previously merged option 1 pull request, this comparison gives an idea of the Laravel Horizon changes from 3.2.5: 03cfd34...derekmd:dashboard-failed-job-count-config-filter The question remaining is what should the default metric config be? Should it stay as 10080 (7 days) if this is being released in the 3.0 branch? 'trim' => [
'recent' => 60,
'recent_failed' => 10080, // new config value added
'failed' => 10080,
'monitored' => 10080,
], or should it default to a more narrow time period? 'trim' => [
'recent' => 60,
'recent_failed' => 1440, // shorten to 24 hours
'failed' => 10080,
'monitored' => 10080,
], |
24 hours for me. Thanks! |
Sorry for the late reply (family holiday), but basically what @shawnhind said. @derekmd I'd keep the default config values equal to each other (10080) |
I think it's ok to keep the current default. You're free to change it afterwards. |
PR was merged. |
Related: #621
After giving the latest version a try on production, for our use case, we think that the number of failed jobs last hour is actually more insightful than the number of failed jobs for the whole week (or even longer if we decide to bump up our
trim
config).In my opinion, adjusting
trim
config to only 1 hour just for the dashboard metric doesn't sound like the best solution here (as we may actually need to access the old failed jobs).That's why I would like to propose to make this configurable (e.g. option #3 from @derekmd 's proposed solution #621 (comment)) and it can default to the
trim.failed
The text was updated successfully, but these errors were encountered: