NETOBSERV-226 Refactor query API and filters management #105

jotak · 2022-03-24T09:59:08Z

JIRA https://issues.redhat.com/browse/NETOBSERV-226

Backend change:

Backend http API: more explicit query params to not mix filters with non-filter params
"Match any" rewritten: now runs multiple subqueries in parallel, and aggregate the results. It's a better use of labels (indexes), so should be more performant, and also less complexity in LogQL queries
Refactoring the groups logic to handle a couple of missing cases, like with match all + (Src+Dst) common filters. Still a complex part (but complexity was moved from the query builder to pre-processing of filters)
More separation between CSV-export code/endpoint and flow-fetching code/endpoint
Fixed some mixed up uses of stringLabelFilter / regexLabelFilter (basically: always use regexLabelFilter when regex is involved)
Removed some regex feature within exact match (it seems a bit weird to me to me exact match and regex..., and also simplified code to remove ... but we can add it back if there's really some use cases)
Some utility stuff ...
More tests

Frontend change:

Dissociate Columns vs Filters (we must be able to add new filters without adding new columns)
Adapt to new backend API: filters are now passed as a unique query param
Code quality: avoid super large files, split in more react components, etc.
Moved backend logic of all/any matching to frontend. Defining some of the logic rightly in the filter definition themselves help simplifying the code (IMHO)

openshift-ci · 2022-03-24T09:59:09Z

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

jotak · 2022-03-24T09:59:40Z

opening as draft, needs frontend update (wip) and some cleanup

jotak · 2022-03-24T10:00:23Z

pkg/handler/flows.go

+	return parsed
+}
+
+// groupFilters creates groups of filters for fetching strategy, that depends on the match all/any param


I'm not super fan of this groupFilters function, will try to simplify it

jotak · 2022-03-24T10:15:54Z

pkg/server/flows_test.go

+		// 	inputPath: "?filters=" + url.QueryEscape("SrcK8S_Namespace=\"start-namespace*\""),
+		// 	outputQueries: []string{
+		// 		`?query={app="netobserv-flowcollector",SrcK8S_Namespace=~"^start-namespace.*"}`,
+		// 	},
+		// }, {
+		// 	inputPath: "?filters=" + url.QueryEscape("SrcK8S_Namespace=\"*end-namespace\""),
+		// 	outputQueries: []string{
+		// 		`?query={app="netobserv-flowcollector",SrcK8S_Namespace=~".*end-namespace$"}`,
+		// 	},
+		// }, {
+		// 	inputPath: "?filters=" + url.QueryEscape("SrcK8S_Namespace=\"mid-n*e\""),
+		// 	outputQueries: []string{
+		// 		`?query={app="netobserv-flowcollector",SrcK8S_Namespace=~"^mid-n.*e$"}`,
+		// 	},


@jpinsonneau I'd like to get your opinion on that. At the moment, I've removed this mix of exact-matching/start-matching/end-matching, mostly because I don't really see the point of having these particular matches using the double-quotes (which, to me, really mean exact match). I guess if you need these start/end matches, you can use the partial match without the double-quotes, no? Or am I missing a use case?

sure so as soon as you will find a * char in the filter you will go for partial match right ?

jpinsonneau · 2022-03-24T16:46:21Z

pkg/handler/flows.go

+				aggStreams = streams
+				aggregated = r
+			} else {
+				aggStreams = append(aggStreams, streams...)


How do you plan to remove duplicates here ?
What about the limit ?

good points, it needs dedup, I will work on it.

for limit, either we just throw away results outside of the limit, but but it might be a shame, so another option could be to find some twist in the UI to not present the limit option as something hard-defined, but with a more qualitative wording instead? (e.g 100 => fetch few, 500 => fetch more , 1000 => fetch many more .. or find something better) any idea?

For sure this solution comes with some challenges, but IMO we cannot continue without using loki indexes if we want to scale

jotak · 2022-03-30T12:46:52Z

@jpinsonneau this is almost ready, I think it's fine to start reviewing / testing, though there's still a couple of things that I need to do:

Test / fix the CSV export (I'm pretty sure it's broken)
Restore some missing filters
Check use of _.cloneDeep(filters) (probably bugged)
Add some tests (e.g. on flow-query.ts)
Restore more tests on filters component

I've edited the PR description to add details about what changed / why.

github-actions · 2022-03-30T12:59:37Z

New image: ["quay.io/netobserv/network-observability-console-plugin:9a460ce"]. It will expire after two weeks.

github-actions · 2022-03-31T16:26:19Z

New image: ["quay.io/netobserv/network-observability-console-plugin:2329388"]. It will expire after two weeks.

jotak · 2022-03-31T16:27:15Z

New image: ["quay.io/netobserv/network-observability-console-plugin:2329388"]. It will expire after two weeks.

@memodi hey, here's the image built from CI

- Backend http API: more explicit query params to not mix filters with non-filter params - "Match any" rewritten: now runs multiple subqueries in parallel, and aggregate the results. It's a better use of labels (indexes), so should be more performant, and also less complexity in LogQL queries - Refactoring the groups logic to handle a couple of missing cases, like with match all + (Src+Dst) common filters. Still a complex part (but complexity was moved from the query builder to pre-processing of filters) - More separation between CSV-export code/endpoint and flow-fetching code/endpoint - Fixed some mixed up uses of stringLabelFilter / regexLabelFilter (basically: always use regexLabelFilter when regex is involved) - Removed some regex feature within exact match (it seems a bit weird to me to me exact match and regex..., and also simplified code to remove ... but we can add it back if there's really some use cases) - Some utility stuff ... - More tests Add some test, move export handler to own file Remove the filters grouping logic It will be implemented straight from the frontend

