Add Data Consumption Card to Object Service Dashboard #1976

afreen23 · 2019-07-10T10:17:15Z

openshift-ci-robot · 2019-07-10T10:17:29Z

Hi @afreen23. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

frontend/public/components/utils/units.js

afreen23 · 2019-07-17T03:37:49Z

@mareklibra @cloudbehl @rawagner please review it!

mareklibra · 2019-07-17T07:33:31Z

...ackages/noobaa-storage-plugin/src/components/data-consumption-card/data-consumption-card.tsx

+import { DataConsumptionDropdown } from './data-consumption-card-dropdown';
+import { DataConsumptionQueries } from '../../queries';
+import { getMetric, getValue } from '../../utils';
+import './data-consumption-card.scss';


Rest of the code has just single scss import per plugin/application which subsequently @import specialized fragments.
I think we should stay consistent.

IMO, this per-component encapsulation fits better a library than an end-user application.

@spadgett , thoughts?

@christianvogt @vojtechszocs opinion?

My thought is that SCSS is no different than a js module. If a class is specific to a component and no other component, then why not import it at the component level where it belongs. This will also benefit from lazy loading. If we need to refactor / remove the component. Finding the a related CSS code is also easier. It ensures we are being explicit in the dependencies of that component.

We don't use utilities from the global scope. CSS should be no different. I'd even argue that utility classes should also be imported at the component level as well and let webpack chunk those utility classes into a common module at the root. This happens for modules like lodash which is used everywhere.

I'm 100% for co-locating CSS and trying to make as scoped as possible. If we could eventually do what PF4 does, even better.

The dev-console follows this pattern.

rawagner

Some visual issues:
Card header is bigger that others.
Im not sure about those colors in graphs, these are not used anywhere in console AFAIK @lizsurette

...es/noobaa-storage-plugin/src/components/data-consumption-card/data-consumption-card-body.tsx

rawagner · 2019-07-17T07:16:48Z

...oobaa-storage-plugin/src/components/data-consumption-card/data-consumption-card-dropdown.tsx

@@ -0,0 +1,116 @@
+import * as React from 'react';
+import { Dropdown, DropdownToggle, DropdownItem, DropdownPosition } from '@patternfly/react-core';


you should use Dropdown from utils/dropdown.jsx

@rwagner that dropdown dont have the functionality to disable dropdown items. Thats why I used PF4!

rawagner · 2019-07-17T07:20:56Z

...es/noobaa-storage-plugin/src/components/data-consumption-card/data-consumption-card-utils.ts

+
+/* Chart Data Handlers */
+
+export const iopsChartData = (data, key) => {


all these data parsing functions looks very similar to getRangeVectorStats and getInstantVectorStats from graphs/utils.ts. You should reuse them

thanx for telling this, but still for IOPS I need a modified function.

actually it is not valid for my case, the data coming from prom for each dropdown selection is different. I tried to incorporate these but it did not work out.
@rawagner

...ckages/noobaa-storage-plugin/src/components/data-consumption-card/data-consumption-card.scss

...ackages/noobaa-storage-plugin/src/components/data-consumption-card/data-consumption-card.tsx

frontend/packages/noobaa-storage-plugin/src/queries.ts

frontend/packages/noobaa-storage-plugin/src/utils.ts

rawagner · 2019-07-17T07:40:58Z

frontend/packages/noobaa-storage-plugin/src/utils.ts

+  _.get(result, `metric.${metric}`, null);
+
+export const getValue = (result: PrometheusResult): number =>
+  Number(_.get(result, 'value[1]', null));


Number(null) returns 0, I dont think this is safe

In this case it is safe, anyways 0 values are not being plotted on chart.
I intentionally put that, rather than handling null values. I cannot find better approach, if you could suggest something ?

Oh I see! sorry its in utils, I will change it later with the enhancement Pr, because i need to check which components are using it. Please ignore for now.

I fixed it!

lizsurette · 2019-07-17T12:07:14Z

Im not sure about those colors in graphs, these are not used anywhere in console AFAIK @lizsurette

Looks like we are using the new PatternFly 4 Charts with the purple theme in this case, so I think it's good to go! Here is a bit more on the new bar chart and the theme prop:
https://www.patternfly.org/v4/documentation/react/charts/chartbar/

We do have a page calling out all individual chart colors if needed, but I think using a theme in this case works well:
https://www.patternfly.org/v4/design-guidelines/styles/chart-colors

@mceledonia @yuvalgalanti incase you guys have anything additional to add here :)

