NETOBSERV-1959: Remove hardcoded columns rendering + other enhancements #633

jotak · 2024-11-07T16:00:25Z

Remove hardcoded columns
Change some of the rendering scripted functions:
- getConcatenatedValue -> concat now dedicated to just concatenate values
- they can now accept litterals (e.g. concat(DstAddr,':',DstPort))
- remove getSrcOrDstValue since there was also "[...]" for the same role
- add new kubeObject func to show kind-name-namespace sort of things
Improve performance by avoiding re-parsing everything for every rendered flow
- there's probably more that we could do on that side
Add unit tests

Description

Dependencies

operator: netobserv/network-observability-operator#835

Checklist

If you are not familiar with our processes or don't know what to answer in the list below, let us know in a comment: the maintainers will take care of that.

openshift-ci-robot · 2024-11-07T16:00:30Z

openshift-ci · 2024-11-07T16:00:34Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from jotak. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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 · 2024-11-07T16:03:35Z

openshift-ci-robot · 2024-11-07T16:03:41Z

openshift-ci-robot · 2024-11-07T16:03:44Z

jpinsonneau · 2024-11-08T08:57:16Z

web/src/utils/__tests__/columns.spec.ts

+const getConfig = () => {
+  const file = fs.readFileSync('../config/sample-config.yaml', 'utf8');
+  const config = parse(file);
+  const columnsConfig: ColumnConfigDef[] = config.frontend.columns;
+  const fields: FieldConfig[] = config.frontend.fields;
+  return { columnsConfig, fields };
+};


We already have some config sample with columns / fields in https://github.com/netobserv/network-observability-console-plugin/blob/main/web/src/components/__tests-data__/config.ts

I guess it would be better to reuse these

jpinsonneau · 2024-11-08T08:59:19Z

web/src/components/drawer/record/record-field.tsx

          <ResourceIcon kind={kind} />
          <Text component={TextVariants.p} className="co-resource-item__resource-name" data-test-id={value}>
-            {value}
+            &nbsp;{value}


Add a padding or marging using a new classname instead

jpinsonneau · 2024-11-08T09:03:33Z

web/src/components/drawer/record/record-field.tsx

        <div data-test={`field-resource-${kind}.${ns}.${value}`} className="force-truncate">
          {resourceIconText(value, kind, ns)}
          <TextContent className="record-field-tooltip netobserv-no-child-margin">
+            {/* Note: THIS IS THE TOOLTIP */}


If it's not clear here, you can create a kubeObjTooltip function returning the whole TextContent element. I would avoid having brackets for this comment

jpinsonneau · 2024-11-08T09:14:36Z

web/src/components/drawer/record/record-panel.tsx

-      return !forbiddenColumns.includes(c.id) && value !== null && value !== '' && !Number.isNaN(value);
+      return (
+        !forbiddenColumns.includes(c.id) &&
+        value !== undefined &&


I think it was on purepose to keep undefined values here.

The goal was having null to skip a column display when misconfigured and undefined when the data is missing from the record which will result in empty text display

ok I missed this subtelty and it's probably why I have some tests failing.
I'd like somehow to make this more obvious ... but not sure how

jpinsonneau · 2024-11-08T09:30:12Z

web/src/utils/column-parser.ts

+    const obj: KubeObj = {
+      kind: String(getColumnOrRecordValue(columns, record, args[0], '')),
+      namespace: String(getColumnOrRecordValue(columns, record, args[1], '')),
+      name: String(getColumnOrRecordValue(columns, record, args[2], '')),
+      showNamespace: args[3] === '1'
+    };
+    if (!obj.name || !obj.kind) {
+      return undefined;
+    }
+    // Remove empty namespace
+    if (obj.namespace === '') {
+      delete obj.namespace;
+    }
+    return obj;


It would feel easier to read building the KubeObj at return statement:

Suggested change

const obj: KubeObj = {

kind: String(getColumnOrRecordValue(columns, record, args[0], '')),

namespace: String(getColumnOrRecordValue(columns, record, args[1], '')),

name: String(getColumnOrRecordValue(columns, record, args[2], '')),

showNamespace: args[3] === '1'

};

if (!obj.name || !obj.kind) {

return undefined;

}

// Remove empty namespace

if (obj.namespace === '') {

delete obj.namespace;

}

return obj;

const kind = String(getColumnOrRecordValue(columns, record, args[0], ''));

const namespace = String(getColumnOrRecordValue(columns, record, args[1], ''));

const name = String(getColumnOrRecordValue(columns, record, args[2], ''));

const showNamespace = args[3] === '1'

};

if (!name || !kind) {

return undefined;

} else if (namespace === '') {

return { kind, name };

}

return { kind, namespace, name, showNamespace };

jpinsonneau · 2024-11-08T09:34:55Z

web/src/utils/columns.ts

+export interface KubeObj {
+  name: string;
+  kind: string;
+  namespace?: string;
+  showNamespace: boolean;
+}


following kubeObject parser suggestion:

Suggested change

export interface KubeObj {

name: string;

kind: string;

namespace?: string;

showNamespace: boolean;

}

export interface KubeObj {

name: string;

kind: string;

namespace?: string;

showNamespace?: boolean;

}

I did (almost) what you suggested but kept showNamespace strict boolean

codecov · 2024-11-08T16:24:08Z

Codecov Report

Attention: Patch coverage is 62.23776% with 54 lines in your changes missing coverage. Please review.

