-
Notifications
You must be signed in to change notification settings - Fork 188
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
Slave Usage Monitoring UI #1405
Conversation
A few comments based on your task list above:
Will look more at the actual js and leave comments there shortly |
The actual js has a lot of room for improvement. Modularization is one I'm working on now. I think that review is better saved for once the design is decided since it can change a lot before that. |
cool! We need to merge the data with the slave APIs so that they should hostname and cpusUsed vs cpusAllocated (as opposed to the raw no.) Maybe that was obvious / not implemented yet, wanted to make sure! |
Yep we talked about using the attribute values (falling back to the resource ones) as the denominator for getting the usage percentages already And yes @darcatron , would be nice if we sub that slave ID in with the hostname, would be much easier to reason about/read in the ui that way |
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.
What you have here will function. But, in terms of having cleaner, easier to uderstand code in react. I think it might be better to separate these out into more components.
Right now you have the one large SlaveUsage
componenet. I think you could easily have a component for each Slave box (that can take props like the warn/crit levels and actual numerical stats), and maybe even a sub component for the info box that has the raw stats displayed
@@ -2,14 +2,14 @@ import { buildApiAction, buildJsonApiAction } from './base'; | |||
|
|||
export const FetchSlaves = buildApiAction( | |||
'FETCH_SLAVES', | |||
{url: '/slaves'} |
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.
why did you remove all of the leading slashes here?
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 buildApiAction
already has /api/
so it's just an extra slash. ../api//slaves
will work, but it's not proper so I figured i'd remove it. I can drop it from the rest of the files too, but prob better for another 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.
var warningStyle = 'warning'; | ||
var okStyle = 'success'; | ||
|
||
if (isStatCritical(slave, props.cpusUsedStat) || isStatCritical(slave, props.memoryBytesUsedStat) || isStatCritical(slave, props.numTasksStat)) { |
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.
we should probably catch the exceptions you are throwing in the case where we can't properly find the stats. It'd be better to show less stats (excluding the ones that threw an exception), rather than throwing js errors all the way up
}; | ||
|
||
const slaveWithStats = (slave, index, bsStyle, glyphicon) => ( | ||
<Dropdown key={slave.slaveId} id={index.toString()}> |
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.
Any reasoning behind using dropdown here? Seems like an odd component choice
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 dropdown is the little stat menu you get when clicking on a slave btn. Using Dropdown over DropdownMenu allows more styling flexibility. Not sure if that's what you were talking about
SingularityUI/app/rootComponent.jsx
Outdated
@@ -36,6 +36,7 @@ const rootComponent = (Wrapped, title, refresh = _.noop, refreshInterval = true, | |||
}).catch((reason) => { | |||
// Boot React errors out of the promise so they can be picked up by Sentry | |||
setTimeout(() => { | |||
console.log(reason.stack); |
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.
don't forget to remove these after you're done testing ;)
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'm never done testing 😈
@ssalinas got the updates in for the default values. Once you think it's correct for staging, i'll set our config for shortening the slave name to |
@@ -239,6 +239,10 @@ public String getRedirectOnUnauthorizedUrl() { | |||
return redirectOnUnauthorizedUrl; | |||
} | |||
|
|||
public boolean isShortenSlaveUsageHostname() { |
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.
FYI @darcatron , without the getter here, this value is not available when rendering the template, so {{ shortenSlaveUsageHostname }}
would come back empty in the rendered template
Just related to this, or perhaps for potential inspiration: we are currently doing similar monitoring of our Mesos agent usage and utilization via https://github.com/Capgemini/mesos-ui (video at https://youtu.be/bKbOod8Pn4E) |
This is the front-end for the slave usage tracking. Definitely a work in progress, but the general structure is there. Feel free to edit the task lists.
Design Tasks:
Functionality Tasks:
Updated - 2/21/2017
cc: @ssalinas @wsorenson @tpetr
BE: #1400