NETOBSERV-2182 & NETOBSERV-2183 PoC : Filters refactoring#877
NETOBSERV-2182 & NETOBSERV-2183 PoC : Filters refactoring#877jpinsonneau merged 27 commits intonetobserv:mainfrom
Conversation
|
Skipping CI for Draft Pull Request. |
a) What does it mean to have: compared to: b) The dropdown is used to combine/separate peers. This is not intuitive.
a) How do I add Destination Namespace? b) If I click "Swap", it changes to: If I click "Swap" again, it changes to: Previously, there was no "From". It was just:
Thoughts: The "Swap" button would change the "Source" fields to "Destination" and vice versa. |
|
Thanks for your feedback @stleerh. Let me try to adress all of these:
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.
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.
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.
The from / to should never appear. Let me fix that.
That's not something we can easilly change here. I don't want to have These could become a tooltip / popover.
I'm not sure to get what you mean here. Could you please give an example ?
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.
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.
Let me check what I can do there 👍
How do you want to keep these without having a huge list and removing the accordion ? |
|
New image: 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
left a comment
There was a problem hiding this comment.
Reviewed the go code first, will do the frontend later
| } | ||
|
|
||
| func (q *FlowQueryBuilder) addLineFilters(key string, values []string, not bool, moreThan bool) { | ||
| func (q *FlowQueryBuilder) addLineFilters(key string, values []string, not, equal, moreThan bool) { |
There was a problem hiding this comment.
nit: I guess having addLineFilters(filter Match, values []string) starts to make more sense now?
| } else if equal { | ||
| lf, hasEmptyMatch = filters.StringLineFilter(key, values, not) | ||
| } else { | ||
| lf, hasEmptyMatch = filters.StringLineFilterCheckExact(key, values, not) |
There was a problem hiding this comment.
Now that "exact" doesn't rely anymore on the double-quotes, can we remove the old logic in StringLineFilterCheckExact ? Or is it still needed?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)
| 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} | ||
| } |
There was a problem hiding this comment.
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.
| if !equal { | ||
| // match the begining of string if quoted without a star | ||
| // and case insensitive if no quotes | ||
| if !strings.HasPrefix(value, `"`) { |
There was a problem hiding this comment.
here also, shouldn't we get rid of the "foo" syntax recognition?
|
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/netobserv-operator#1632 ? |
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 ? |
Asking AI about some keywords we could use here:
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. |
ah, no |
On that one, I think having 0-padding on |
|
Rebased without changes |
Co-authored-by: Joel Takvorian <joel.takvorian@homeblocks.net>
|
Rebased and fixed gateway case in filters |
jotak
left a comment
There was a problem hiding this comment.
nice work, LGTM
I think it's ok if we address some of the pending points currently under discussion in a follow-up
|
/ok-to-test |
|
New image: It will expire after two weeks. To deploy this build, run from the operator repo, assuming the operator is running: USER=netobserv VERSION=c36b858 make set-plugin-image |
|
@jpinsonneau We can go ahead and merge these PRs since its been sitting for a while. Screen.Recording.2025-12-17.at.1.40.06.PM.movI dont mind raising a bug for the same and tracking in that instead of holding these PRs |
Hey @Amoghrd, good catch. That sounds like a minor but we can adress later indeed. Thanks ! |
|
/label qe-approved |




Description
Merge
Matchoption andBack and forthones into a single new dropdown appearing next to filters values as:Add dropdowns under filters values allowing:
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/netobserv-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.