- Dissociate Columns vs Filters (we must be able to add new filters without adding new columns) - Adapt to new backend API: filters are now passed as a unique query param - Code quality: avoid super large files, split in more react components, etc. - Moved backend logic of all/any matching to frontend. Defining some of the logic rightly in the filter definition themselves help simplifying the code Fix case with different and-filter on same key Fix k8s resource autocompletion, add tests Also fix k8s name regex being too permissive Fix CSV export Add some missing filters - Owner name - Owner kind (merged with kind) - Host renamed Node IP Add more tests fix conflicts Fix warning in test Warning about state modified outside of 'act' was triggered.

- LogQL fix on json "or" fields - Close autocomplete popup on blur - Avoid unnessary call to setIndicator

There was 2 problems with the algo: - Flow records are {stream selector} * Entry line, that is, each entry line in the results has to be considered individually, and merged for iodentical stream selectors - Labels order in stream selector can vary. They must be compared in a consistent way

jotak · 2022-04-07T07:46:03Z

@jpinsonneau my last commit should fix the issue that we've seen

github-actions · 2022-04-07T13:29:42Z

New image: ["quay.io/netobserv/network-observability-console-plugin:f4047c6"]. It will expire after two weeks.

memodi · 2022-04-07T15:40:39Z

/cc @brahaney

memodi · 2022-04-07T18:18:46Z

@jotak I tested this and didn't find any issues with it. Except for my this comment and UX discussion doc which can be probably addressed in follow up tasks if you agree, rest LGTM. I'll let @brahaney add qe-approved label based on his test results.

brahaney

/qe-approved

jotak · 2022-04-08T07:30:27Z

Thank you @brahaney @memodi @jpinsonneau !
Julien, I don't know if you resume your tests since Dan and Mehul approved, let me know if you find anything wrong

jotak · 2022-04-08T07:30:33Z

/approve

openshift-ci · 2022-04-08T07:30:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jotak

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~~ [jotak]

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

jpinsonneau · 2022-04-08T10:45:30Z

Here are some feedback about my today's tests:

I'm not able to type "* or *" for start / end match in Name filter in front end anymore
```
export const k8sName = RegExp('^[-a-zA-Z0-9*]*$');
```
This works fine for Labels like Namespaces
Resource dropdown doesn't work with Node. value even if it's suggested in the list

Not sure if it's related to this PR but I've also seen these bugs:

Wrong source / destination results
- Set 'Destination Name' to 'loki'
- Wrong "dns-default" match
Wrong port results:
- Set 'Port' to '80'
- Getting results about '8080'

Fixes netobserv#105 (comment) wrong "dns-default" appearing when searching for dest name loki

jotak · 2022-04-08T13:59:37Z

@jpinsonneau I fixed 2 of the mentioned issues, and created 2 tickets for the others:

Fixes #105 (comment) wrong "dns-default" appearing when searching for dest name loki

openshift-ci bot added the do-not-merge/work-in-progress label Mar 24, 2022

jotak commented Mar 24, 2022

View reviewed changes

jotak requested a review from jpinsonneau March 24, 2022 10:01

jotak mentioned this pull request Mar 24, 2022

NETOBSERV-178 Topology: build queries for Loki #101

Merged

jotak commented Mar 24, 2022

View reviewed changes

jotak force-pushed the refactor-backend-flow-api branch from bd5c3e5 to eb71fcb Compare March 24, 2022 11:23

jpinsonneau reviewed Mar 24, 2022

View reviewed changes

openshift-ci bot added the needs-rebase label Mar 24, 2022

jotak force-pushed the refactor-backend-flow-api branch from eb71fcb to 8f8aba6 Compare March 24, 2022 17:38

openshift-ci bot removed the needs-rebase label Mar 25, 2022

jotak force-pushed the refactor-backend-flow-api branch from 2443a0a to bebd019 Compare March 30, 2022 10:31

jotak changed the title ~~Refactor query API~~ Refactor query API and filters management Mar 30, 2022

jotak requested a review from jpinsonneau March 30, 2022 12:44

jotak added the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Mar 30, 2022

jotak force-pushed the refactor-backend-flow-api branch from 44dc2d8 to 99551dd Compare March 30, 2022 12:54

jotak added ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. and removed ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. labels Mar 30, 2022

