Skip to content
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

[5.x] Display inactive supervisors in dashboard #1286

Merged

Conversation

PrinsFrank
Copy link
Contributor

When running horizon spread over a lot of machines and with a lot of supervisors per machine it becomes difficult to see what machine misses what specific supervisor. (You have to count the number of rows and then find the one that is missing).

This PR adds any missing supervisor in the overview for the machine, indicated with a cross icon so It's visible that the supervisor is not running. (The number of processes is also 0)

I think I have a fix for the underlying missing supervisors issue, we're running that right now in production. If so, a PR will follow somewhere next week.

@PrinsFrank PrinsFrank force-pushed the make-inactive-supervisors-visible branch 3 times, most recently from 8df7018 to acf3aa2 Compare June 20, 2023 21:50
@PrinsFrank PrinsFrank force-pushed the make-inactive-supervisors-visible branch from acf3aa2 to e876265 Compare June 20, 2023 21:53
@PrinsFrank PrinsFrank changed the title [5.x] Make inactive supervisors visible [5.x] Display inactive supervisors in dashboard Jun 20, 2023
@driesvints
Copy link
Member

Thanks. Can you add some screenshots of this?

@PrinsFrank
Copy link
Contributor Author

Of course! Here are some for a simple scenario with just one machine:
image

image

The two supervisors with the cross icon before them were previously not visible.

@taylorotwell taylorotwell merged commit ea1ff82 into laravel:5.x Jun 22, 2023
10 checks passed
@bjarn
Copy link

bjarn commented Jul 12, 2023

Hi @PrinsFrank and @driesvints,

Great contribution! However, there are scenarios where it might be better to hide them.

For example; I have projects (and others I know have them too) where they intentionally do not add a supervisor to certain workers (e.g. via the Horizon env variable).
This scenario causes a lot of "inactive" supervisors to be shown under workers where they shouldn't.

Maybe a setting toggle is useful, or we should add a config setting to config/horizon.php where they expect certain supervisors.

Example:
Screenshot 2023-07-12 at 21 13 14

@PrinsFrank
Copy link
Contributor Author

@bjarn Do you have an example config and a screenshot of one such scenario? We have a similar setup running and the supervisors that should not run for a specific provisioning plan are not shown.

@bjarn
Copy link

bjarn commented Jul 12, 2023

@bjarn Do you have an example config and a screenshot of one such scenario? We have a similar setup running and the supervisors that should not run for a specific provisioning plan are not shown.

@PrinsFrank You were fast! Just posted one at the same time when you replied.

Some parts of the config where I define the environments and where it gets it from.

    /*
    | ------------------------------------------------------------------
    | Horizon Environment
    | ------------------------------------------------------------------
    |
    | This value determines Horizon's environment
    |
    */
    'env' => env('HORIZON_ENV'),

    'environments' => [
        'ams' => [
            'ams-worker-1' => [
                'connection' => 'redis',
                'queue' => ['...-ams'],
                ....
            ],
        ],
        'mia' => [
            'mia-worker-1' => [
                'connection' => 'redis',
                'queue' => ['...-mia'],
                ....
            ],
        ],
        'syd' => [
            'syd-worker-1' => [
                'connection' => 'redis',
                'queue' => ['...-syd'],
                ....
            ],
        ],
        'fns' => [
            'fns-app-1' => [
                'connection' => 'redis',
                'queue' => ['...'],
                ....
            ],
        ],

I do think I see a pattern. It's not showing 'mia' and 'syd' as missing in the other workers, just the 'fns' ones, which is the one hosting the Horizon dashboard.

@PrinsFrank
Copy link
Contributor Author

@bjarn Thanks, I am able to reproduce this, thanks! Let's see if I can fix this quickly.

@PrinsFrank
Copy link
Contributor Author

@bjarn Fixed in #1294

@PrinsFrank
Copy link
Contributor Author

PrinsFrank commented Jul 12, 2023

To clarify: The intention was to show supervisors for environments where the supervisors were intended to run on that environment but where the supervisor had crashed. But running multiple machines with different environments was not working correctly (with the --enviroment flag or using a different environment in the .env). When this PR gets merged that will now also work, and missing supervisors will only be displayed for supervisors that are actually missing.

If you want you can add my fork as a vcs repository and require the branch to verify the fix:

composer config repositories.foo vcs "https://github.com/prinsfrank/laravel-horizon"

composer require laravel/horizon:dev-fix-inactive-supervisors-shown-on-different-env

@bjarn
Copy link

bjarn commented Jul 12, 2023

That's great! I'll try to test it tomorrow :)!

And makes sense! Thanks a lot for fixing this so quickly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants