NETOBSERV-1185 Console plugin async overview metrics #389

jpinsonneau · 2023-09-13T14:40:38Z

This PR improve Overview panel loading & code:

Renamed records & metrics routes to
- /loki/flow/records
- /loki/flow/metrics
Show graphs as soon as metrics are available
Keep previously loaded graph dimensions in local storage
Reduce number of queries according to selected panels / features
Add number of queries in summary
Fix testing

You can either check the number of queries in the side panel or in your browser debug -> network section to test this 🤓

*/!\ QE automated tests might break since the API route has been renamed and loading behavior changed /!*

codecov · 2023-09-13T14:45:00Z

Codecov Report

Attention: 18 lines in your changes are missing coverage. Please review.

Comparison is base (62bdb9d) 56.69% compared to head (ebc3c45) 57.64%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #389      +/-   ##
==========================================
+ Coverage   56.69%   57.64%   +0.94%     
==========================================
  Files          29      169     +140     
  Lines        2025     7977    +5952     
  Branches        0      968     +968     
==========================================
+ Hits         1148     4598    +3450     
- Misses        761     3103    +2342     
- Partials      116      276     +160

Flag	Coverage Δ
uitests	`58.68% <86.44%> (?)`
unittests	`54.71% <50.00%> (-1.98%)`	⬇️

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

Files	Coverage Δ
pkg/server/routes.go	`89.65% <100.00%> (ø)`
web/src/api/loki.ts	`100.00% <100.00%> (ø)`
web/src/components/__tests-data__/config.ts	`100.00% <100.00%> (ø)`
web/src/components/__tests-data__/flows.ts	`100.00% <100.00%> (ø)`
web/src/components/metrics/metrics-content.tsx	`86.66% <100.00%> (ø)`
...b/src/components/metrics/metrics-total-content.tsx	`93.10% <100.00%> (ø)`
web/src/components/metrics/stat-donut.tsx	`68.29% <100.00%> (ø)`
...c/components/netflow-overview/netflow-overview.tsx	`70.00% <ø> (ø)`
...c/components/query-summary/flows-query-summary.tsx	`96.15% <100.00%> (ø)`
...components/query-summary/metrics-query-summary.tsx	`61.76% <100.00%> (ø)`
... and 11 more

... and 121 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pkg/server/routes.go

jotak · 2023-09-29T08:13:19Z

web/src/components/metrics/metrics-content.tsx

 import { NamedMetric } from '../../api/loki';
 import { MetricType } from '../../model/flow-query';
+import { LOCAL_STORAGE_OVERVIEW_METRICS_DIMENSION_KEY, useLocalStorage } from '../../utils/local-storage-hook';


Side remark: We might want to use some formatting tool to organize import automatically like we have on the go side
E.g. https://medium.com/weekly-webtips/how-to-sort-imports-like-a-pro-in-typescript-4ee8afd7258a

Added to https://issues.redhat.com/browse/NETOBSERV-268

jotak · 2023-09-29T08:23:26Z

web/src/components/netflow-traffic.tsx

