CNV-16653: Statistics of secondary network interfaces #1033

DanaOrr · 2023-01-31T15:30:09Z

Signed-off-by: Dana Orr dorr@redhat.com

📝 Description

Adding the option to see in a popover on the overview page the network interfaces that the machine has.
And I added on the metrics page the possibility to see the network in, out and bandwidth graphs for each NIC and for all of them together.

🎥 Demo

Before:
On the overview page:

On the metrics page:

After:

openshift-ci-robot · 2023-01-31T15:30:13Z

@DanaOrr: This pull request references CNV-16653 which is a valid jira issue.

In response to this:

Signed-off-by: Dana Orr dorr@redhat.com

📝 Description

Adding the option to see in a popover on the overview page the network interfaces that the machine has.
And I added on the metrics page the possibility to see the network in, out and bandwidth graphs for each NIC and for all of them together.

🎥 Demo

Before:
On the overview page:

On the metrics page:

After:

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.

hstastna

Left some comments and questions, PTAL. Otherwise great work, Dana! :)

hstastna · 2023-01-31T17:32:31Z

...view/components/VirtualMachinesOverviewTabUtilization/components/NetworkUtil/NetworkUtil.tsx

  const totalTransferred = xbytes(networkInData + networkOutData || 0, {
    iec: true,
    fixed: 0,
  });
-
+  const networkInterfacesLength = interfacesNames?.length;


I think you can use interfacesNames?.length directly in the line 92. No need to create a new variable for this, as this is very short and not used in any other places within this file :)

hstastna · 2023-01-31T17:45:40Z

src/utils/components/Charts/NetworkUtil/NetworkThresholdChartSingleSource.tsx

+  const tickValues = Array.from({ length: Ymax + 1 }, (_, index) => {
+    if (index === 0) return '1 Bps';
+    if (index === Math.round(Ymax)) return `${Math.round(Ymax + 1)} Bps`;
+    return `${index.toString()} Bps`;


Hmm, do we really need to use toString for the index in here, if we are using a template literal (with backticks `) ?

you right👍

hstastna · 2023-01-31T17:46:35Z

src/utils/components/Charts/NetworkUtil/NetworkThresholdChartSingleSource.tsx

+  const legendData =
+    !isEmpty(chartData) &&
+    chartData?.map((newChartdata, index) => {
+      return { childName: newChartdata?.[index]?.name, name: newChartdata?.[index]?.name };


Here the values for childName and name seem to be identical. is this intentional?

The childName is used to synchronize the data series associated with the given chart name so it is better to have the same name I think...

hstastna · 2023-01-31T17:52:04Z

src/views/virtualmachines/details/tabs/metrics/NetworkCharts/NetworkCharts.tsx

-    [vmi],
+  const { t } = useKubevirtTranslation();
+  const interfacesNames = useMemo(() => {
+    const interfaces = vmi?.spec?.domain?.devices?.interfaces.map((nic) => nic?.name);


Maybe better ...vmi?.spec?.domain?.devices?.interfaces?.map((nic) => ... to make sure some interfaces are present :)

Suggested change

const interfaces = vmi?.spec?.domain?.devices?.interfaces.map((nic) => nic?.name);

const interfaces = vmi?.spec?.domain?.devices?.interfaces?.map((nic) => nic?.name);

hstastna · 2023-01-31T17:57:24Z

...view/components/VirtualMachinesOverviewTabUtilization/components/NetworkUtil/NetworkUtil.tsx

@@ -1,12 +1,17 @@
-import React from 'react';
+/* eslint-disable react-hooks/rules-of-hooks */


Do we really have to use this here? Feels like we are going to disrespect some rules... IMO we should avoid doing this as much as possible.

hstastna · 2023-01-31T18:01:19Z

...view/components/VirtualMachinesOverviewTabUtilization/components/NetworkUtil/NetworkUtil.tsx

+                    to={`${getResourceUrl({
+                      model: VirtualMachineModel,
+                      resource: vmi,
+                    })}/metrics?network=All networks`}


This looks kinda weird, I mean the space in the link, between All and networks. Are you sure this is working as expected?

Yes, it works as expected, but I deleted "all networks" and it will move to the same place I wanted,
so it's better to delete, thank you :)

metalice · 2023-02-05T11:26:05Z

src/utils/components/Charts/NetworkUtil/NetworkThresholdChartSingleSource.tsx

+    });
+  const isReady = !isEmpty(chartData);
+
+  const findChartMaxYAxis = (chartData1: { x: Date; y: number; name: string }[][]) => {


can be exported to util file?

metalice · 2023-02-05T11:26:36Z

src/utils/components/Charts/NetworkUtil/NetworkThresholdChartSingleSource.tsx

+    return maxY;
+  };
+  const Ymax = findChartMaxYAxis(chartData);
+  const tickValues = Array.from({ length: Ymax + 1 }, (_, index) => {


can be exported to util file?

@DanaOrr maybe it worth to move it to src/utils/components/Charts/utils/utils.ts ?

metalice · 2023-02-05T11:27:02Z

src/utils/components/Charts/NetworkUtil/NetworkThresholdChartSingleSource.tsx

  });

-  const isReady = !isEmpty(chartData);
+  const newTickFormat = (tick: any, index: number, ticks: any[]) => {


can be exported to util file?
please consider changing the name to something more informative

metalice · 2023-02-05T11:27:35Z

src/utils/components/Charts/NetworkUtil/NetworkThresholdChartSingleSource.tsx

-              />
+              {isReady &&
+                chartData?.map((newChartdata) => (
+                  // eslint-disable-next-line react/jsx-key


metalice · 2023-02-05T11:28:18Z

src/utils/components/Charts/utils/utils.ts

+  return [
+    {
+      ...response?.data?.result?.[0],
+      metric: { ...response?.data?.result?.[0]?.metric, interface: 'all-networks' },


consider transforming all-network string to a const

metalice · 2023-02-05T11:29:49Z

src/views/virtualmachines/details/tabs/metrics/NetworkCharts/hook/useNetworkData.tsx

@@ -28,40 +34,80 @@ const useNetworkData: UseNetworkData = (vmi, nic) => {
    [vmi, duration, nic],
  );

+  const isAllNetwork = nic === 'All networks';


consider using a const for all networks

metalice · 2023-02-05T11:32:29Z

...view/components/VirtualMachinesOverviewTabUtilization/components/NetworkUtil/NetworkUtil.tsx

+                        resource: vmi,
+                      })}/metrics?network=${networkInterface.name}`}
+                    >
+                      {networkInterface.name}


please add ?.

all over the file where needed

DanaOrr · 2023-02-08T12:59:40Z

/retest

upalatucci · 2023-02-09T12:39:02Z

/retest

upalatucci · 2023-02-09T12:39:39Z

/lgtm

hstastna

Some more little changes and we are good to go! ;)

hstastna · 2023-02-09T13:22:38Z

src/utils/components/Charts/utils/utils.ts

+  return singleNic ? [singleNic] : response?.data?.result;
+};
+
+export const allNetworks = 'All networks';


