-
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
Resource Usage UI #1592
Resource Usage UI #1592
Conversation
/cc @darcatron |
I changed the colors used by the bar chart/Breakdown component to use the bootstrap contextual backgrounds (http://getbootstrap.com/css/#helper-classes-backgrounds) when possible, supplemented by similar generic colors: These match much better with our newer design elements. As far as what information they are conveying on the utilization page, each section is the actual number of requests that are under or over utilizing CPU/memory out of the total number of requests. I'm also adding a section showing the 24 average for overall cpu/mem utilization. |
<div className="col-md-10"> | ||
<div className="row"> | ||
<div className="col-md-3"> | ||
<div className="aggregate"> |
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 seems like a good thing to break out into a separate component.
import Utils from '../../../utils'; | ||
import { STAT_NAMES, HUNDREDTHS_PLACE } from '../Constants'; | ||
|
||
const getPctSlaveUsage = (slaves, slaveUsages, usageCallback, resourceCallback) => { |
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.
Mapper
might be a more descriptive suffix than Callback
for the parameter names. You could also avoid passing around the slaves
and slaveUsages
parameters around with
const getPctSlaveUsage = (usageMapper, resourceMapper) => (slaves, slaveUsages) => { //...}
const getCpuUtilizationPct = getPctSlaveUsage(
usage => usage.cpusUsed,
slave => Utils.getMaxAvailableResource(slave, STAT_NAMES.cpusUsedStat)
);
const getMemUtilizationPct = getPctSlaveUsage(
usage => usage.memoryBytesUsed,
slave => Utils.getMaxAvailableResource(slave, STAT_NAMES.memoryBytesUsedStat)
);
const totalResource = slaves.map(resourceCallback) | ||
.reduce((acc, val) => acc + parseFloat(val), 0); | ||
|
||
return Utils.roundTo((totalUsage / totalResource) * 100, HUNDREDTHS_PLACE); |
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 line might be good to abstract out into a helper function since it does one very specific thing.
<h4>Current</h4> | ||
<div className="row"> | ||
<div className="col-xs-12"> | ||
<div className="aggregate graph col-xs-2"> |
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 is very similar to ClusterAggregates.jsx
. I'd strongly suggest creating a new component.
@@ -56,6 +56,7 @@ const maybeLink = (name, value) => { | |||
}; | |||
|
|||
const SlaveResourceHealthMenuItems = ({stats}) => { | |||
stats = _.filter(stats, (stat) => _.values(STAT_NAMES).includes(stat.name)); |
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.
If you're going to use includes
(ES2016), I'd suggest that you don't use _.contains
elsewhere in your code. You can also directly use Array.prototype.filter
used={utilization.cpuUsed / utilization.numTasks} | ||
style={utilization.cpuUsed >= utilization.cpuReserved ? 'danger' : null} | ||
> | ||
<p>{Utils.roundTo(utilization.cpuUsed / utilization.numTasks, 2)} of {utilization.cpuReserved / utilization.numTasks} CPU reserved</p> |
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.
Move these percentage calculations into a separate utility function instead of using the magic number 2
all over the place.
|
||
return utilization ? ( | ||
<CollapsableSection id="request-utilization" title="Resource usage" subtitle="(past 24 hours)" defaultExpanded={isCpuOverAllocated}> | ||
{isFetching ? <div className="page-loader fixed" /> : attributes} |
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 loading component would be good to abstract out too.
@@ -80,7 +92,7 @@ export default class RequestFilters extends React.Component { | |||
return ( | |||
<NavItem | |||
key={index} | |||
className={classNames({'separator-pill': _.contains([3, 5], index)})} | |||
className={classNames({'separator-pill': _.contains([3, 5, 7], index)})} |
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.
???
margin-left : 10px | ||
width : 160px | ||
|
||
small |
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 would override the color of <small>
tags everywhere. If you actually want to do that, move this rule out of the slaveUsage.styl
, otherwise scope this override to just this page.
This one is looking good. Already found a few requests that were really over-allocated using it. Going to merge so @baconmania can work off of some of this to eventually add disk utilization details once we gather them as well |
New Features
/utilization
page (formerly/slave-usage
). Max over/under CPU and memory numbers are linked to the corresponding request. Colored sections of the breakdown link to the appropriate filter on the Requests page.Modifications
/slave-usage
page has been renamed to/utilization
. The tiles within the Slave Health section have had their details replaced by popovers to eliminate them being covered by the tooltip component. Overall layout was improved and code refactored to better align with project standards.TODO: