Add the top consumers card to dashboard #1722

pcbailey · 2019-06-14T00:08:08Z

This PR adds the top consumers card to the overview dashboard.

Loaded card:

openshift-ci-robot · 2019-06-14T00:08:22Z

Hi @pcbailey. 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.

Details

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.

spadgett · 2019-06-14T18:30:50Z

Can you add a screenshot?

spadgett

It looks like this PR rewrites a bunch of unit conversions and formatting that we already have elsewhere in console.

spadgett · 2019-06-14T18:32:18Z

frontend/public/components/dashboard/top-consumers-card/strings.js

We should use consistent case in the descriptions (either title or sentence)

spadgett · 2019-06-14T18:33:20Z

frontend/public/components/dashboard/top-consumers-card/utils.tsx

Can we use the existing utilities we have in units.js? If they aren't sufficient, we should add to those.

spadgett · 2019-06-14T18:34:14Z

frontend/public/components/dashboard/top-consumers-card/utils.tsx

console naming conventions are

Suggested change

export const formatCpu = (cpuTime: string, fixed: number = 0): string => Number(cpuTime).toFixed(fixed);

export const formatCPU = (cpuTime: string, fixed: number = 0): string => Number(cpuTime).toFixed(fixed);

spadgett · 2019-06-14T18:34:54Z

frontend/public/components/dashboards-page/overview-dashboard/top-consumers-card.tsx

FYI: we are in the process of migrating Dropdown to PF4 styles

spadgett · 2019-06-14T18:35:20Z

frontend/public/components/dashboards-page/overview-dashboard/top-consumers-card.tsx

Current console convention is to put the _ at the end.

Suggested change

