-
-
Notifications
You must be signed in to change notification settings - Fork 718
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
Include scheduler logs in performance_report #4781
Comments
@quasiben in case anyone on your team wants to add this |
@charlesbluca do you have time to look at this issue? If you do, that'd be great especially given recent experience with performance_report updates |
Sure! I can look into this |
I have a pretty simple implementation of this working: Happy to open a PR for this, although a thought that crossed my mind while working on this is that scheduler (and worker) logs might be easier to incorporate into the performance reports if we migrate some of the Tornado app information (where scheduler/worker logs currently reside) into the Bokeh dashboard's "Info" tab instead of just redirecting. Based on the nested nature of the Tornado app, I thought this might be a good place to try out something like the nested HTML reprs @jacobtomlinson has been working on. |
Woot. Hooray for the fast turnaround. I know that this is just an early/rough draft, but two small comments (you may already have these in mind)
If we're being bound by Bokeh's table plot type then I would encourage us to switch to generating raw HTML. My experience has been that Bokeh's tables don't provide much, while they do restrict creativity a bit. Generating raw HTML for tables is pretty easy. |
Are we okay with including internal CSS on the dashboard? I've found that a combo of that with Bokeh's I'm generally fine with generating raw HTML, my biggest concern there would be if that can be dynamically generated in case we want to have a streamed version of this widget in the dashboard itself. Is there any examples within the dashboard of general raw HTML for something like this? |
Yeah, right now I think that it makes sense to optimize for static HTML, either for the performance report, or for the dashboard info pages (we serve static log reports there as well) 99% of the time I want logs it's to look at something that has already happened, rather than to look at ongoing activity. I think that if removing the option to stream logs can give us a better reader experience then we should go ahead with it. |
You may want to check out http://localhost:8787/info/main/logs.html to see the current state of the dump of logs that we have. I suspect that we could improve on this in the future (although I think that getting logs into the performance report is higher priority) |
Cool! Also, I encourage you to think outside the Bokeh box here. It's a fine styling to mimic, but by no means special. We can break out of / extend that style if we find it helpful. For one thing, we probably don't need to restrict the space of the table in the same way that Bokeh does. |
For example, if using the table forces us to specify a fixed size then let's drop the table entirely and use |
The sizing here is less of a table problem and more of a Bokeh app quirk that might be going over my head - in general, to add the raw HTML to the app, I've been wrapping it in a I'm okay with dropping the idea of a log table, though if this is to get around the sizing issue that might just require some Bokeh debugging. |
You're probably already looking at it, but the code in Scheduler.performance_report that generates the first page of the performance report might be a good model. That manages raw HTML and seems to avoid the sizing issue that you're running into. |
Inspecting the HTML on that page, it looks like it has the same behavior of my table, in that regardless of the sizing policy specified, the containing In that case, it might be best to go with your suggestion, and use simple |
I'm curious Charles, what are your thoughts? You're as likely as anyone
here to have to look at logs. What looks good to you here and what looks
bad?
…On Fri, Jun 11, 2021 at 10:01 AM Charles Blackmon-Luca < ***@***.***> wrote:
Here's what something like that would look like:
[image: image]
<https://user-images.githubusercontent.com/20627856/121706311-82ed4580-caa3-11eb-9e99-446fda2b9eeb.png>
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#4781 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACKZTCTSX6X2MA5H7GCMKDTSIQLXANCNFSM44BSY7BA>
.
|
From an appearance perspective, I find this approach a little bland, though that might be expected since this is just some slightly formatted terminal output. I personally feel like in the long run, output like this might look better within the context of a larger page with collapsible elements for the scheduler/workers, where all of their logs could be viewed maybe with some additional info. But for now this should be good, I think I would just want to make the styling for warning/critical/error logs more visually distinct from info logs, and maybe do something visually appealing with the log level (an icon to the left of the log, for example). From a UX perspective, I could see this being a chore to navigate in cases where there is an exceptional amount of logs, as it is essentially equivalent to scrolling through the logs on the terminal. That was part of the reason I initially gravitated towards a Bokeh table, as that offered sorting out of the box along with the potential for being filterable with some extra work. This is definitely something we could do with raw HTML, but it would require some server-side JS that I'm not really sure we would want. Currently, I've only been testing these changes with trivial commands that generate minimal logs. I will try running this with more complicated scripts that generate logs closer to what the average user would have to look at. |
I agree with you about the bland nature. I notice that you're using bold/italics for different levels. It might be worth adding a little bit of inline CSS to the I agree that filtering would be good, but I also agree unfortunately that I don't know a good way to do this without JS. cc'ing also @jacobtomlinson who might have suggestions around the right way to display these. |
That looks pretty good to me? Shall we try merging that in and then leaving improvement for a later PR? |
Sounds good to me! Opened in #4909 |
We might want to add a new tab in the performance report that includes the logs from the scheduler
The text was updated successfully, but these errors were encountered: