Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add metric charts to cluster overview page #971

Merged
merged 1 commit into from
Nov 29, 2022
Merged

Add metric charts to cluster overview page #971

merged 1 commit into from
Nov 29, 2022

Conversation

pcbailey
Copy link
Member

@pcbailey pcbailey commented Nov 17, 2022 •
edited
Loading

📝 Description

This PR replaces the resources inventory card on the cluster overview page with the metric charts card.

Note: The VMs chart in the screenshot below is empty because I was unable to obtain a cluster with the metric present. The metric has been merged to 4.12 and 4.13, but there is no 4.12 build is available at this time that contains that particular commit.

🎥 Screenshot

Selection_198

@openshift-ci openshift-ci bot added the approved This issue is something we want to fix label Nov 17, 2022
@avivtur
Copy link
Member

avivtur commented Nov 17, 2022

/hold
testing this PR locally on the new 4.13 cluster and crashes
overview-charts-crash

avivtur
avivtur reviewed Nov 17, 2022
View reviewed changes
Copy link
Member

@avivtur avivtur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please go over all missing optional chainings to object (obj?.field) to avoid crashes

avivtur
avivtur reviewed Nov 17, 2022
View reviewed changes
src/views/clusteroverview/OverviewTab/metric-charts-card/components/ChartCard.scss Outdated Show resolved Hide resolved
src/views/clusteroverview/OverviewTab/metric-charts-card/components/ChartCard.tsx Show resolved Hide resolved
src/views/clusteroverview/OverviewTab/metric-charts-card/components/MetricChart.scss Show resolved Hide resolved
src/views/clusteroverview/OverviewTab/metric-charts-card/components/MetricChart.tsx Outdated Show resolved Hide resolved
src/views/clusteroverview/OverviewTab/metric-charts-card/components/MetricChart.tsx Outdated Show resolved Hide resolved
src/views/clusteroverview/OverviewTab/metric-charts-card/utils/hooks/types.ts Outdated Show resolved Hide resolved
src/views/clusteroverview/OverviewTab/metric-charts-card/components/MetricChart.tsx Outdated Show resolved Hide resolved
@pcbailey pcbailey force-pushed the create-metrics-card branch 3 times, most recently from 2f6f6d5 to f016633 Compare November 24, 2022 02:54
avivtur
avivtur reviewed Nov 27, 2022
View reviewed changes
Copy link
Member

@avivtur avivtur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one-day-trend

this image present either a new cluster that just created the first VM or first VM created in a project scope, and we have only one point of data, maybe it's best to make sure we have 2 points? also I think the single-day trend is a bit confusing, IMO it will be a better idea to name it "Today's trend" instead of "last 1 days' trend", I'd ask Foday about this

src/views/clusteroverview/OverviewTab/metric-charts-card/components/ChartCard.scss Outdated Show resolved Hide resolved
src/views/clusteroverview/OverviewTab/metric-charts-card/utils/utils.ts Outdated Show resolved Hide resolved
src/views/clusteroverview/OverviewTab/metric-charts-card/utils/hooks/useXAxisTicks.ts Outdated Show resolved Hide resolved
src/views/clusteroverview/OverviewTab/metric-charts-card/utils/hooks/utils.ts Outdated Show resolved Hide resolved
src/views/clusteroverview/OverviewTab/metric-charts-card/utils/hooks/utils.ts Outdated

export const getLargestValue = (data: PrometheusValue[]) => {
return data?.reduce((acc, point) => {
const currValue = Number(point?.[1]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we do something point?.y instead of point?.[1]? the 1 can be not clear in the future for other developer

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are PrometheusValue types, which are arrays with two values, so we can't index them like an object. I created a function to make it easier to understand.

src/views/clusteroverview/OverviewTab/metric-charts-card/utils/utils.ts Outdated Show resolved Hide resolved
src/views/clusteroverview/OverviewTab/metric-charts-card/utils/utils.ts Outdated
import { METRICS } from './constants';

export const formatCurrentValue = (value: number, metric: string) => {
switch (metric) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we get a sum of VMs that is with a decimal point? if not this function is not really needed,
but if we still need it I think this would look a bit cleaner to this function,

export const formatCurrentValue = (value: number, metric: string): number => 
  metric === METRICS.VM ? Math.trunc(value) : value;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wrote this function when I was using a placeholder query for VMs and forgot to remove it. We don't need it, so I got rid of it.

src/views/clusteroverview/OverviewTab/metric-charts-card/components/MetricChart.scss Show resolved Hide resolved
src/views/clusteroverview/OverviewTab/metric-charts-card/components/MetricChart.tsx
},
}}
/>
<ChartGroup>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can remove this ChartGroup and everything will stay the same, can you check that?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed it and it removed the body of the chart.

metalice
metalice reviewed Nov 29, 2022
View reviewed changes
src/utils/i18n.ts Outdated
@@ -40,3 +40,5 @@
// t('plugin__kubevirt-plugin~Node')
// t('plugin__kubevirt-plugin~Create a Virtual Machine from a template')
// t('plugin__kubevirt-plugin~Hardware Devices')
// t('plugin__kubevirt-plugin~VMs')
// t('plugin__kubevirt-plugin~vCPU')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do u need it if we can get 't' everywhere we need now? not only as a hook

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be a good idea to make a bug for removing this file and adding the function t we created wherever needed, but in a different PR

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed!

avivtur
avivtur reviewed Nov 29, 2022
View reviewed changes
Copy link
Member

@avivtur avivtur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks great!
one last comment I added can that can also be in a follow-up PR, let's wait for @metalice to review his comments as well

src/views/clusteroverview/OverviewTab/metric-charts-card/components/ChartCard.scss Outdated Show resolved Hide resolved
@avivtur
Copy link
Member

avivtur commented Nov 29, 2022

lgtm from me, @metalice?

@metalice
Copy link
Member

/lgtm

@metalice
Copy link
Member

/hold cancel

@metalice
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm Passed code review, ready for merge label Nov 29, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 29, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: metalice, pcbailey

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:

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

@openshift-merge-robot openshift-merge-robot merged commit 4320abf into kubevirt-ui:main Nov 29, 2022
@pcbailey
Copy link
Member Author

/cherry-pick release-4.12

@openshift-cherrypick-robot openshift-cherrypick-robot mentioned this pull request Nov 29, 2022
Merged
@openshift-cherrypick-robot
Copy link
Collaborator

@pcbailey: new pull request created: #990

In response to this:

/cherry-pick release-4.12

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@avivtur avivtur avivtur left review comments

@metalice metalice metalice left review comments

@upalatucci upalatucci Awaiting requested review from upalatucci

Assignees

@metalice metalice

Labels
approved This issue is something we want to fix lgtm Passed code review, ready for merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@pcbailey @avivtur @metalice @openshift-cherrypick-robot @openshift-merge-robot