Please, use THIS_FORMAT for constants like that, and place constants into constants.ts file, under utils dir (if there isn't such a file, feel free to create it). It is better to have constants in a separate file :)
Additionally, don't be afraid to use the constant where needed. For example, I can see in NetworkCharts.tsx in this PR 'All networks' string instead of a constant.

hstastna · 2023-02-09T13:24:06Z

src/views/virtualmachines/details/tabs/metrics/NetworkCharts/NetworkCharts.tsx

+
+  const interfacesNames = useMemo(() => {
+    const interfaces = vmi?.spec?.domain?.devices?.interfaces?.map((nic) => nic?.name);
+    interfaces?.unshift('All networks');


You could use the constant in here, for 'All networks' ;)

hstastna · 2023-02-09T13:24:19Z

src/views/virtualmachines/details/tabs/metrics/NetworkCharts/NetworkCharts.tsx

+  const query = useQuery();
+  const [isDropdownOpen, setIsDropdownOpen] = React.useState<boolean>(false);
+  const [selectedNetwork, setSelectedNetwork] = React.useState<string>(
+    query?.get('network') || 'All networks',


You could use the constant in here, for 'All networks', too ;)

hstastna · 2023-02-09T13:26:57Z

src/utils/components/Charts/utils/utils.ts

+  PrometheusResponse,
+  PrometheusResult,
+  PrometheusValue,
+} from '@openshift-console/dynamic-plugin-sdk';

 export const SINGLE_VM_DURATION = 'SINGLE_VM_DURATION';
 export const TICKS_COUNT = 100;


I'd move those constants into a separate constants.ts file ;) WDYT?

It's a good idea but it requires me to change the import to many more files and it creates a load on this PR..
so maybe in another PR? :)

metalice · 2023-02-13T09:39:10Z

/approve

hstastna

Some more little things, otherwise great work! :)

hstastna · 2023-02-13T11:36:34Z

src/utils/components/Charts/NetworkUtil/NetworkThresholdChartSingleSource.tsx

+    return maxY;
+  };
+  const Ymax = findChartMaxYAxis(chartData);
+  const tickValues = Array.from({ length: Ymax + 1 }, (_, index) => {


@DanaOrr maybe it worth to move it to src/utils/components/Charts/utils/utils.ts ?

hstastna · 2023-02-13T11:39:38Z

src/views/virtualmachines/details/tabs/metrics/virtual-machine-metrics-tab.scss

+.network-poppver {
+display: flex;
+justify-content: space-between;
+}


.network-poppver { display: flex; justify-content: space-between; }

plus maybe it is nicer also to use "popover" instead of "poppver" to have this class more self-explanatory :)

Thanks for making this nicer, Dana! :) 👍🏽

