Added top consumer card for storage dashboard #349

gnehapk · 2019-04-12T07:46:17Z

coveralls · 2019-04-12T07:50:35Z

Pull Request Test Coverage Report for Build 1184

32 of 39 (82.05%) changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.1%) to 87.211%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/components/StorageOverview/TopConsumers/TopConsumers.js	14	21	66.67%

Totals
Change from base Build 1181:	-0.1%
Covered Lines:	3346
Relevant Lines:	3675

💛 - Coveralls

coveralls · 2019-04-12T07:50:35Z

Pull Request Test Coverage Report for Build 1230

33 of 36 (91.67%) changed or added relevant lines in 3 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.02%) to 87.352%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/components/StorageOverview/TopConsumers/TopConsumers.js	14	15	93.33%
src/utils/utils.js	1	3	33.33%

Totals
Change from base Build 1223:	0.02%
Covered Lines:	3378
Relevant Lines:	3707

💛 - Coveralls

gnehapk · 2019-04-12T07:55:14Z

@rawagner @mareklibra Please review

rawagner · 2019-04-12T07:53:21Z

src/components/StorageOverview/TopConsumers/TopConsumers.js

+import { getTopConsumerVectorStats } from '../../../selectors';
+
+const TopConsumersBody = ({ stats }) => {
+  let results = 'Not data available';


typo - should be No data available

rawagner · 2019-04-12T08:01:00Z

src/selectors/prometheus/selectors.js

+export const getTopConsumerVectorStats = result => {
+  let maxVal = 0;
+
+  forEach(result, namespace => {


why not ES6 forEach ?

rawagner · 2019-04-12T08:01:32Z

src/selectors/prometheus/selectors.js

+  let largestLengthArrayIndex = 0;
+
+  // timestamps count and values are not same for all the namespaces. Hence, finding the largest length array and taking its timestamps value as x-axis point
+  forEach(result, (namespace, index) => {


why not ES6 forEach ?

rawagner · 2019-04-12T08:05:39Z

src/components/StorageOverview/TopConsumers/TopConsumers.js

+      <Row>
+        <Col lg={12} md={12} sm={12} xs={12}>
+          <LineChart
+            className="kubevirt-top-consumers__body"


I dont see this classname in any scss file

atiratree · 2019-04-12T10:35:02Z

src/components/StorageOverview/TopConsumers/TopConsumers.js

+  if (stats.length) {
+    const columnsConf = getTopConsumerVectorStats(stats);
+    const { columns, unit } = columnsConf;
+    const formatTime = x => {


new Date(x).toISOString().substring(11,16) is a bit simpler. Could you please put this into utils? And maybe rename it as formatToShortISOTime ?

toISOString method will return the time in zero UTC time zone. Instead of that toString method can be used.

atiratree · 2019-04-12T10:44:25Z

src/selectors/prometheus/selectors.js

@@ -28,3 +28,40 @@ export const getLastUtilizationStat = response => {
  const history = getUtilizationVectorStats(response);
  return history ? history[history.length - 1] : null;
 };
+
+export const getTopConsumerVectorStats = result => {


can you please move this to prometheus/storage.js ? it is getting overcrowded here IMO.

atiratree · 2019-04-12T10:45:55Z

src/selectors/prometheus/selectors.js

+export const getTopConsumerVectorStats = result => {
+  let maxVal = 0;
+
+  result.forEach(namespace => {


you can use map here instead

I don't think map can be used here. Map method creates a new array with the results of calling a provided function on every element in the calling array. Here, I don't want to create a new array after performing the required actions. I just want to iterate over the array in order to find the max element. Hence, used forEach.

atiratree · 2019-04-12T10:47:45Z

src/selectors/prometheus/selectors.js

+  let maxVal = 0;
+
+  result.forEach(namespace => {
+    namespace.values.forEach(value => {


you could also use lodash flatMap here and have one Math.max call

yes, flatMap will be a better solution.

atiratree · 2019-04-12T10:50:51Z

src/selectors/prometheus/selectors.js

+    });
+  });
+
+  const maxCapacityConveretd = { ...formatBytes(maxVal) };


gnehapk · 2019-04-14T13:23:25Z

Incorporated the review comments. @rawagner @mareklibra @suomiy Please review.

atiratree · 2019-04-15T07:19:44Z

src/selectors/prometheus/storage.js

+  let flattenedResult = [];
+
+  // returns all the namespace.values in an array
+  flattenedResult = flatMap(result, namespace => namespace.values);


you can name the variables accordingly and we might not need the comments
e.g.

namespaceValues = allValues = // allBytes ?

gnehapk · 2019-04-15T09:10:57Z

Rebased the PR. @rawagner @suomiy Please review.

atiratree · 2019-04-15T09:23:13Z

src/selectors/prometheus/storage.js

+  let namespaceValues = [];
+  let allBytes = [];
+
+  namespaceValues = flatMap(result, namespace => namespace.values);


can be const

atiratree · 2019-04-15T09:23:22Z

src/selectors/prometheus/storage.js

+  let allBytes = [];
+
+  namespaceValues = flatMap(result, namespace => namespace.values);
+  allBytes = flatMap(namespaceValues, value => parseNumber(value[1]));


atiratree · 2019-04-15T09:23:47Z

sass/components/StorageOverview/top-consumer.scss

+
+.kubevirt-top-consumer__time-duration {
+  float: right;
+  padding-right: 17px;


please change to rem

gnehapk · 2019-04-15T10:13:39Z

@rawagner @suomiy Amended the PR. Please review.

gnehapk mentioned this pull request Apr 12, 2019

Integrated top consumer card for storage dashboard kubevirt/web-ui#298

Merged

kubevirt-bot added the size/L label Apr 12, 2019

gnehapk mentioned this pull request Apr 12, 2019

[WIP] Added top consumers card #334

Closed

rawagner suggested changes Apr 12, 2019

View reviewed changes

gnehapk force-pushed the storage-top-consumer branch from 691f862 to 95c0933 Compare April 12, 2019 10:32

atiratree suggested changes Apr 12, 2019

View reviewed changes

gnehapk force-pushed the storage-top-consumer branch 2 times, most recently from 0fd9058 to 815f232 Compare April 14, 2019 12:49

atiratree reviewed Apr 15, 2019

View reviewed changes

gnehapk force-pushed the storage-top-consumer branch from 815f232 to c4a040b Compare April 15, 2019 09:01

atiratree suggested changes Apr 15, 2019

View reviewed changes

Added top consumer card for storage dashboard

74657b9

gnehapk force-pushed the storage-top-consumer branch from c4a040b to 74657b9 Compare April 15, 2019 09:55

rawagner merged commit e59f281 into kubevirt:master Apr 15, 2019

Added top consumer card for storage dashboard #349

Added top consumer card for storage dashboard #349

Uh oh!

Conversation

gnehapk commented Apr 12, 2019

Uh oh!

coveralls commented Apr 12, 2019

Pull Request Test Coverage Report for Build 1184

💛 - Coveralls

Uh oh!

coveralls commented Apr 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 1230

💛 - Coveralls

Uh oh!

gnehapk commented Apr 12, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnehapk commented Apr 14, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnehapk commented Apr 15, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnehapk commented Apr 15, 2019

Uh oh!

Uh oh!

coveralls commented Apr 12, 2019 •

edited

Loading