NIFI-2795 Sys Diagnostics in Cluster UI#1042
NIFI-2795 Sys Diagnostics in Cluster UI#1042jvwing wants to merge 3 commits intoapache:masterfrom jvwing:NIFI-2795-cluster-ui-sysdiag-3
Conversation
mcgilman
left a comment
mcgilman
left a comment
There was a problem hiding this comment.
Thanks for the PR! Just reading through the proposed changes this looks like a great addition. I've made a few comments inline.
I haven't had a chance to run the PR yet but I wanted to make sure that we have a good experience when the user has access to the controller (to be able to use the cluster table) but not access to the system details. Currently, I'm assuming that it would print that the user was unauthorized.
We should be able to check for this ahead of time and then conditionally add the tabs. I thought that we were already doing this for the 'system diagnostics' link in the summary page but apparently not.
Thoughts?
| tab.grid.resizeCanvas(); | ||
| } | ||
| } catch (ex) { | ||
| console.error("Failed to resize tab", tab, ex); |
There was a problem hiding this comment.
Is there a reason why this may fail? Maybe check ahead of time to prevent the exception and log message?
There was a problem hiding this comment.
There is no good reason for it to fail, I'll change this.
| }] | ||
| }; | ||
|
|
||
| var clusterTabs = [nodesTab, systemTab, jvmTab, flowFileTab, contentTab]; |
There was a problem hiding this comment.
Does it make sense to conditionally add some tabs when the user does not have access to them? Specifically, retrieving system diagnostics requires explicit access. We can add this to the CurrentUserEntity to know when the user has permissions to this (similar to the checks for canAccessController/canModifyController).
There was a problem hiding this comment.
That's a great idea, @mcgilman, I'll look into it.
|
I added server- and client-side modifications to pre-authorize the system diagnostics API call, and removed the unnecessary error handling code. |
|
Reviewing... |
|
@jvwing While reviewing I stumbled into an existing bug filtering the tables in the cluster page. I've addressed it here [1]. Hoping you could just review what I did so I could include it in your PR. Thanks! [1] mcgilman@45d9fc3 |
|
@mcgilman Your changes look good to me. I had noticed that filter behavior, but I wasn't sure how intentional it was. Your fix does allow filtering by the full address or status text as displayed, rather than just one column value, and that seems more intuitive to users. Should I merge that commit into the PR branch? |
|
@jvwing I'll just squash those changes into your commit before I push. Just wanted to communicate before I do that. Thanks! |
|
@jvwing Just finished up testing and everything looks good. This has been merged to master. I did find another minor issue with sorting the table but I updated the sort method accordingly. Thanks! |
|
Thank you, @mcgilman. |
2 participants
Reworked the existing cluster UI to provide tabs containing multiple data tables. Added views for System, JVM, FlowFile Storage and Content Storage diagnostics. This is a UI-only change built on top of the existing System Diagnostics API.