-
-
Notifications
You must be signed in to change notification settings - Fork 1
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
Breakdown of jobs by state #63
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@egze I want to start off by acknowledging and appreciating all your effort. I'm extremely humbled that you find this project useful and have taken time to improve it. I think your additions are great and with some adjustments can get to a point where they can be included.
I very much like the filtering by state, and the job count for each, but not at the expense of losing the "all" view. I personally find seeing all my jobs in one view very helpful, especially during development and I think from a usability standpoint it means an additional click to inspect a job that isn't executing by default. The functionality is useful though and it is still worthwhile to have.
Changing the time from a date to relative can be very useful but I don't believe it should replace it. There are times that I find it useful to see the date of a job. Making it possible to alternate the time format would be fantastic feature. I would go one step further and make it possible to display in local time, in addition to UTC and relative.
My main concern is the inclusion of Timex. I don't have an issue with the library itself but what the library includes. As you can see that by including Timex it is also including a HTTP client, SSL certs, parsers, etc. The most worrying is tzdata, which by default will make requests to fetch the latest timezone data. Not all projects may need dependencies, nor want them. In regard to the tzdata library, you have to update your config to disable auto-downloading. Not everyone may be aware of this when including this and it is something to be careful with.
The inclusion of Timex is a blocker because of what it depends on. It's excessive for just working out relative time. Elixir 1.18 includes a new Duration struct, which may open up an alternative path to achieve relative time outside of Timex in future. Or it may be possible to leverage the browser and use Intl.RelativeTimeFormat, but that may be tricky to include client side JS via Live Dashboard. Another option forward would be to not include Timex but simply check if it is present and if so then provide the ability to display time relatively.
I would like to reiterate that I truly appreciate your effort and skill with this proposal. If would like to split out the relative time and job filtering into separate PRs that will help expedite getting one feature in whilst the other continues to progress. Thanks again.
lib/oban/live_dashboard.ex
Outdated
<p class="font-weight-bolder"><%= job.worker %></p> | ||
<pre class="args font-weight-lighter text-muted"><%= truncate(inspect(job.args)) %></pre> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's worthwhile including the job's args here. Firstly, that's not the intention of this dashboard, it's to give a simple overview of jobs. Secondly, for jobs with large args it becomes almost useless as it's very unlikely to provide meaningful insight within 50 characters.
Following up on my earlier comment, I also want to avoid custom mark up etc. In this instance, it's very much a departure from the norm and should be avoided.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The args are useful to me, when the args are dynamic - it's nice to quickly spot a specific value. Also I have ideas to implement search and then filtering by args and showing them makes even more sense.
I was inspired by the layout of Oban.Web and they have it https://getoban.pro/oban
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not the intention of this dashboard, it's to give a simple overview of jobs
I see it a bit differently. I would like it to have the same feature set as Oban Web. Some missing things are: search, ability to retry/execute jobs, charts.
Please have a think about this direction. I would love to contribute and eventually reach this state with your library. But would totally get if you want to keep it simple.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking some more on this, I have warmed to the idea of include the args along side the worker name. I do agree it can be useful.
I see it a bit differently. I would like it to have the same feature set as Oban Web.
If you want those features then there is a solution already available. The reason why I created this alternative was because there was no solution to assist during development, to inspect and debug background jobs. The people who develop Oban Web fund the development of Oban and I have no intention of using this to take away someone's lunch, so to speak.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oban Web is going open source, so it's not taking anyones lunch. And I would use it actually, if it worked with SQLite. But only Oban works with it. Web and Pro have some things that only work with Postgres.
I tried to not use Timex, and reach for JS. But unfortunately it's not cleanly possible at the moment. See discussion: phoenixframework/phoenix_live_dashboard#455 What I can offer is: implement the relative_time feature without the Timex dependency. Someone already did it: https://gist.github.com/tlemens/88e9b08f62150ba6082f478a4a03ac52 And also we can make it configurable. additional_pages: [
oban: {Oban.LiveDashboard, relative_timestamp: true}
] If What do you think? EDIT: I removed Timex and formatted the value with The discussion in the phoenix_live_dashboard repo is going quite good and we may have better support for injecting JS soon. Then I can do another go at it to solve it in frontend. |
That makes sense. I'll bring the EDIT: I did it 95ab7b1. Also cleaned up the markup a bit and removed some css classes I wanted to use, but never did. |
@evilmarty But I will also do my best to work on the current PR if you are still interested. Please check if there's still something you want to change. |
@egze I have been toying with another approach for relative time that does not require any additional libraries. To give you a hint, it's used by format_uptime. Your suggestion to use be able to configure this may be a solution though I would like to try supporting a toggle option if possible. It may be better to have that as a separate PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add tests for filtering between states.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done a68eec2
I noticed that I had to do a Oban.LiveDashboardTest.Repo.delete_all(Oban.Job)
in the test, because somehow the values from other tests were still there. Maybe something that you can look into later. Ideally the queries should be rolled back after each test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the heads up. I'll look into proper tear downs.
Co-authored-by: Marty Zalega <[email protected]>
Co-authored-by: Marty Zalega <[email protected]>
Co-authored-by: Marty Zalega <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for make those changes. I have a made another round of comments and I feel comfortable to go ahead after they have been actioned.
Co-authored-by: Marty Zalega <[email protected]>
@evilmarty The new feature with the retry buttons caused some merge conflicts. I tried to fix it, but not sure anymore if I did it cleanly. Please test. |
lib/oban/live_dashboard.ex
Outdated
|> assign(job_state: Map.get(params, "job_state", "executing")) | ||
|> assign(sort_by: Map.get(params, "job_state")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be moved to handle_params
otherwise sort_by
and job_state
won't get updated except for when the view is re-mounted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. handle_params
deserves a refactor later IMO.
I added navigation to the page and it makes it possible to filter jobs by state. For example I only want to observe jobs in
executing
state.Also improved some small things in the list table:
We can also make it all configurable.