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

feat: expose queue driver, schedule status #3593

Merged
merged 19 commits into from
Nov 19, 2022
Merged

feat: expose queue driver, schedule status #3593

merged 19 commits into from
Nov 19, 2022

Conversation

imorland
Copy link
Member

@imorland imorland commented Aug 11, 2022

Changes proposed in this pull request:
An effort to promote the use of the scheduler, this proposes exposing it's basic status to the dashboard. Possible statuses are

  • Never run
  • Active
  • Inactive

If there are no scheduled tasks registered, then the scheduler status is not displayed.

A simple timestamp is stored in settings, with a check to see if the last run was > 5 minutes ago.

Additionally, we expose the driver in use for the Queue, using shared logic from InfoCommand

Reviewers should focus on:
As other info added to the admin payload, strings are hard coded. Should these perhaps become translations? I'd lean towards that, actually.

Screenshot
Active:
image

Inactive:
image

Never run:
image

Redis queue driver:
image

edit - added info link to scheduler setup information
image

Info console command:
image

Necessity

  • Has the problem that is being solved here been clearly explained?
  • If applicable, have various options for solving this problem been considered?
  • For core PRs, does this need to be in core, or could it be in an extension?
  • Are we willing to maintain this for years / potentially forever?

Confirmed

  • Frontend changes: tested on a local Flarum installation.
  • Backend changes: tests are green (run composer test).
  • Core developer confirmed locally this works as intended.
  • Tests have been added, or are not appropriate here.

Required changes:

  • Related documentation PR: (Remove if irrelevant)
  • Related core extension PRs: (Remove if irrelevant)

@imorland imorland requested review from luceos, davwheat and SychO9 August 11, 2022 22:05
Copy link
Member

@davwheat davwheat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be worth adding warning icons for inactive or never run, warning of the potential consequences?

@luceos
Copy link
Member

luceos commented Aug 12, 2022

Doesn't laravel dispatch events when the scheduled tasks run?

With scheduled tasks requiring registration we can only show scheduled tasks health when there actually tasks scheduled.

I don't know how much this conflicts with our plans for an advanced tab for queues, and other things mostly for larger communities ...

@imorland
Copy link
Member Author

Doesn't laravel dispatch events when the scheduled tasks run?

It does, ScheduledTaskStarting, ScheduledTaskFinished and ScheduledTaskFailed, which indeed could be used for a more detailed insight into scheduled tasks for sure. Perhaps that's something we'd like to introduce as well? The aim for this high level status indicator is more about "Is the schedule:run command being trigged regularly"

With scheduled tasks requiring registration we can only show scheduled tasks health when there actually tasks scheduled.

This implementation is not dependent on any tasks being registered, (see above comment).

I don't know how much this conflicts with our plans for an advanced tab for queues, and other things mostly for larger communities ...

I don't see any information about these plans on the roadmap or issues, perhaps I missed this? Could you perhaps point me in the direction of this so I can take a look?

@imorland
Copy link
Member Author

Would it be worth adding warning icons for inactive or never run, warning of the potential consequences?

Yeah, I think that's a good call 👍

@SychO9
Copy link
Member

SychO9 commented Aug 17, 2022

The advanced admin page had conversations here and there but nothing concrete written yet, although Daniël did push some code for it at some point though. So we don't yet know for sure what it will be like and I don't know when it will be tackled.

So I think it's fine to move with this, for now, I like the additional info. I haven't yet looked at the code, but I think we should really move the two discussions about setting up Scheduler(linked to in this pr) and Queues(linked to by the package manager) to the docs and link to the docs instead, though not necessarily blocking this PR.

@luceos
Copy link
Member

luceos commented Aug 17, 2022

Agreed. I do still believe it only makes sense to show the status for scheduler in case there are at least commands registered to run with it aka there's a need for it to run. Otherwise it will just confuse people about a need for the scheduler which doesn't even exist. Scenario's like "I have an error with X, but I noticed the scheduler isn't running so how can I run it?" will probably happen where X is unrelated to any scheduled task.

@imorland
Copy link
Member Author

imorland commented Aug 17, 2022

In order to maintain a consistent look, how about introducing an additional status for the scheduler element - Not required, which will be displayed when there are no tasks registered?

This would still keep the need for a schedule present in the forum admin's mind, but would make it clear that with the current set of extensions installed it is not needed, therefore would not cause unneccessary support/help requests?

@SychO9
Copy link
Member

SychO9 commented Aug 17, 2022

I agree with Daniël, I prefer only showing the status if it's necessary rather then adding more statuses as that sounds like over complicating it for the end user.

@imorland imorland requested a review from a team as a code owner November 3, 2022 22:31
@luceos
Copy link
Member

luceos commented Nov 4, 2022

I am starting to like this a lot. I also think it suffices for a first version, but I also think we need to make things more idiot proof and more intuitive in the future (2.x or sooner). The issue I feel is that we try to cramp too much in that little status bar, an advanced page would be far more helpful if we add more information on what for instance a scheduler is, why it is needed and why this seems to be required now. We want to prevent too much support load, and for that reason we need to be transparent about these things.

@imorland
Copy link
Member Author

imorland commented Nov 4, 2022

I am starting to like this a lot. I also think it suffices for a first version, but I also think we need to make things more idiot proof and more intuitive in the future (2.x or sooner). The issue I feel is that we try to cramp too much in that little status bar, an advanced page would be far more helpful if we add more information on what for instance a scheduler is, why it is needed and why this seems to be required now. We want to prevent too much support load, and for that reason we need to be transparent about these things.

Absolutely agree with you.

@imorland imorland self-assigned this Nov 7, 2022
@imorland imorland added this to the 1.6 milestone Nov 7, 2022
@SychO9 SychO9 modified the milestones: 1.6, 1.7 Nov 15, 2022
@imorland imorland merged commit 47d2053 into main Nov 19, 2022
@imorland imorland deleted the im/admin-status branch November 19, 2022 22:23
@SychO9 SychO9 added the javascript Pull requests that update Javascript code label Feb 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
javascript Pull requests that update Javascript code type/feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants