Add storage efficiency card on persistent dashboard. #6104

a2batic · 2020-07-25T21:00:54Z

Thanks James Espy, Paul Cuzner , for the providing the metrics.
Signed-off-by: Kanika Murarka kmurarka@redhat.com

kyoto · 2020-07-26T23:57:44Z

frontend/packages/ceph-storage-plugin/src/constants/queries.ts

@@ -87,6 +89,13 @@ export const CAPACITY_BREAKDOWN_QUERIES = {
    'max(ceph_pool_max_avail * on (pool_id) group_left(name)ceph_pool_metadata{name=~"(.*file.*)|(.*block.*)"})',
 };

+export const POOL_STORAGE_EFFICIENCY_QUERIES = {


FYI @openshift/openshift-team-monitoring

nice, thank you!

that is an incarnation of centralizing queries, correct?

Ah no. The centralizing PR is #6128

gnehapk · 2020-07-27T04:00:53Z

...ponents/dashboard-page/storage-dashboard/storage-efficiency-card/storage-efficiency-card.tsx

+
+import './storage-efficiency-card.scss';
+
+const getGaugeValue = (data) => _.get(data, 'data.result[0].value[1]');


Can you move this to utils , so that we can reuse this in other cards ?
We wont miss the item this way during refactoring phase.

afreen23 · 2020-07-27T04:16:41Z

...ts/dashboard-page/storage-dashboard/storage-efficiency-card/storage-efficiency-card-item.tsx

+  const savingsValue = Number(saved);
+  if (saved) {
+    const savedBytes = humanizeBinaryBytes(savingsValue).string;
+    stats = savingsValue <= 0 ? 'No Savings' : savedBytes;


Will you ever get a negative value here ?

Good catch, no, it can be 0 or more only

afreen23 · 2020-07-27T04:25:13Z

...ponents/dashboard-page/storage-dashboard/storage-efficiency-card/storage-efficiency-card.tsx

+
+import './storage-efficiency-card.scss';
+
+const getGaugeValue = (data) => _.get(data, 'data.result[0].value[1]');


Can you move this to utils , so that we can reuse this in other cards ?
We wont miss the item this way during refactoring phase.

afreen23 · 2020-07-27T04:27:38Z

...ts/dashboard-page/storage-dashboard/storage-efficiency-card/storage-efficiency-card-item.tsx

+      <div className="co-inventory-card__item">
+        <div className="ceph-storage-efficiency-card__row-title">{title}</div>
+        <div className="ceph-storage-efficiency-card__row-status-item">


Since these are rows for the ItemBody and to make BEM naming align with the actual purpose

Suggested change

<div className="co-inventory-card__item">

<div className="ceph-storage-efficiency-card__row-title">{title}</div>

<div className="ceph-storage-efficiency-card__row-status-item">

<div className="co-inventory-card__row-item">

<div className="ceph-storage-efficiency-card__row-item-title">{title}</div>

<div className="ceph-storage-efficiency-card__row-item-status">

Also ItemBody can be RowItem

We have to keep co-inventory-card__row class as it is becasue it is used across dashboard https://github.com/openshift/console/blob/master/frontend/packages/console-shared/src/components/dashboard/inventory-card/inventory-card.scss#L4.

Sure, I missed in flow :D

I dont find this naming right though.

cloudbehl · 2020-07-27T09:38:22Z

@a2batic can you add screenshots?

afreen23 · 2020-07-28T05:28:25Z

@a2batic We too want ot make it generic just like breakdown card. Instead of maintaining same copies for different plugins.
It can be done in a followup PR as well !

afreen23 · 2020-07-28T09:14:54Z

...ponents/dashboard-page/storage-dashboard/storage-efficiency-card/storage-efficiency-card.tsx

+  const compressionRatioProps = {
+    ratio,
+    isLoading: !poolCapacityRatioResult,
+    error: !!poolCapacityRatioResultError,
+  };


Suggested change

const compressionRatioProps = {

ratio,

isLoading: !poolCapacityRatioResult,

error: !!poolCapacityRatioResultError,

};

const compressionRatioProps = {

ratio,

isLoading: !poolCapacityRatioResult,

error: !!poolCapacityRatioResultError,

infotext: infotext

handler: getCompressionRatio,

};

afreen23 · 2020-07-28T09:29:50Z

...ts/dashboard-page/storage-dashboard/storage-efficiency-card/storage-efficiency-card-item.tsx

+const ItemBody: React.FC<ItemBodyProps> = React.memo(
+  ({ title, stats, infoText, isLoading, error }) => {
+    let status: React.ReactElement;
+    if (isLoading) {
+      status = <div className="skeleton-text ceph-storage-efficiency-card__item-body--loading" />;
+    } else if (error || !stats) {
+      status = <span className="co-dashboard-text--small text-muted">Not available</span>;
+    } else {
+      status = <span className="ceph-storage-efficiency-card__item-text">{stats}</span>;
+    }
+    return (
+      <div className="co-inventory-card__item">
+        <div className="ceph-storage-efficiency-card__item-title">{title}</div>
+        <div className="ceph-storage-efficiency-card__item-status">
+          {status}
+          <FieldLevelHelp>{infoText}</FieldLevelHelp>
+        </div>
+      </div>
+    );
+  },
+);


We can make this component more generic, renaming to StorageEfficiencyItem and avoid using SavedItem, CompressionItem.

You can pass one handler function to it to get the stats:

({ title, data, infoText, isLoading, error, handler }) => { let status: React.ReactElement; const stats = handler(data);

The handler will be your extracted logic for :

if (ratio) { const capacityRatio = Number(ratio); stats = capacityRatio === 0 ? '1:1' : `${Math.round(capacityRatio)}:1`; } if (ratio) { const capacityRatio = Number(ratio); stats = capacityRatio === 0 ? '1:1' : `${Math.round(capacityRatio)}:1`; }

which you can pass as prop;

You can call directly StorageEfficiencyItem as:

<DashboardCardBody className="co-dashboard-card__body--no-padding"> <StorageEfficiencyItem {...compressionRatioProps} /> <StorageEfficiencyItem {...savingsProps} /> </DashboardCardBody>

afreen23 · 2020-07-28T16:52:45Z

...ts/dashboard-page/storage-dashboard/storage-efficiency-card/storage-efficiency-card-item.tsx

+export const EfficiencyItemBody: React.FC<EfficiencyItemBodyProps> = React.memo(
+  ({ title, stats, infoText, isLoading, error, handler }) => {
+    let status: React.ReactElement;
+    const metricStats = stats ? handler() : null;


As per the below suggestion:

Suggested change

const metricStats = stats ? handler() : null;

afreen23 · 2020-07-28T16:52:58Z

...ponents/dashboard-page/storage-dashboard/storage-efficiency-card/storage-efficiency-card.tsx

+  const ratio = getGaugeValue(poolCapacityRatioResult);
+  const saved = getGaugeValue(poolSavedResult);
+
+  const compressionStats = () => {
+    const capacityRatio = Number(ratio);
+    return capacityRatio === 0 ? '1:1' : `${Math.round(capacityRatio)}:1`;
+  };
+
+  const savingStats = () => {
+    const savingsValue = Number(saved);
+    const savedBytes = humanizeBinaryBytes(savingsValue).string;
+    return savingsValue === 0 ? 'No Savings' : savedBytes;
+  };


When getGaugeValue(poolCapacityRatioResult) returns undefined.
Then you will have capacityRatio and savingsValue as 0 and it will render a text 1:1 and No savings.
You dont want to show that , instead you want to have Not available.

You can have :

const compressionStats = () => { if (capacityRatio) { const capacityRatio = Number(ratio); return capacityRatio === 0 ? '1:1' : `${Math.round(capacityRatio)}:1`; }; return capacityRatio; }; const savingStats = () => { if(savingsValue) { const savingsValue = Number(saved); const savedBytes = humanizeBinaryBytes(savingsValue).string; return savingsValue === 0 ? 'No Savings' : savedBytes; } return savingsValue; };

@afreen23 if getGaugeValue(poolCapacityRatioResult) returns undefined, capacityRatio and ratio will be NaN, becasue Number(undefined) gives NaN https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number.

Therefore, we dont need to put if, because I am checking for value to be 0 or not. wdyt?

@a2batic you are missing my point here. What I pointed above is if capacityRatio and savingsValue wont be 0 and they will default to the text. Instead I would at least prefer to set for such cases Not available.
An improvement over stats ? handler() : null; , we can avoid explicitly setting null and let the handler itself respond.

I am alright if you want to keep it that way.

@afreen23, thanks for clarifying, I understand your point, only reasons to put it in element, So we dont have to check for every item in their respective functions.

What about doing this way #6104 (review) ?

a2batic · 2020-07-28T18:37:02Z

/retest

afreen23 · 2020-07-28T19:09:45Z

/lgtm

afreen23 · 2020-07-28T19:11:54Z

/lgtm cancel

#6104 (comment)

afreen23

Lets do this way

afreen23 · 2020-07-28T19:14:19Z

...ponents/dashboard-page/storage-dashboard/storage-efficiency-card/storage-efficiency-card.tsx

+  const savingsProps = {
+    stats: saved,
+    isLoading: !poolSavedResult,
+    error: !!poolSavedResultError,


Suggested change

error: !!poolSavedResultError,

error: !!poolSavedResultError || !saved

afreen23 · 2020-07-28T19:14:33Z

...ponents/dashboard-page/storage-dashboard/storage-efficiency-card/storage-efficiency-card.tsx

+  const compressionRatioProps = {
+    stats: ratio,
+    isLoading: !poolCapacityRatioResult,
+    error: !!poolCapacityRatioResultError,


Suggested change

error: !!poolCapacityRatioResultError,

error: !!poolCapacityRatioResultError || !ratio

afreen23 · 2020-07-28T19:15:40Z

...ts/dashboard-page/storage-dashboard/storage-efficiency-card/storage-efficiency-card-item.tsx

+    } else if (error || !metricStats) {
+      status = <span className="co-dashboard-text--small text-muted">Not available</span>;
+    } else {
+      status = <span className="ceph-storage-efficiency-card__item-text">{metricStats}</span>;


Suggested change

status = <span className="ceph-storage-efficiency-card__item-text">{metricStats}</span>;

status = <span className="ceph-storage-efficiency-card__item-text">{getStats()}</span>;

afreen23 · 2020-07-28T19:16:10Z

...ts/dashboard-page/storage-dashboard/storage-efficiency-card/storage-efficiency-card-item.tsx

+    const metricStats = stats ? handler() : null;
+    if (isLoading) {
+      status = <div className="skeleton-text ceph-storage-efficiency-card__item-body--loading" />;
+    } else if (error || !metricStats) {


Suggested change

} else if (error || !metricStats) {

} else if (error) {

afreen23 · 2020-07-28T19:16:31Z

...ts/dashboard-page/storage-dashboard/storage-efficiency-card/storage-efficiency-card-item.tsx

+import { FieldLevelHelp } from '@console/internal/components/utils';
+
+export const EfficiencyItemBody: React.FC<EfficiencyItemBodyProps> = React.memo(
+  ({ title, stats, infoText, isLoading, error, handler }) => {


Suggested change

({ title, stats, infoText, isLoading, error, handler }) => {

({ title, stats, infoText, isLoading, error, getStats }) => {

Signed-off-by: Kanika Murarka <kmurarka@redhat.com>

afreen23 · 2020-07-29T15:52:15Z

/lgtm

openshift-ci-robot · 2020-07-29T15:52:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: a2batic, afreen23, gnehapk

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~frontend/packages/ceph-storage-plugin/OWNERS~~ [afreen23,gnehapk]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot requested review from bipuladh and gnehapk July 25, 2020 21:01

kyoto reviewed Jul 26, 2020

View reviewed changes

gnehapk reviewed Jul 27, 2020

View reviewed changes

gnehapk approved these changes Jul 27, 2020

View reviewed changes

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 27, 2020

afreen23 suggested changes Jul 27, 2020

View reviewed changes

a2batic force-pushed the storage-efficiency-card branch from 67b74ce to ae36239 Compare July 28, 2020 08:36

afreen23 suggested changes Jul 28, 2020

View reviewed changes

a2batic force-pushed the storage-efficiency-card branch from ae36239 to 3b953b7 Compare July 28, 2020 12:31

afreen23 suggested changes Jul 28, 2020

View reviewed changes

openshift-ci-robot assigned afreen23 Jul 28, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 28, 2020

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 28, 2020

afreen23 suggested changes Jul 28, 2020

View reviewed changes

Add storage efficiency card on persistent dashboard.

153341d

Signed-off-by: Kanika Murarka <kmurarka@redhat.com>

a2batic force-pushed the storage-efficiency-card branch from 3b953b7 to 153341d Compare July 29, 2020 15:45

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 29, 2020

openshift-merge-robot merged commit 8d10a88 into openshift:master Jul 29, 2020

spadgett added this to the v4.6 milestone Jul 30, 2020


		import './storage-efficiency-card.scss';

		const getGaugeValue = (data) => _.get(data, 'data.result[0].value[1]');

	error: !!poolSavedResultError,
	error: !!poolSavedResultError \|\| !saved

	error: !!poolCapacityRatioResultError,
	error: !!poolCapacityRatioResultError \|\| !ratio

	status = <span className="ceph-storage-efficiency-card__item-text">{metricStats}</span>;
	status = <span className="ceph-storage-efficiency-card__item-text">{getStats()}</span>;

	({ title, stats, infoText, isLoading, error, handler }) => {
	({ title, stats, infoText, isLoading, error, getStats }) => {

Add storage efficiency card on persistent dashboard. #6104

Add storage efficiency card on persistent dashboard. #6104

Conversation

a2batic commented Jul 25, 2020 • edited

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

afreen23 Jul 27, 2020 • edited

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

afreen23 Jul 27, 2020 • edited

Choose a reason for hiding this comment

afreen23 Jul 27, 2020 • edited

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

cloudbehl commented Jul 27, 2020

afreen23 commented Jul 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afreen23 Jul 28, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

a2batic Jul 28, 2020 • edited

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

a2batic commented Jul 28, 2020

afreen23 commented Jul 28, 2020

afreen23 commented Jul 28, 2020

afreen23 left a 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

afreen23 commented Jul 29, 2020

openshift-ci-robot commented Jul 29, 2020

a2batic commented Jul 25, 2020 •

edited

afreen23 Jul 27, 2020 •

edited

afreen23 Jul 27, 2020 •

edited

afreen23 Jul 27, 2020 •

edited

afreen23 Jul 28, 2020 •

edited

a2batic Jul 28, 2020 •

edited