Skip to content
This repository has been archived by the owner on Apr 28, 2020. It is now read-only.

Refactor TopConsumers and add Workload and Infrastructure metrics #289

Merged
merged 1 commit into from
Apr 8, 2019

Conversation

rawagner
Copy link
Contributor

No description provided.

@coveralls
Copy link

coveralls commented Mar 25, 2019

Pull Request Test Coverage Report for Build 1055

  • 46 of 65 (70.77%) changed or added relevant lines in 8 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-0.3%) to 87.321%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/components/Dashboard/TopConsumers/ConsumersResults.js 8 9 88.89%
src/components/ClusterOverview/TopConsumers/utils.js 3 5 60.0%
src/components/ClusterOverview/TopConsumers/TopConsumers.js 10 26 38.46%
Files with Coverage Reduction New Missed Lines %
src/selectors/prometheus/selectors.js 1 87.5%
Totals Coverage Status
Change from base Build 1052: -0.3%
Covered Lines: 3133
Relevant Lines: 3440

💛 - Coveralls

@coveralls
Copy link

Pull Request Test Coverage Report for Build 890

  • 33 of 35 (94.29%) changed or added relevant lines in 2 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.03%) to 87.232%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/components/ClusterOverview/TopConsumers/ClusterTopConsumers.js 16 17 94.12%
src/components/ClusterOverview/TopConsumers/TopConsumers.js 17 18 94.44%
Files with Coverage Reduction New Missed Lines %
src/components/ClusterOverview/TopConsumers/TopConsumers.js 1 86.11%
Totals Coverage Status
Change from base Build 875: 0.03%
Covered Lines: 2857
Relevant Lines: 3135

💛 - Coveralls

@rawagner rawagner changed the title WIP: Refactor TopConsumers and add new ClusterTopConsumers component Refactor TopConsumers and add new ClusterTopConsumers component Mar 25, 2019
sass/components/ClusterOverview/consumers.scss Outdated Show resolved Hide resolved
return `${bytes.value} ${bytes.unit}`;
};

const getConsumers = (kind, results, nameLabel, formatLabel) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe this would better fit to selectors

Copy link
Contributor Author

@rawagner rawagner Mar 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved to selectors/prometheus/selectors

};

const getConsumers = metric => {
const max = Math.max(...metric.consumers.map(c => c.usage));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now value will be always relative to the highest consumers, which I don't think is very useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

after discussion with @jelkosz this is what we want. The first consumer bar is 100% and others are relative to the first one.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I talked to @jelkosz and we concluded that it would be good to include some icon with a popover that would explain what is happening. E.g. something something, The value xy is relative to the current higest value (100% bar). It could also include something about the query.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

created a followup #313 - I will ask UXD for help

@rawagner rawagner force-pushed the consumers branch 3 times, most recently from 3912d61 to dd78fa2 Compare March 29, 2019 09:23
@@ -1,10 +1,51 @@
.kubevirt-consumers__bar .progress {
.kubevirt-consumers__bar {
height: 20px;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the em units are preferable in other places as well. We should probably convert the whole dashboard to be consistent with this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@rawagner rawagner force-pushed the consumers branch 5 times, most recently from 1185d99 to dc84b25 Compare March 29, 2019 14:10
@rawagner rawagner changed the title Refactor TopConsumers and add new ClusterTopConsumers component Refactor TopConsumers Mar 29, 2019
@rawagner rawagner changed the title Refactor TopConsumers Refactor TopConsumers and add Workload and Infrastructure metrics Mar 29, 2019
@rawagner rawagner changed the title Refactor TopConsumers and add Workload and Infrastructure metrics WIP: Refactor TopConsumers and add Workload and Infrastructure metrics Apr 1, 2019
@rawagner rawagner force-pushed the consumers branch 8 times, most recently from 310e20d to d74ba77 Compare April 5, 2019 08:38
@rawagner rawagner changed the title WIP: Refactor TopConsumers and add Workload and Infrastructure metrics Refactor TopConsumers and add Workload and Infrastructure metrics Apr 5, 2019
@rawagner
Copy link
Contributor Author

rawagner commented Apr 5, 2019

@suomiy @mareklibra can you please review ?

</DashboardCardHeader>
<DashboardCardBody>
<ConsumersFilter>
{getFormElement(typeDropdown)}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would be maybe good to add some margin between them

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added

space

<DashboardCard>
<DashboardCardHeader>
<DashboardCardTitle>Top Consumers</DashboardCardTitle>
<DashboardCardTitleHelp>help for top consumers</DashboardCardTitleHelp>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re: #313. For now all the metrics behave the same, so we could add the explanation text here until #313 is resolved.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added Consumption bar is relative to biggest consumer which is always 100% Not sure if that is good enough

filter !== 'All' ? metrics[metricKey].consumers.filter(c => c.kind === filter) : metrics[metricKey].consumers;
const max = Math.max(...filteredConsumers.map(c => c.usage));
return filteredConsumers
const getMetricConsumers = ({ consumers }) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should probably go to selectors or some util class (Dashboard/TopConsumers/utils or some other utils?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getMetricConsumers moved to Dashboard/TopConsumers/utils. The other two methods were moved to ClusterOverview/TopConsumers/utils

padding-bottom: 10px;
border-bottom: 1px solid #d1d1d1;
.kubevirt-consumers__dropdown-filter {
width: 49%;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think margin should be more descriptive/generic than width

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Im using Columns + padding as I had some issues on firefox when not using width.

- add Loading state
- add Workload and Infrastructure metrics
@atiratree atiratree merged commit 05a95ea into kubevirt:master Apr 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants