framework icon indicating copy to clipboard operation
framework copied to clipboard

feat: expose queue driver, schedule status

Open imorland opened this issue 3 years ago • 7 comments

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

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

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 avatar Aug 11 '22 22:08 imorland

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 ...

luceos avatar Aug 12 '22 06:08 luceos

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 avatar Aug 12 '22 08:08 imorland

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 👍

imorland avatar Aug 12 '22 08:08 imorland

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.

SychO9 avatar Aug 17 '22 10:08 SychO9

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.

luceos avatar Aug 17 '22 10:08 luceos

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?

imorland avatar Aug 17 '22 11:08 imorland

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.

SychO9 avatar Aug 17 '22 11:08 SychO9

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.

luceos avatar Nov 04 '22 06:11 luceos

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 avatar Nov 04 '22 08:11 imorland