afreen23 · 2019-07-18T06:35:01Z

Card header is bigger that others.

similar reasons: @rawagner
https://github.com/openshift/console/pull/2040/files#r304337872

...obaa-storage-plugin/src/components/object-data-reduction-card/object-data-reduction-card.tsx

openshift-ci-robot · 2019-07-19T00:11:52Z

@afreen23: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/ok-to-test

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

cloudbehl · 2019-07-19T04:23:34Z

/ok-to-test

afreen23 · 2019-07-19T04:37:31Z

/test e2e-aws-console

gnehapk · 2019-07-19T04:45:37Z

...es/noobaa-storage-plugin/src/components/data-consumption-card/data-consumption-card-utils.ts

+  );
+  const valueP = humanizeBinaryBytesWithoutB(
+    data[1].reduce((total: number, physical: BarChartData) => total + physical.y, 0),
+  );


Nit: Here also you can destruct the valueL and valueP as you did for others humanizeBinaryBytesWithoutB return value.

nope I cant because I am using it multiple times, there will be same variables issues @gnehapk

afreen23 · 2019-07-19T05:13:05Z

/test e2e-aws-console
/test e2e-aws-console-olm

afreen23 · 2019-07-19T05:39:47Z

/test e2e-aws-console

cloudbehl · 2019-07-19T06:01:16Z

/approve

@rawagner can you have a look at this once.

afreen23 · 2019-07-19T10:26:44Z

/test e2e-aws-console-olm
/test e2e-aws-console
/test e2e-aws

rawagner · 2019-07-19T14:41:24Z

/lgtm

afreen23 · 2019-07-19T15:56:44Z

/retest

cloudbehl · 2019-07-19T18:58:29Z

/approve
/lgtm

openshift-ci-robot · 2019-07-19T18:58:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afreen23, cloudbehl, rawagner

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/noobaa-storage-plugin/OWNERS~~ [cloudbehl,rawagner]

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

afreen23 · 2019-07-19T19:40:51Z

/test e2e-aws-console

afreen23 · 2019-07-20T06:25:13Z

/test e2e-aws

openshift-bot · 2019-07-20T08:14:29Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2019-07-20T11:55:30Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 10, 2019

openshift-ci-robot requested review from cloudbehl and mareklibra July 10, 2019 10:17

openshift-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 10, 2019

openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 10, 2019

afreen23 force-pushed the noobaa_data-consumption-card branch from 752b71c to b1b8f7f Compare July 16, 2019 04:19

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 16, 2019

afreen23 changed the title ~~[WIP] Add Data Consumption Card to Object Service Dashboard~~ Add Data Consumption Card to Object Service Dashboard Jul 16, 2019

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 16, 2019

afreen23 force-pushed the noobaa_data-consumption-card branch from b1b8f7f to 01844de Compare July 16, 2019 04:22

spadgett requested changes Jul 16, 2019

View reviewed changes

frontend/public/components/utils/units.js Outdated Show resolved Hide resolved

mareklibra reviewed Jul 17, 2019

View reviewed changes

rawagner suggested changes Jul 17, 2019

View reviewed changes

afreen23 force-pushed the noobaa_data-consumption-card branch from 01844de to 25f6c58 Compare July 18, 2019 21:58

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 18, 2019

afreen23 force-pushed the noobaa_data-consumption-card branch from 25f6c58 to dacc1dd Compare July 18, 2019 23:13

openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 18, 2019

afreen23 commented Jul 18, 2019

View reviewed changes

...obaa-storage-plugin/src/components/object-data-reduction-card/object-data-reduction-card.tsx Outdated Show resolved Hide resolved

openshift-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 19, 2019

gnehapk reviewed Jul 19, 2019

View reviewed changes

afreen23 force-pushed the noobaa_data-consumption-card branch from dacc1dd to c98c3c8 Compare July 19, 2019 12:57

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

afreen23 force-pushed the noobaa_data-consumption-card branch from 433337c to 1a0be82 Compare July 19, 2019 14:40

