NETOBSERV-2182 & NETOBSERV-2183 PoC : Filters refactoring #877

jpinsonneau · 2025-06-03T14:37:14Z

Description

Merge Match option and Back and forth ones into a single new dropdown appearing next to filters values as:

any => match at least one filter; source or destination is not mandatory in that case so you can simply specify a namespace / name for example
one way => strictly match filters mentioned, also allow specifying a filter without it's direction which will result in two queries (one for source, one for destination)
peers => specify up to two peers (behind the scene, Source and Destination) and run the opposite query to get return traffic

Add dropdowns under filters values allowing:

enable / disable
remove a src / dst if set
swap
remove

Removed the Source and Destination accordion from the filters selection. The source is forced only when using one way or peers, else it's both by default.

Added contains vs equals comparator. Contains is the exact same behavior of previous implementation and equals works as same as when using quotes.

Dependencies

Requires netobserv/network-observability-operator#1632 to get updated filters config

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 · 2025-06-03T14:37:19Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

openshift-ci · 2025-06-03T14:37:23Z

[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 assign kalmanmeth for approval. For more information see the 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

codecov · 2025-06-03T14:39:53Z

Codecov Report

Attention: Patch coverage is 76.08696% with 11 lines in your changes missing coverage. Please review.

Project coverage is 49.19%. Comparing base (832cdf7) to head (9427285).
Report is 6 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/model/filters/logql.go	68.42%	2 Missing and 4 partials ⚠️
pkg/model/filters/filters.go	78.26%	3 Missing and 2 partials ⚠️

❗ There is a different number of reports uploaded between BASE (832cdf7) and HEAD (9427285). Click for more details.

HEAD has 3 uploads less than BASE

Flag BASE (832cdf7) HEAD (9427285)

unittests 2 1

uitests 2 0

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #877      +/-   ##
==========================================
- Coverage   55.19%   49.19%   -6.01%     
==========================================
  Files         199       39     -160     
  Lines       10620     3336    -7284     
  Branches     1231        0    -1231     
==========================================
- Hits         5862     1641    -4221     
+ Misses       4391     1525    -2866     
+ Partials      367      170     -197

Flag	Coverage Δ
uitests	`?`
unittests	`49.19% <76.08%> (-0.06%)`	⬇️

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

Files with missing lines	Coverage Δ
pkg/loki/flow_query.go	`67.70% <100.00%> (+0.33%)`	⬆️
pkg/model/filters/filters.go	`83.33% <78.26%> (+4.08%)`	⬆️
pkg/model/filters/logql.go	`61.37% <68.42%> (-3.15%)`	⬇️

... and 160 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

web/src/utils/__tests__/back-and-forth.spec.ts

stleerh · 2025-06-13T19:33:35Z

It seems hard to understand because of all the different states.

a) What does it mean to have:

                Source                           Destination
Any:   From Namespace: "netobserv"   To Namespace: openshift-console

compared to:

                Peer A                           Peer B
Peers:    Namespace: "netobserv"   Namespace: openshift-console

b) The dropdown is used to combine/separate peers. This is not intuitive.

I add a Namespace "netobserv" as a filter. It is "One way". It appears as the Source Namespace.

a) How do I add Destination Namespace?

b) If I click "Swap", it changes to:

          Destination
To Namespace: "netobserv"

If I click "Swap" again, it changes to:

From Namespace: "netobserv"

Previously, there was no "From". It was just:

Namespace: "netobserv"

There's a lot of space wasted by having a label (e.g. "Source" or "Peer A") above the filter selection
The topology should not overlap the scope selection (e.g. Node).
When I click "Swap", the zoom level in topology resets.
In the filter field dropdown, since the list is quite long, I suggest to alphabetize it even though "Namespace" might be the most popular selection.
In the filter field dropdown, it doesn't have a scrollbar. The dropdown field scrolls with the window scrollbar which is kind of odd, and this only works with the scroller on my mouse. If I click the scrollbar, the dropdown disappears.

Thoughts:
I would still go back to "Source Namespace" and "Destination Namespace" fields. Then "Namespace" or explicitly "Source/Dest Namespace" means source or destination.