DanaOrr · 2023-02-14T07:45:10Z

/retest

hstastna

I've added another little comment for even better code ;) ...

hstastna · 2023-02-14T14:26:56Z

...view/components/VirtualMachinesOverviewTabUtilization/components/NetworkUtil/NetworkUtil.tsx

+            position="bottom"
+          >
+            <div>
+              <Button variant="link" isInline>


It's better to use variant={ButtonVariant.link} :)
(Sorry, I didn't see that earlier)

Thank you Hilda ! :)

hstastna · 2023-02-14T14:27:52Z

src/views/virtualmachines/details/tabs/metrics/virtual-machine-metrics-tab.scss

+.network-poppver {
+display: flex;
+justify-content: space-between;
+}


Thanks for making this nicer, Dana! :) 👍🏽

Signed-off-by: Dana Orr <dorr@redhat.com>

openshift-ci · 2023-02-15T11:33:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: DanaOrr, hstastna, metalice, upalatucci

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:

~~OWNERS~~ [metalice]

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

openshift-ci bot requested review from hstastna and upalatucci January 31, 2023 15:30

openshift-merge-robot added the needs-rebase label Jan 31, 2023

DanaOrr force-pushed the secondary-network-interfaces branch 7 times, most recently from 3fc5233 to 2914a27 Compare January 31, 2023 16:10

openshift-merge-robot removed the needs-rebase label Jan 31, 2023

hstastna reviewed Jan 31, 2023

View reviewed changes

DanaOrr force-pushed the secondary-network-interfaces branch 2 times, most recently from a19b372 to b77f4b3 Compare February 1, 2023 13:31

metalice reviewed Feb 5, 2023

View reviewed changes

openshift-merge-robot added the needs-rebase label Feb 6, 2023

DanaOrr force-pushed the secondary-network-interfaces branch 3 times, most recently from 3df6d74 to 8121b3a Compare February 8, 2023 11:01

openshift-merge-robot removed the needs-rebase label Feb 8, 2023

DanaOrr force-pushed the secondary-network-interfaces branch from 8121b3a to c4481ce Compare February 8, 2023 11:12

DanaOrr force-pushed the secondary-network-interfaces branch from c4481ce to 2e60c6b Compare February 9, 2023 11:56

openshift-ci bot assigned upalatucci Feb 9, 2023

openshift-ci bot added the lgtm Passed code review, ready for merge label Feb 9, 2023

hstastna suggested changes Feb 9, 2023

View reviewed changes

openshift-ci bot assigned hstastna Feb 9, 2023

openshift-ci bot removed the lgtm Passed code review, ready for merge label Feb 9, 2023

DanaOrr force-pushed the secondary-network-interfaces branch from 2e60c6b to 9629f7c Compare February 12, 2023 10:15

openshift-ci bot added the approved This issue is something we want to fix label Feb 13, 2023

hstastna reviewed Feb 13, 2023

View reviewed changes

DanaOrr force-pushed the secondary-network-interfaces branch 4 times, most recently from 5c99632 to cb5e5f6 Compare February 13, 2023 14:15

hstastna reviewed Feb 14, 2023

View reviewed changes

Statistics of secondary network interfaces

bb33bb4

Signed-off-by: Dana Orr <dorr@redhat.com>

DanaOrr force-pushed the secondary-network-interfaces branch from cb5e5f6 to bb33bb4 Compare February 15, 2023 06:46

hstastna approved these changes Feb 15, 2023

View reviewed changes

openshift-ci bot added the lgtm Passed code review, ready for merge label Feb 15, 2023

openshift-merge-robot merged commit e903e43 into kubevirt-ui:main Feb 15, 2023

	const interfaces = vmi?.spec?.domain?.devices?.interfaces.map((nic) => nic?.name);
	const interfaces = vmi?.spec?.domain?.devices?.interfaces?.map((nic) => nic?.name);

		@@ -1,12 +1,17 @@
		import React from 'react';
		/* eslint-disable react-hooks/rules-of-hooks */

CNV-16653: Statistics of secondary network interfaces #1033

CNV-16653: Statistics of secondary network interfaces #1033

Conversation

DanaOrr commented Jan 31, 2023

📝 Description

🎥 Demo

openshift-ci-robot commented Jan 31, 2023

📝 Description

🎥 Demo

hstastna 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

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

DanaOrr commented Feb 8, 2023

upalatucci commented Feb 9, 2023

upalatucci commented Feb 9, 2023

hstastna left a comment

Choose a reason for hiding this comment

hstastna Feb 9, 2023 • edited Loading

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

metalice commented Feb 13, 2023

hstastna 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

DanaOrr commented Feb 14, 2023

hstastna 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

openshift-ci bot commented Feb 15, 2023

hstastna Feb 9, 2023 •

edited

Loading