const _TopConsumersCard: React.FC<TopConsumerProps> = ({

const TopConsumersCard_: React.FC<TopConsumerProps> = ({

spadgett · 2019-06-14T18:35:34Z

frontend/public/components/dashboards-page/overview-dashboard/top-consumers-card.tsx

Suggested change

openShiftFlag: state[featureReducerName].get(FLAGS.OPENSHIFT),

openshiftFlag: state[featureReducerName].get(FLAGS.OPENSHIFT),

spadgett · 2019-06-14T18:36:47Z

frontend/public/components/dashboards-page/overview-dashboard/top-consumers-card.tsx

Suggested change

infraCpuResults,

infraCPUResults,

spadgett · 2019-06-14T18:36:54Z

frontend/public/components/dashboards-page/overview-dashboard/top-consumers-card.tsx

Suggested change

workloadCpuResults,

workloadCPUResults,

spadgett · 2019-06-14T18:38:41Z

/cc @TheRealJon

rawagner · 2019-06-14T18:43:14Z

It looks like this PR rewrites a bunch of unit conversions and formatting that we already have elsewhere in console.

right, it needs to sync with #1723 which changes formatting/conversion functions a bit to fit existing use-cases and also new ones (#1723 (comment))

spadgett · 2019-06-14T19:14:10Z

/assign

pcbailey · 2019-06-16T17:37:09Z

Hey, @spadgett. I'm sorry. I should have been clearer that this wasn't ready for your review, yet. I was just posting a very early version so @rawagner could look at it and make sure I wasn't way off track with the changes I've made thus far. That being said, I appreciate you taking the time to review it and I'll make sure to address your comments in my next update and post a screenshot when I have one.

rawagner · 2019-07-11T08:10:11Z

PR updated, here is how it currently looks like

rawagner · 2019-07-11T13:06:53Z

/ok-to-test

jelkosz · 2019-07-15T15:26:57Z

/test e2e-aws

vojtechszocs · 2019-07-16T17:50:37Z

frontend/public/components/dashboard/grid.tsx

Can you explain why is this needed?

Thats actually a little bug if width ref hook which caused issues with chart size. I removed this change a will create a separate PR

proper fix is here #2058

vojtechszocs · 2019-07-16T17:53:20Z

frontend/public/components/dashboard/top-consumers-card/consumers-filter.tsx

Nit: extra newline.

vojtechszocs · 2019-07-16T17:56:38Z

frontend/public/components/dashboards-page/overview-dashboard/queries.ts

Nit: two extra newlines.

vojtechszocs · 2019-07-16T18:00:07Z

frontend/public/components/dashboards-page/overview-dashboard/top-consumers-card.tsx

Nit: no need for a separate pluginItems variable

plugins.registry.getDashboardsOverviewTopConsumerItems().forEach(pluginItem => { // ... });

vojtechszocs · 2019-07-16T18:01:08Z

frontend/public/components/dashboards-page/overview-dashboard/top-consumers-card.tsx

This can be simplified

if (!topConsumers[pluginItem.properties.name]) { topConsumers[pluginItem.properties.name] = { metric: pluginItem.properties.metric, queries: pluginItem.properties.queries, mutator: pluginItem.properties.mutator, }; }

vojtechszocs · 2019-07-16T18:05:37Z

frontend/public/components/dashboards-page/overview-dashboard/top-consumers-card.tsx

Please extract repeated expressions as variables

topConsumersMap[type] metricTypeMap[sortOption]

extracted those expressions to variables and reused

vojtechszocs · 2019-07-16T18:07:53Z

frontend/public/components/dashboards-page/overview-dashboard/top-consumers-card.tsx

Why [key in MetricType]? with the trailing ?

If I dont add ? then TS is expecting all values of MetricType in queries prop and thats not what I always want ie here https://github.com/openshift/console/pull/1722/files#diff-163991d4c119db922a729100ce2bea7fR27 i dont have MetricType.NETWORK

OK, makes sense.

Actually, we could re-use DashboardsOverviewTopConsumerItem.queries type here, but we can improve it later.

vojtechszocs · 2019-07-17T15:22:44Z

Thanks @rawagner for addressing the review comments.

FYI @TheRealJon Some changes were made to graph components.

/lgtm
/approve

atiratree · 2019-07-18T11:01:31Z

/lgtm

openshift-ci-robot · 2019-07-18T11:01:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pcbailey, suomiy, vojtechszocs

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~frontend/OWNERS~~ [vojtechszocs]

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 added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 14, 2019

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 14, 2019

openshift-ci-robot requested review from alecmerdler and kyoto June 14, 2019 00:08

rawagner mentioned this pull request Jun 14, 2019

Add capacity card #1723

Merged

spadgett requested changes Jun 14, 2019

View reviewed changes

openshift-ci-robot requested a review from TheRealJon June 14, 2019 18:38

spadgett added this to the v4.2 milestone Jun 14, 2019

openshift-ci-robot assigned spadgett Jun 14, 2019

pcbailey force-pushed the add-top-consumers-dashboard-card branch 2 times, most recently from c21ea08 to 883b3b3 Compare June 19, 2019 14:54

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

pcbailey force-pushed the add-top-consumers-dashboard-card branch from 883b3b3 to 7e94f42 Compare July 9, 2019 20:59

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

rawagner force-pushed the add-top-consumers-dashboard-card branch 2 times, most recently from d222472 to 37ced5e Compare July 11, 2019 08:08

rawagner force-pushed the add-top-consumers-dashboard-card branch 2 times, most recently from 2ef31a3 to 7616556 Compare July 11, 2019 08:32

pcbailey changed the title ~~WIP: Add the top consumers card to dashboard~~ Add the top consumers card to dashboard Jul 11, 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 11, 2019

openshift-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Jul 11, 2019

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

rawagner force-pushed the add-top-consumers-dashboard-card branch 2 times, most recently from 44ee6d9 to 0c5cd82 Compare July 13, 2019 10:38

rawagner force-pushed the add-top-consumers-dashboard-card branch from 0c5cd82 to a452d47 Compare July 16, 2019 13:47

vojtechszocs suggested changes Jul 16, 2019

View reviewed changes

rawagner force-pushed the add-top-consumers-dashboard-card branch 2 times, most recently from 17592a8 to 8713d5c Compare July 16, 2019 20:41

openshift-ci-robot assigned vojtechszocs Jul 17, 2019

openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 17, 2019

rawagner force-pushed the add-top-consumers-dashboard-card branch from 8713d5c to ceb7d03 Compare July 17, 2019 16:46

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

Add Top Consumers dashboard card

f9a7c14

rawagner force-pushed the add-top-consumers-dashboard-card branch from ceb7d03 to f9a7c14 Compare July 18, 2019 07:38

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

openshift-ci-robot assigned atiratree Jul 18, 2019

openshift-merge-robot merged commit 3bcde7c into openshift:master Jul 18, 2019

pcbailey deleted the add-top-consumers-dashboard-card branch October 26, 2022 12:30

	export const formatCpu = (cpuTime: string, fixed: number = 0): string => Number(cpuTime).toFixed(fixed);
	export const formatCPU = (cpuTime: string, fixed: number = 0): string => Number(cpuTime).toFixed(fixed);

	const _TopConsumersCard: React.FC<TopConsumerProps> = ({
	const TopConsumersCard_: React.FC<TopConsumerProps> = ({

	openShiftFlag: state[featureReducerName].get(FLAGS.OPENSHIFT),
	openshiftFlag: state[featureReducerName].get(FLAGS.OPENSHIFT),

Add the top consumers card to dashboard #1722

Add the top consumers card to dashboard #1722

Uh oh!

Conversation

pcbailey commented Jun 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Jun 14, 2019

Uh oh!

spadgett commented Jun 14, 2019

Uh oh!

spadgett left a comment

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!

spadgett commented Jun 14, 2019

Uh oh!

rawagner commented Jun 14, 2019

Uh oh!

spadgett commented Jun 14, 2019

Uh oh!

pcbailey commented Jun 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rawagner commented Jul 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rawagner commented Jul 11, 2019

Uh oh!

jelkosz commented Jul 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!

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!

pcbailey commented Jun 14, 2019 •

edited

Loading

pcbailey commented Jun 16, 2019 •

edited

Loading

rawagner commented Jul 11, 2019 •

edited

Loading