jotak force-pushed the refactor-backend-flow-api branch from a025f97 to 7b1265b Compare March 30, 2022 16:46

jotak added ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. and removed ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. labels Mar 31, 2022

jotak force-pushed the refactor-backend-flow-api branch from f267539 to 13e2c0f Compare March 31, 2022 16:03

jotak added ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. and removed ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. labels Mar 31, 2022

jotak changed the title ~~Refactor query API and filters management~~ NETOBSERV-226 Refactor query API and filters management Apr 1, 2022

jotak requested a review from jpinsonneau April 6, 2022 07:20

jotak added 4 commits April 7, 2022 09:33

Fix several issues:

ae66963

- LogQL fix on json "or" fields - Close autocomplete popup on blur - Avoid unnessary call to setIndicator

Fix merge conflicts

d8ec2ed

jotak force-pushed the refactor-backend-flow-api branch from ae47385 to d8ec2ed Compare April 7, 2022 09:11

jotak added the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Apr 7, 2022

openshift-ci bot requested a review from brahaney April 7, 2022 15:40

brahaney approved these changes Apr 7, 2022

View reviewed changes

openshift-ci bot assigned brahaney Apr 7, 2022

openshift-ci bot added the lgtm label Apr 7, 2022

openshift-ci bot added the approved label Apr 8, 2022

openshift-merge-robot merged commit eadc9be into netobserv:main Apr 8, 2022

jotak added a commit to jotak/network-observability-console-plugin that referenced this pull request Apr 8, 2022

Fix linefilter regex, disallowing '"' in before-match

c6c2fae

Fixes netobserv#105 (comment) wrong "dns-default" appearing when searching for dest name loki

jotak added a commit to jotak/network-observability-console-plugin that referenced this pull request Apr 8, 2022

Fix linefilter regex, disallowing '"' in before-match

6f52938

Fixes netobserv#105 (comment) wrong "dns-default" appearing when searching for dest name loki

jotak mentioned this pull request Apr 8, 2022

Fix linefilter regex, disallowing '"' in before-match #119

Merged

jpinsonneau mentioned this pull request Apr 11, 2022

NETOBSERV-226 matrix merger implementation #120

Merged

openshift-merge-robot pushed a commit that referenced this pull request Apr 19, 2022

Fix linefilter regex, disallowing '"' in before-match (#119)

8debd99

Fixes #105 (comment) wrong "dns-default" appearing when searching for dest name loki

jpinsonneau mentioned this pull request May 11, 2022

NETOBSERV-291 - NetFlow table needs pagination #121

Closed

jotak deleted the refactor-backend-flow-api branch November 7, 2024 10:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NETOBSERV-226 Refactor query API and filters management #105

NETOBSERV-226 Refactor query API and filters management #105

jotak commented Mar 24, 2022 •

edited

Loading

openshift-ci bot commented Mar 24, 2022

jotak commented Mar 24, 2022

jotak Mar 24, 2022

jotak Mar 24, 2022

jpinsonneau Mar 24, 2022

jpinsonneau Mar 24, 2022

jotak Mar 24, 2022

jotak commented Mar 30, 2022 •

edited

Loading

github-actions bot commented Mar 30, 2022

github-actions bot commented Mar 31, 2022

jotak commented Mar 31, 2022

jotak commented Apr 7, 2022

github-actions bot commented Apr 7, 2022

memodi commented Apr 7, 2022

memodi commented Apr 7, 2022

brahaney left a comment

jotak commented Apr 8, 2022

jotak commented Apr 8, 2022

openshift-ci bot commented Apr 8, 2022

jpinsonneau commented Apr 8, 2022

jotak commented Apr 8, 2022

NETOBSERV-226 Refactor query API and filters management #105

NETOBSERV-226 Refactor query API and filters management #105

Conversation

jotak commented Mar 24, 2022 • edited Loading

openshift-ci bot commented Mar 24, 2022

jotak commented Mar 24, 2022

jotak Mar 24, 2022

Choose a reason for hiding this comment

jotak Mar 24, 2022

Choose a reason for hiding this comment

jpinsonneau Mar 24, 2022

Choose a reason for hiding this comment

jpinsonneau Mar 24, 2022

Choose a reason for hiding this comment

jotak Mar 24, 2022

Choose a reason for hiding this comment

jotak commented Mar 30, 2022 • edited Loading

github-actions bot commented Mar 30, 2022

github-actions bot commented Mar 31, 2022

jotak commented Mar 31, 2022

jotak commented Apr 7, 2022

github-actions bot commented Apr 7, 2022

memodi commented Apr 7, 2022

memodi commented Apr 7, 2022

brahaney left a comment

Choose a reason for hiding this comment

jotak commented Apr 8, 2022

jotak commented Apr 8, 2022

openshift-ci bot commented Apr 8, 2022

jpinsonneau commented Apr 8, 2022

jotak commented Apr 8, 2022

jotak commented Mar 24, 2022 •

edited

Loading

jotak commented Mar 30, 2022 •

edited

Loading