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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added top consumer card for storage dashboard #349

Added top consumer card for storage dashboard #349

gnehapk commented Apr 12, 2019

coveralls commented Apr 12, 2019

coveralls commented Apr 12, 2019 •

edited

Loading

gnehapk commented Apr 12, 2019

rawagner Apr 12, 2019

gnehapk Apr 12, 2019

rawagner Apr 12, 2019

rawagner Apr 12, 2019

rawagner Apr 12, 2019

atiratree Apr 12, 2019

gnehapk Apr 14, 2019

atiratree Apr 12, 2019

gnehapk Apr 14, 2019

atiratree Apr 12, 2019

gnehapk Apr 14, 2019

atiratree Apr 12, 2019

gnehapk Apr 14, 2019

atiratree Apr 12, 2019

gnehapk Apr 14, 2019

gnehapk commented Apr 14, 2019

atiratree Apr 15, 2019

gnehapk Apr 15, 2019

gnehapk commented Apr 15, 2019

atiratree Apr 15, 2019

atiratree Apr 15, 2019

atiratree Apr 15, 2019

gnehapk commented Apr 15, 2019

Added top consumer card for storage dashboard #349

Added top consumer card for storage dashboard #349

Conversation

gnehapk commented Apr 12, 2019

coveralls commented Apr 12, 2019

Pull Request Test Coverage Report for Build 1184

💛 - Coveralls

coveralls commented Apr 12, 2019 • edited Loading

Pull Request Test Coverage Report for Build 1230

💛 - Coveralls

gnehapk commented Apr 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnehapk commented Apr 14, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnehapk commented Apr 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnehapk commented Apr 15, 2019

coveralls commented Apr 12, 2019 •

edited

Loading