-
Notifications
You must be signed in to change notification settings - Fork 14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
NETOBSERV-1269 refactor overlapping detection for BNF #379
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #379 +/- ##
==========================================
- Coverage 57.69% 57.33% -0.37%
==========================================
Files 166 168 +2
Lines 7817 7936 +119
Branches 962 968 +6
==========================================
+ Hits 4510 4550 +40
- Misses 3030 3107 +77
- Partials 277 279 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. 📢 Have feedback on the report? Share it here. |
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=5f8efb0 make set-plugin-image |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's test it 😸 thanks @jotak
@@ -387,6 +388,13 @@ export const NetflowTraffic: React.FC<{ | |||
topologyOptions.groupTypes | |||
]); | |||
|
|||
const getFetchFunctionsClbk = React.useCallback(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a huge fan of this Clbk
suffix here. Can we either:
- use distinct names in
back-and-forth.ts
/netflow-traffic.tsx
- rename import like
import { getFetchFunctions as getBackAndForthFetch } from '../utils/back-and-forth';
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure I can do that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
// When bnf is on, this replaces the usual getTopologyMetrics with a function with same arguments that runs 3 queries and merge their results | ||
// in order to get the ORIGINAL + SWAPPED - OVERLAP | ||
// OVERLAP being ORIGINAL AND SWAPPED. | ||
// E.g: if ORIGINAL is "SrcNs=foo", SWAPPED is "DstNs=foo" and OVERLAP is "SrcNs=foo AND DstNs=foo" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That may increase the too many outstanding request
issue when not using filters.
See https://issues.redhat.com/browse/NETOBSERV-1235 comments with recording.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, that's one more query indeed...
I don't see this error often, do you?
we could also start thinking more seriously about a throttling mechanism in the UI ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't retested using loki-operator but it will since the default value is used for outstanding requests.
We already have a story for graphs loadings improvments: https://issues.redhat.com/browse/NETOBSERV-1185
Let's start from there since it's the page that runs the most of them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't remember, do we have a workaround for the "too many outstanding requests" issue? (a loki config?)
Doing some tests on my side, I don't get the error, but I can imagine people may have it.
I can also take a stab at 1185 but that would likely be another PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually, how about chaining those promises? bc076f0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To reproduce you need to remove all filters and set auto refresh to 15s or spam the refresh button.
Then it will appear by itself after some time.
As far as I know loki-operator
doesn't expose this config.
https://redhat-internal.slack.com/archives/C02939DP5L5/p1693313480687369?thread_ts=1693259801.293669&cid=C02939DP5L5
Chaining the requests would not solve the issue and will make the page load slower right ?
We need to take advantage of parallelization; Loki query-frontend
component is designed for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Chaining the requests would not solve the issue and will make the page load slower right ?
Make it load slower => yes; but it should reduce the number of parallel queries, which in my understanding is the cause of "too many outstanding requests" error ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fwiw netobserv/network-observability-operator#409 + #380 should make it less likely to get "too many outstanding requests"
LGTM after testing both 1269 & 1270 cases 🥳 |
This fix allows working on situations as described in https://issues.redhat.com/browse/NETOBSERV-1269 and, hopefully, is a more generic / consistent solution anyway. Changing how overlapping results are managed: work on results rather than on query to remove overlapping parts. When fetching flows, we don't explicitly remove the overlaps, but this is managed implicitly with the existing dedup functions When fetching metrics, we consider that the computed metrics for any label set is the ORIGINAL metrics + the SWAPPED metrics - the OVERLAP metrics. Add tests, fix tests Note that adjustment is necessary in MetricsQuerySummary test as the "endWithTolerance" calculation has changed a little bit, modifying computed rates ... new values are still close to the previous ones Rename callback function
/approve |
[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:
Approvers can indicate their approval by writing |
This fix allows working on situations as described in https://issues.redhat.com/browse/NETOBSERV-1269 and, hopefully, is a more generic / consistent solution in any case:
It changes how overlapping results are managed, now working on results rather than on query to remove overlapping parts.
Also fixes https://issues.redhat.com/browse/NETOBSERV-1270