Project coverage is 56.54%. Comparing base (c12294d) to head (ca42865).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
web/src/utils/column-parser.ts	66.27%	27 Missing and 2 partials ⚠️
web/src/components/drawer/record/record-field.tsx	56.66%	11 Missing and 2 partials ⚠️
web/src/utils/columns.ts	35.29%	9 Missing and 2 partials ⚠️
web/src/components/netflow-traffic.tsx	75.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #633      +/-   ##
==========================================
+ Coverage   55.75%   56.54%   +0.78%     
==========================================
  Files         195      196       +1     
  Lines       10065    10063       -2     
  Branches     1200     1195       -5     
==========================================
+ Hits         5612     5690      +78     
+ Misses       4084     4008      -76     
+ Partials      369      365       -4

Flag	Coverage Δ
uitests	`59.25% <62.23%> (+1.14%)`	⬆️
unittests	`50.58% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
web/src/components/__tests-data__/columns.ts	`100.00% <ø> (ø)`
web/src/components/__tests-data__/flows.ts	`100.00% <ø> (ø)`
web/src/components/drawer/record/record-panel.tsx	`52.23% <100.00%> (+2.23%)`	⬆️
...omponents/tabs/netflow-table/netflow-table-row.tsx	`91.66% <ø> (ø)`
web/src/components/netflow-traffic.tsx	`66.76% <75.00%> (-0.10%)`	⬇️
web/src/utils/columns.ts	`78.10% <35.29%> (+23.00%)`	⬆️
web/src/components/drawer/record/record-field.tsx	`52.04% <56.66%> (+2.87%)`	⬆️
web/src/utils/column-parser.ts	`66.27% <66.27%> (ø)`

... and 1 file with indirect coverage changes

---- 🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests
JS Bundle Analysis - Avoid shipping oversized bundles

openshift-ci-robot · 2024-11-12T10:15:04Z

- Remove hardcoded columns - Change some of the rendering scripted functions: - getConcatenatedValue -> concat now dedicated to just concatenate values - they can now accept litterals (e.g. concat(DstAddr,':',DstPort)) - remove getSrcOrDstValue since there was also "[...]" for the same role - add new kubeObject func to show kind-name-namespace sort of things - Improve performance by avoiding re-parsing everything for every rendered flow - there's probably more that we could do on that side - Add unit tests

Previously, visible fields were determined out of several criteria including: - fields with calculated values were filtered out - record with null value was filtered out (as opposed to undefined) Also, "Common" columns was determined by having a calculated value (ie. non-common columns were prohibited to use calculated) This must change since columns can now have calculated and still must be visible (e.g. SrcK8S_Name is a kubeObject calculated value) - So, Common now is when calculated in the form of "[a,b]" - Visible columns for side panel is now based on being tied to fields

Those fields should be correctly displayed when ICMP data is present, and not displayed (in side panel) when ICMP data is absent.

openshift-ci · 2024-11-18T10:05:08Z

New changes are detected. LGTM label has been removed.

jotak requested a review from jpinsonneau November 7, 2024 16:00

openshift-ci-robot added the jira/valid-reference label Nov 7, 2024

jotak added no-qe This PR doesn't necessitate QE approval no-doc This PR doesn't require documentation change on the NetObserv operator labels Nov 7, 2024

jotak force-pushed the refac-columns branch 3 times, most recently from 6b7b546 to 23bba4c Compare November 7, 2024 17:01

jpinsonneau reviewed Nov 8, 2024

View reviewed changes

jotak mentioned this pull request Nov 8, 2024

Adapt columns in frontend config with changes in plugin netobserv/network-observability-operator#835

Merged

jotak requested a review from jpinsonneau November 8, 2024 15:54

jpinsonneau approved these changes Nov 13, 2024

View reviewed changes

openshift-ci bot assigned jpinsonneau Nov 13, 2024

openshift-ci bot added the lgtm label Nov 13, 2024

jotak added 7 commits November 18, 2024 11:01

preprocess a bit more of parsed functions

3b4f9ce

address feedback

f712d05

Use fieldValue func for filters

ce61994

NETOBSERV-1969: Fix IcmpType & IcmpCode

ac300c2

Those fields should be correctly displayed when ICMP data is present, and not displayed (in side panel) when ICMP data is absent.

format

ca42865

jotak force-pushed the refac-columns branch from 0119f46 to ca42865 Compare November 18, 2024 10:05

openshift-ci bot removed the lgtm label Nov 18, 2024

jotak merged commit 8fb9a44 into netobserv:main Nov 19, 2024
7 of 9 checks passed

jpinsonneau mentioned this pull request Dec 9, 2024

NETOBSERV-1814 add cypress checks on fields #604

Merged

10 tasks

NETOBSERV-1959: Remove hardcoded columns rendering + other enhancements #633

NETOBSERV-1959: Remove hardcoded columns rendering + other enhancements #633

Uh oh!

Conversation

jotak commented Nov 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Dependencies

Checklist

Uh oh!

openshift-ci-robot commented Nov 7, 2024 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Dependencies

Checklist

Uh oh!

openshift-ci bot commented Nov 7, 2024

Uh oh!

openshift-ci-robot commented Nov 7, 2024 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Dependencies

Checklist

Uh oh!

openshift-ci-robot commented Nov 7, 2024 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Dependencies

Checklist

Uh oh!

openshift-ci-robot commented Nov 7, 2024 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Dependencies

Checklist

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!

codecov bot commented Nov 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

openshift-ci-robot commented Nov 12, 2024 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Dependencies

Checklist

Uh oh!

openshift-ci bot commented Nov 18, 2024

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jotak commented Nov 7, 2024 •

edited

Loading

openshift-ci-robot commented Nov 7, 2024 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Nov 7, 2024 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Nov 7, 2024 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Nov 7, 2024 •

edited by openshift-ci bot

Loading

codecov bot commented Nov 8, 2024 •

edited

Loading

openshift-ci-robot commented Nov 12, 2024 •

edited by openshift-ci bot

Loading