openshift-ci-robot assigned rawagner Jul 19, 2019

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

afreen23 force-pushed the noobaa_data-consumption-card branch from 1a0be82 to ecb9fcd Compare July 19, 2019 17:07

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

Added Data Consumption Card to Object Service Dashboard

e2b169c

afreen23 force-pushed the noobaa_data-consumption-card branch from ecb9fcd to e2b169c Compare July 19, 2019 18:14

openshift-ci-robot assigned cloudbehl Jul 19, 2019

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

openshift-merge-robot merged commit 8d6207c into openshift:master Jul 20, 2019

spadgett added this to the v4.2 milestone Jul 20, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Data Consumption Card to Object Service Dashboard #1976

Add Data Consumption Card to Object Service Dashboard #1976

afreen23 commented Jul 10, 2019 •

edited

Loading

openshift-ci-robot commented Jul 10, 2019

afreen23 commented Jul 17, 2019

mareklibra Jul 17, 2019

spadgett Jul 19, 2019

christianvogt Jul 19, 2019

rawagner left a comment

rawagner Jul 17, 2019

afreen23 Jul 17, 2019

rawagner Jul 17, 2019

afreen23 Jul 18, 2019

afreen23 Jul 18, 2019

rawagner Jul 17, 2019

afreen23 Jul 18, 2019 •

edited

Loading

afreen23 Jul 18, 2019 •

edited

Loading

afreen23 Jul 19, 2019

lizsurette commented Jul 17, 2019

afreen23 commented Jul 18, 2019 •

edited

Loading

openshift-ci-robot commented Jul 19, 2019

cloudbehl commented Jul 19, 2019

afreen23 commented Jul 19, 2019

gnehapk Jul 19, 2019

afreen23 Jul 19, 2019 •

edited

Loading

afreen23 commented Jul 19, 2019

afreen23 commented Jul 19, 2019

cloudbehl commented Jul 19, 2019

afreen23 commented Jul 19, 2019

rawagner commented Jul 19, 2019

afreen23 commented Jul 19, 2019

cloudbehl commented Jul 19, 2019

openshift-ci-robot commented Jul 19, 2019

afreen23 commented Jul 19, 2019

afreen23 commented Jul 20, 2019

openshift-bot commented Jul 20, 2019

openshift-bot commented Jul 20, 2019

		@@ -0,0 +1,116 @@
		import * as React from 'react';
		import { Dropdown, DropdownToggle, DropdownItem, DropdownPosition } from '@patternfly/react-core';


		/* Chart Data Handlers */

		export const iopsChartData = (data, key) => {

Add Data Consumption Card to Object Service Dashboard #1976

Add Data Consumption Card to Object Service Dashboard #1976

Conversation

afreen23 commented Jul 10, 2019 • edited Loading

openshift-ci-robot commented Jul 10, 2019

afreen23 commented Jul 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rawagner 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

Choose a reason for hiding this comment

afreen23 Jul 18, 2019 • edited Loading

Choose a reason for hiding this comment

afreen23 Jul 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lizsurette commented Jul 17, 2019

afreen23 commented Jul 18, 2019 • edited Loading

openshift-ci-robot commented Jul 19, 2019

cloudbehl commented Jul 19, 2019

afreen23 commented Jul 19, 2019

Choose a reason for hiding this comment

afreen23 Jul 19, 2019 • edited Loading

Choose a reason for hiding this comment

afreen23 commented Jul 19, 2019

afreen23 commented Jul 19, 2019

cloudbehl commented Jul 19, 2019

afreen23 commented Jul 19, 2019

rawagner commented Jul 19, 2019

afreen23 commented Jul 19, 2019

cloudbehl commented Jul 19, 2019

openshift-ci-robot commented Jul 19, 2019

afreen23 commented Jul 19, 2019

afreen23 commented Jul 20, 2019

openshift-bot commented Jul 20, 2019

openshift-bot commented Jul 20, 2019

afreen23 commented Jul 10, 2019 •

edited

Loading

afreen23 Jul 18, 2019 •

edited

Loading

afreen23 Jul 18, 2019 •

edited

Loading

afreen23 commented Jul 18, 2019 •

edited

Loading

afreen23 Jul 19, 2019 •

edited

Loading