The "Swap" button would change the "Source" fields to "Destination" and vice versa.

jpinsonneau · 2025-06-16T08:35:23Z

Thanks for your feedback @stleerh. Let me try to adress all of these:

It seems hard to understand because of all the different states.

a) What does it mean to have:

                Source                           Destination
Any:   From Namespace: "netobserv"   To Namespace: openshift-console

compared to:

                Peer A                           Peer B
Peers:    Namespace: "netobserv"   Namespace: openshift-console

Any means you can have either source namespace = netobserv or destination namespace = openshift-console or both matching. I realise the From / To mentions should not appear in that case btw.

Peers means you will only get flows matching (source namespace = netobserv and destination namespace = openshift-console) OR (destination namespace = netobserv and source namespace = openshift-console)

If you feel it's not clear enough I can try to improve the display here but showing the full query will definitly take too much space in the screen.

b) The dropdown is used to combine/separate peers. This is not intuitive.

Do you have a better way to do so ? I tried to reduce the number of filters in the selection by separating Source / Destination from the dropdown.

I add a Namespace "netobserv" as a filter. It is "One way". It appears as the Source Namespace.

a) How do I add Destination Namespace?

For now, you need to click on the arrow next to your value and then click "as destination"

It's an extra step but I didn't found a better way yet. We may find a way to select Source / Destination before validating the filter but I don't want to keep the accordion in the filter selection.

b) If I click "Swap", it changes to:
          Destination
To Namespace: "netobserv"
If I click "Swap" again, it changes to:
From Namespace: "netobserv"
Previously, there was no "From". It was just:
Namespace: "netobserv"

The from / to should never appear. Let me fix that.

There's a lot of space wasted by having a label (e.g. "Source" or "Peer A") above the filter selection

That's not something we can easilly change here. I don't want to have Peer A: Namespace: netobserv in a row. Maybe I should move the tips away ?

These could become a tooltip / popover.

The topology should not overlap the scope selection (e.g. Node).

I'm not sure to get what you mean here. Could you please give an example ?

When I click "Swap", the zoom level in topology resets.

Yes, any filter change re-run a query and redraw the topology as usual. We can improve that in a followup but let's focus on the filtering engine first.

In the filter field dropdown, since the list is quite long, I suggest to alphabetize it even though "Namespace" might be the most popular selection.

Yes, since we don't have the accordion anymore it makes sense. I can even put an autocomplete here if you feel it will help.

In the filter field dropdown, it doesn't have a scrollbar. The dropdown field scrolls with the window scrollbar which is kind of odd, and this only works with the scroller on my mouse. If I click the scrollbar, the dropdown disappears.

Let me check what I can do there 👍

Thoughts: I would still go back to "Source Namespace" and "Destination Namespace" fields. Then "Namespace" or explicitly "Source/Dest Namespace" means source or destination.

The "Swap" button would change the "Source" fields to "Destination" and vice versa.

How do you want to keep these without having a huge list and removing the accordion ?

github-actions · 2025-07-15T08:15:51Z

New image:
quay.io/netobserv/network-observability-console-plugin:98ac525

It will expire after two weeks.

To deploy this build, run from the operator repo, assuming the operator is running:

USER=netobserv VERSION=98ac525 make set-plugin-image

jotak

Reviewed the go code first, will do the frontend later

jotak · 2025-07-15T08:21:43Z

pkg/loki/flow_query.go

 	}

 	return nil
 }