-          return res.stats;
-        })
-      ];
+      if (!selectedPanels.some(p => [DROPPED_ID_MATCHER, DNS_ID_MATCHER, RTT_ID_MATCHER].includes(p.id))) {


not sure to understand this logic : if there's any of [drops/dns/rtt] in panels then we don't fetch the bytes/packets metrics? Why?

If there is any panel which is not requiring special query (drop / dns / rtt),
then we query the defaults metrics..

An alternative could be to prepend a matcher to these defaults to manage this case the same way as nexts ones but users will loose their previous selections since it's stored in local storage.

I might be wrong but I think that selectedPanels.some(p => [DROPPED_ID_MATCHER, DNS_ID_MATCHER, RTT_ID_MATCHER].includes(p.id)) is always false.
This comes down to writing for instance ["drops", "dns", "rtt"].includes("top_avg_rtt_line"), that's never going to happen (?)
Should it be instead:

selectedPanels.some(p => [DROPPED_ID_MATCHER, DNS_ID_MATCHER, RTT_ID_MATCHER].some(matcher => p.id.includes(matcher)))

?

I think this would be more correct and is perhaps easier to understand:

if (selectedPanels.some(p => !p.id.includes(DROPPED_ID_MATCHER) && !p.id.includes(DNS_ID_MATCHER) && !p.id.includes(RTT_ID_MATCHER))) { }

You are right, thanks for catching that !

I applied your last suggestion: 15babbe

jotak

mostly LGTM with just a doubt on something
I'll also run some tests before approving

github-actions · 2023-09-29T08:36:39Z

New image:
quay.io/netobserv/network-observability-console-plugin:3be4f7a

It will expire after two weeks.

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

USER=netobserv VERSION=3be4f7a make set-plugin-image

jotak · 2023-09-29T11:23:13Z

After testing, I can confirm my comment here that showing only [dns/rtt/drops] panel won't save us from doing a standard flows query, so I think that should be fixed.

Also a small thing, looks like the Globe icon alignment isn't perfect:

Apart from that, this looks good

jpinsonneau · 2023-09-29T12:13:16Z

Also a small thing, looks like the Globe icon alignment isn't perfect:

should be better with that: 2be8de9

jotak

LGTM

jpinsonneau · 2023-09-29T15:43:01Z

@nathan-weinberg would it be fine to merge this PR before QE validation ?
It may break your automated testings 🤔

I have 3 PRs needing these changes. If you prefer to take your time I will rebase them on this PR.
Thanks !

nathan-weinberg · 2023-10-03T13:20:55Z

/cc @Amoghrd

@jpinsonneau If it's a breaking change, let's try and update the tests first so our regressions do not turn up false positives - i.e. prefer the rebase method :D

Do we have a Jira for this change?

jpinsonneau · 2023-10-03T14:23:27Z

Do we have a Jira for this change?

I've created https://issues.redhat.com/browse/NETOBSERV-1348 for you guys

nathan-weinberg · 2023-10-03T15:20:50Z

/ok-to-test

github-actions · 2023-10-03T15:25:11Z

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

It will expire after two weeks.

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

USER=netobserv VERSION=ba1acf4 make set-plugin-image

nathan-weinberg · 2023-10-03T22:38:45Z

With changes in https://github.com/openshift/openshift-tests-private/pull/11959 the test cases remain stable: https://mastern-jenkins-csb-openshift-qe.apps.ocp-c1.prod.psi.redhat.com/job/ocp-veno/job/no-regression/280/

@jpinsonneau any manual verifications need to be done before merging?

jpinsonneau · 2023-10-04T07:26:35Z

@jpinsonneau any manual verifications need to be done before merging?

Not particularly. Just that the graphs still shows 😸

nathan-weinberg · 2023-10-04T14:47:25Z

@jpinsonneau any manual verifications need to be done before merging?

Not particularly. Just that the graphs still shows 😸

Sounds good - I am spinning up another cluster today to investigate some frontend failures @Amoghrd wanted me to double-check so I can do that as well!

openshift-ci · 2023-10-04T15:13:02Z

New changes are detected. LGTM label has been removed.

jpinsonneau · 2023-10-04T15:14:15Z

Rebased the PR
no changes from my end

nathan-weinberg · 2023-10-04T15:23:34Z

/ok-to-test

github-actions · 2023-10-04T15:27:38Z

New image:
quay.io/netobserv/network-observability-console-plugin:74296d1

It will expire after two weeks.

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

USER=netobserv VERSION=74296d1 make set-plugin-image

nathan-weinberg · 2023-10-04T19:02:49Z

Full Success Run with our automated tests: https://mastern-jenkins-csb-openshift-qe.apps.ocp-c1.prod.psi.redhat.com/job/ocp-veno/job/no-regression/281/

nathan-weinberg · 2023-10-04T19:02:52Z

/label qe-approved

jpinsonneau · 2023-10-05T08:24:43Z

Thanks for the team effort ! 🥳 Merging now

openshift-ci · 2023-10-05T08:25:03Z

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by:

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

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

openshift-ci bot added the do-not-merge/work-in-progress label Sep 13, 2023

jpinsonneau force-pushed the 1185 branch from 82cdfa8 to 5036bb6 Compare September 14, 2023 15:41

jpinsonneau requested review from jotak and OlivierCazade September 14, 2023 15:43

jpinsonneau marked this pull request as ready for review September 14, 2023 15:43

openshift-ci bot removed the do-not-merge/work-in-progress label Sep 14, 2023

This was referenced Sep 22, 2023

WIP - TCP retransmit #400

Closed

NETOBSERV-1337 DSCP filter & side panel details #401

Merged

NETOBSERV-1319 single graph focus #405

Merged

jotak reviewed Sep 29, 2023

View reviewed changes

pkg/server/routes.go Show resolved Hide resolved

jotak reviewed Sep 29, 2023

View reviewed changes

jotak added the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Sep 29, 2023

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

jotak previously approved these changes Sep 29, 2023

View reviewed changes

openshift-ci bot assigned jotak Sep 29, 2023

openshift-ci bot added the lgtm label Sep 29, 2023

openshift-ci bot requested a review from Amoghrd October 3, 2023 13:20

jpinsonneau added the testing-breaking-change label Oct 3, 2023

openshift-ci bot added the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Oct 3, 2023

openshift-merge-robot added the needs-rebase label Oct 4, 2023

jpinsonneau added 3 commits October 4, 2023 17:09

Console plugin async overview metrics

cb5d990

fix fetchOverview default query condition

4c8703b

query summary row align center

ebc3c45

jpinsonneau dismissed jotak’s stale review via ebc3c45 October 4, 2023 15:12

jpinsonneau force-pushed the 1185 branch from 2be8de9 to ebc3c45 Compare October 4, 2023 15:12

openshift-ci bot removed the lgtm label Oct 4, 2023

openshift-merge-robot removed the needs-rebase label Oct 4, 2023

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

openshift-ci bot added the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Oct 4, 2023

openshift-ci bot added the qe-approved QE has approved this pull request label Oct 4, 2023

jpinsonneau added lgtm approved labels Oct 5, 2023

openshift-ci bot merged commit 390a160 into netobserv:main Oct 5, 2023
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NETOBSERV-1185 Console plugin async overview metrics #389

NETOBSERV-1185 Console plugin async overview metrics #389

jpinsonneau commented Sep 13, 2023 •

edited

Loading

codecov bot commented Sep 13, 2023 •

edited

Loading

jotak Sep 29, 2023

jpinsonneau Sep 29, 2023

jotak Sep 29, 2023

jpinsonneau Sep 29, 2023

jotak Sep 29, 2023

jotak Sep 29, 2023 •

edited

Loading

jpinsonneau Sep 29, 2023

jotak left a comment

github-actions bot commented Sep 29, 2023

jotak commented Sep 29, 2023

jpinsonneau commented Sep 29, 2023

jotak left a comment

jpinsonneau commented Sep 29, 2023

nathan-weinberg commented Oct 3, 2023

jpinsonneau commented Oct 3, 2023

nathan-weinberg commented Oct 3, 2023

github-actions bot commented Oct 3, 2023

nathan-weinberg commented Oct 3, 2023

jpinsonneau commented Oct 4, 2023

nathan-weinberg commented Oct 4, 2023

openshift-ci bot commented Oct 4, 2023

jpinsonneau commented Oct 4, 2023

nathan-weinberg commented Oct 4, 2023

github-actions bot commented Oct 4, 2023

nathan-weinberg commented Oct 4, 2023

nathan-weinberg commented Oct 4, 2023

jpinsonneau commented Oct 5, 2023

openshift-ci bot commented Oct 5, 2023

NETOBSERV-1185 Console plugin async overview metrics #389

NETOBSERV-1185 Console plugin async overview metrics #389

Conversation

jpinsonneau commented Sep 13, 2023 • edited Loading

codecov bot commented Sep 13, 2023 • edited Loading

Codecov Report

jotak Sep 29, 2023

Choose a reason for hiding this comment

jpinsonneau Sep 29, 2023

Choose a reason for hiding this comment

jotak Sep 29, 2023

Choose a reason for hiding this comment

jpinsonneau Sep 29, 2023

Choose a reason for hiding this comment

jotak Sep 29, 2023

Choose a reason for hiding this comment

jotak Sep 29, 2023 • edited Loading

Choose a reason for hiding this comment

jpinsonneau Sep 29, 2023

Choose a reason for hiding this comment

jotak left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 29, 2023

jotak commented Sep 29, 2023

jpinsonneau commented Sep 29, 2023

jotak left a comment

Choose a reason for hiding this comment

jpinsonneau commented Sep 29, 2023

nathan-weinberg commented Oct 3, 2023

jpinsonneau commented Oct 3, 2023

nathan-weinberg commented Oct 3, 2023

github-actions bot commented Oct 3, 2023

nathan-weinberg commented Oct 3, 2023

jpinsonneau commented Oct 4, 2023

nathan-weinberg commented Oct 4, 2023

openshift-ci bot commented Oct 4, 2023

jpinsonneau commented Oct 4, 2023

nathan-weinberg commented Oct 4, 2023

github-actions bot commented Oct 4, 2023

nathan-weinberg commented Oct 4, 2023

nathan-weinberg commented Oct 4, 2023

jpinsonneau commented Oct 5, 2023

openshift-ci bot commented Oct 5, 2023

jpinsonneau commented Sep 13, 2023 •

edited

Loading

codecov bot commented Sep 13, 2023 •

edited

Loading

jotak Sep 29, 2023 •

edited

Loading