-func (q *FlowQueryBuilder) addLineFilters(key string, values []string, not bool, moreThan bool) {
+func (q *FlowQueryBuilder) addLineFilters(key string, values []string, not, equal, moreThan bool) {


nit: I guess having addLineFilters(filter Match, values []string) starts to make more sense now?

jotak · 2025-07-15T08:23:27Z

pkg/loki/flow_query.go

@@ -132,6 +132,8 @@ func (q *FlowQueryBuilder) addLineFilters(key string, values []string, not bool,
 		var hasEmptyMatch bool
 		if q.config.IsNumeric(key) {
 			lf, hasEmptyMatch = filters.NumericLineFilter(key, values, not, moreThan)
+		} else if equal {
+			lf, hasEmptyMatch = filters.StringLineFilter(key, values, not)
 		} else {
 			lf, hasEmptyMatch = filters.StringLineFilterCheckExact(key, values, not)


Now that "exact" doesn't rely anymore on the double-quotes, can we remove the old logic in StringLineFilterCheckExact ? Or is it still needed?

I wanted to keep it in case you explicitly put quotes. I guess that could be useful if the user is not typing the entire query and didn't notices he is in "contains" mode.

Also, that will cover all cases for quickfilters:
https://github.com/netobserv/network-observability-console-plugin/pull/877/files#diff-f541e57c7f8271ea4db204c1d4b20b8d9d4592947242d2b33924f6f87df071f6R29-R30

quickfilters can also be migrated to the new format (we could even implement an automatic migration in the operator, so that it's fully transparent to the user). It's just to avoid having 2 different ways to do the same thing
(but that could be done in a follow-up as well)

jotak · 2025-07-15T08:30:15Z

pkg/model/filters/filters.go

+func NewMatch(key, values string) Match {
+	return Match{Key: key, Values: values}
+}
+func NewEqual(key, values string) Match {
+	return Match{Key: key, Values: values, Equal: true}
+}


I think it would be more clear to have NewEqualMatch and NewRegexMatch, and also perhaps having a Regex bool instead of an Equal bool (reversing the logic), as the equal match is the most natural; I'm nitpicking a bit I know, just trying to make it as easy to understand as possible.

jotak · 2025-07-15T08:38:51Z

pkg/model/filters/logql.go

+		if !equal {
+			// match the begining of string if quoted without a star
+			// and case insensitive if no quotes
+			if !strings.HasPrefix(value, `"`) {


here also, shouldn't we get rid of the "foo" syntax recognition?

See #877 (comment)

jotak · 2025-07-15T08:55:31Z

When I tried to open the filters dropdown, it disappeared! (firefox here, if that matters; and ocp 4.19)

Kooha-2025-07-15-10-52-52.webm

jpinsonneau · 2025-07-15T09:00:43Z

When I tried to open the filters dropdown, it disappeared! (firefox here, if that matters; and ocp 4.19)

Kooha-2025-07-15-10-52-52.webm

did you deployed netobserv/network-observability-operator#1632 ?

jotak · 2025-07-15T09:05:37Z

Any means you can have either source namespace = netobserv or destination namespace = openshift-console or both matching. I realise the From / To mentions should not appear in that case btw.

Peers means you will only get flows matching (source namespace = netobserv and destination namespace = openshift-console) OR (destination namespace = netobserv and source namespace = openshift-console)

If you feel it's not clear enough I can try to improve the display here but showing the full query will definitly take too much space in the screen.

I think I'm not convinced with "peer" name, because that's not necessarily between 2 peers, it can be just from and to a namespace for instance ; in that case, "peer" doesn't make so much sense. I know @stleerh mentioned previously he found the "back and forth" naming confusing, but IMO it's still more accurate. What could we use, else? "With return traffic" ?

Also, on not showing the full query because it takes too much space: how about having the full query displayed in a tooltip ?

jpinsonneau · 2025-07-15T09:13:06Z

Any means you can have either source namespace = netobserv or destination namespace = openshift-console or both matching. I realise the From / To mentions should not appear in that case btw.
Peers means you will only get flows matching (source namespace = netobserv and destination namespace = openshift-console) OR (destination namespace = netobserv and source namespace = openshift-console)
If you feel it's not clear enough I can try to improve the display here but showing the full query will definitly take too much space in the screen.

I think I'm not convinced with "peer" name, because that's not necessarily between 2 peers, it can be just from and to a namespace for instance ; in that case, "peer" doesn't make so much sense. I know @stleerh mentioned previously he found the "back and forth" naming confusing, but IMO it's still more accurate. What could we use, else? "With return traffic" ?

Asking AI about some keywords we could use here:

Bidirectionnal
Duplex traffic
Two-way communication
Endpoints (A & B) to replace Peers

Also, on not showing the full query because it takes too much space: how about having the full query displayed in a tooltip ?

My future goal is to bring a promQL input in the UI that will contains the current query when you switch from the guided view.

jotak · 2025-07-15T09:14:09Z

When I tried to open the filters dropdown, it disappeared! (firefox here, if that matters; and ocp 4.19)
Kooha-2025-07-15-10-52-52.webm

did you deployed netobserv/network-observability-operator#1632 ?

ah, no

jotak · 2025-07-15T09:16:08Z

There's a lot of space wasted by having a label (e.g. "Source" or "Peer A") above the filter selection

That's not something we can easilly change here. I don't want to have Peer A: Namespace: netobserv in a row. Maybe I should move the tips away ?

On that one, I think having 0-padding on pf-v5-c-menu-toggle (when it has a dropdown) helps a bit, although there's still the space taken by the upper label

jpinsonneau · 2025-07-15T09:38:01Z

Rebased without changes

jpinsonneau · 2025-07-15T10:44:11Z

On that one, I think having 0-padding on pf-v5-c-menu-toggle (when it has a dropdown) helps a bit, although there's still the space taken by the upper label

done in 56b2232

jotak · 2025-07-15T12:43:31Z

* Bidirectionnal

* Duplex traffic

* Two-way communication

* Endpoints (A & B) to replace Peers

"Bidirectionnal" sounds best to me. Simple, accurate and short. wdyt @stleerh ?

github-actions · 2025-07-15T13:06:02Z

New image:
quay.io/netobserv/network-observability-console-plugin:393e86f

It will expire after two weeks.

To deploy this build, run from the operator repo, assuming the operator is running:

USER=netobserv VERSION=393e86f make set-plugin-image

pkg/handler/flows.go

jotak · 2025-07-16T07:56:07Z

web/src/components/dropdowns/match-dropdown.tsx

+      case 'any':
+        return (
+          <>
+            <LongArrowAltUpIcon />
+            <LongArrowAltDownIcon />
+          </>
+        );
+      case 'all':
+        return <LongArrowAltUpIcon />;
+      case 'peers':
+        return <RouteIcon />;


I'd put "all" on top, it should be the default (at least it has always been)

The Up/Down icons look more appropriate for peers (which we may rename as "bidirectional")

But I can't see which icon would really work to distinguish any/all , then... ; if we don't find something, we can still remove those icons and keep only text.

(I mean, putting All on top is for L13 not here)

Hm yeah indeed I forgot to update these icons. Currently it appears only next to the selected one:

Would it be better like this ?

381ef88

jotak · 2025-07-16T08:13:35Z

web/src/model/filters.ts

@@ -1,12 +1,14 @@
 import _ from 'lodash';
+import { FilterCompare } from '../components/toolbar/filters/compare-filter';


Could you perhaps move FilterCompare here? It doesn't sound right to have a model that depends on a component, that would be better the other way around.

Co-authored-by: Joel Takvorian <[email protected]>

jpinsonneau · 2025-07-23T09:11:28Z

Rebased without changes

openshift-ci · 2025-07-23T12:25:01Z

@jpinsonneau: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/plugin-cypress	`5266018`	link	true	`/test plugin-cypress`
ci/prow/qe-e2e-console-tests	`5266018`	link	false	`/test qe-e2e-console-tests`

Full PR test history. Your PR dashboard.

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-sigs/prow repository. I understand the commands that are listed here.

openshift-ci bot added the do-not-merge/work-in-progress label Jun 3, 2025

jpinsonneau requested a review from stleerh June 4, 2025 07:17

jpinsonneau added the needs-review Tells that the PR needs a review label Jun 4, 2025

jpinsonneau changed the title ~~PoC : Provide a way to change the filter field~~ NETOBSERV-2183 PoC : Provide a way to change the filter field Jun 4, 2025

jpinsonneau changed the title ~~NETOBSERV-2183 PoC : Provide a way to change the filter field~~ NETOBSERV-2182 & NETOBSERV-2183 PoC : Filters refactoring Jun 5, 2025

jpinsonneau force-pushed the 2183 branch from 1423a09 to ce2485d Compare June 5, 2025 13:04

jpinsonneau commented Jun 5, 2025

View reviewed changes

web/src/utils/__tests__/back-and-forth.spec.ts Outdated Show resolved Hide resolved

jpinsonneau mentioned this pull request Jun 12, 2025

NETOBSERV-2182 & NETOBSERV-2183 PoC : Filters refactoring netobserv/network-observability-operator#1632

Open

10 tasks

jpinsonneau force-pushed the 2183 branch from ce2485d to ff787f5 Compare June 12, 2025 14:32

jpinsonneau force-pushed the 2183 branch from 9427285 to 49a0ed9 Compare July 2, 2025 10:36

jpinsonneau marked this pull request as ready for review July 2, 2025 10:38

openshift-ci bot removed the do-not-merge/work-in-progress label Jul 2, 2025

jpinsonneau requested review from jotak and OlivierCazade July 3, 2025 13:07

jotak added the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Jul 15, 2025

jotak reviewed Jul 15, 2025

View reviewed changes

jpinsonneau force-pushed the 2183 branch from 650fdc3 to 7671e53 Compare July 15, 2025 09:37

github-actions bot removed the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Jul 15, 2025

jotak added the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Jul 15, 2025

jotak reviewed Jul 15, 2025

View reviewed changes

pkg/handler/flows.go Outdated Show resolved Hide resolved

github-actions bot removed the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Jul 16, 2025

jotak reviewed Jul 16, 2025

View reviewed changes

jpinsonneau mentioned this pull request Jul 22, 2025

NETOBSERV-2097 pf6 update #721

Draft

10 tasks

jpinsonneau and others added 17 commits July 23, 2025 11:05

chips refactoring for more options

dddbf04

refactor match and back and forth options

3ebf1a0

search input with popper

3805569

fix styling

0e6533a

adapt src / dst actions naming when using peers

1773477

explicitly show and / or

ffe1128

address feedback

366446d

add autocompletes on search field

a225f73

equals vs contains

7656b0e

improved suggestions and display

b3ace9c

fix autocomplete display value usage

d2c84e5

simplify addLineFilters args

bcb12a5

regex / equal match

11855f6

remove padding on filters menu toggle

1555473

Update pkg/handler/flows.go

19ee838

Co-authored-by: Joel Takvorian <[email protected]>

improve match dropdown

e89b4e7

move FilterCompare type to model

5266018

jpinsonneau force-pushed the 2183 branch from f1ae8bd to 5266018 Compare July 23, 2025 09:11

		@@ -1,12 +1,14 @@
		import _ from 'lodash';
		import { FilterCompare } from '../components/toolbar/filters/compare-filter';

NETOBSERV-2182 & NETOBSERV-2183 PoC : Filters refactoring #877

Are you sure you want to change the base?

NETOBSERV-2182 & NETOBSERV-2183 PoC : Filters refactoring #877

Uh oh!

Conversation

jpinsonneau commented Jun 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Dependencies

Checklist

Uh oh!

openshift-ci bot commented Jun 3, 2025

Uh oh!

openshift-ci bot commented Jun 3, 2025

Uh oh!

codecov bot commented Jun 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

stleerh commented Jun 13, 2025

Uh oh!

jpinsonneau commented Jun 16, 2025

Uh oh!

github-actions bot commented Jul 15, 2025

Uh oh!

jotak 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!

Choose a reason for hiding this comment

Uh oh!

jotak commented Jul 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jpinsonneau commented Jul 15, 2025

Uh oh!

jotak commented Jul 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jpinsonneau commented Jul 15, 2025

Uh oh!

jotak commented Jul 15, 2025

Uh oh!

jotak commented Jul 15, 2025

Uh oh!

jpinsonneau commented Jul 15, 2025

Uh oh!

jpinsonneau commented Jul 15, 2025

Uh oh!

jotak commented Jul 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jul 15, 2025

Uh oh!

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

jpinsonneau commented Jun 3, 2025 •

edited

Loading

codecov bot commented Jun 3, 2025 •

edited

Loading

jotak commented Jul 15, 2025 •

edited

Loading

jotak commented Jul 15, 2025 •

edited

Loading

jotak commented Jul 15, 2025 •

edited

Loading