NETOBSERV-1227 NETOBSERV-1388: Max / P90 / P99 graphs #412

jpinsonneau · 2023-10-12T16:11:02Z

Description

Add functions for time based metrics (DNS Latency / RTT):

min
max
P90
P99

Related changes:

refactor of overview page / graphs
new metrics on topology labels
query summary
improved testing using a list of expected queries

These changes will also fix NETOBSERV-1388

Dependencies

Based on #405
Panel ids will need update in #424

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.

codecov · 2023-10-18T15:42:56Z

Codecov Report

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

Comparison is base (988780d) 58.75% compared to head (a09bdfe) 57.89%.
Report is 2 commits behind head on main.

Files	Patch %	Lines
web/src/components/netflow-traffic.tsx	65.92%	60 Missing and 1 partial ⚠️
pkg/loki/topology_query.go	53.16%	33 Missing and 4 partials ⚠️
...components/query-summary/metrics-query-summary.tsx	57.14%	15 Missing and 9 partials ⚠️
...c/components/netflow-overview/netflow-overview.tsx	83.18%	10 Missing and 9 partials ⚠️
web/src/utils/metrics.ts	51.61%	14 Missing and 1 partial ⚠️
pkg/loki/flow_query.go	0.00%	14 Missing ⚠️
web/src/components/metrics/metrics-graph-total.tsx	65.00%	9 Missing and 5 partials ⚠️
...eb/src/components/netflow-overview/panel-kebab.tsx	65.00%	13 Missing and 1 partial ⚠️
web/src/components/metrics/metrics-donut.tsx	47.36%	7 Missing and 3 partials ⚠️
pkg/handler/validation.go	35.71%	9 Missing ⚠️
... and 14 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #412      +/-   ##
==========================================
- Coverage   58.75%   57.89%   -0.86%     
==========================================
  Files         140      168      +28     
  Lines        5976     8407    +2431     
  Branches      999     1060      +61     
==========================================
+ Hits         3511     4867    +1356     
- Misses       2303     3228     +925     
- Partials      162      312     +150

Flag	Coverage Δ
uitests	`59.22% <67.78%> (+0.47%)`	⬆️
unittests	`54.22% <45.21%> (?)`

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.

jpinsonneau · 2023-10-27T08:14:15Z

@jotak & @OlivierCazade could you please take a look at this draft so we can validate the approach ?
I would like to confirm this approach fits our needs and is not too heavy to maintain.
If you have a lighter alternative in mind please let me know 😃

Thanks !

github-actions · 2023-10-31T08:51:15Z

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

It will expire after two weeks.

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

USER=netobserv VERSION=6fc9562 make set-plugin-image

openshift-ci-robot · 2023-10-31T14:12:45Z

jotak · 2023-10-31T14:31:00Z

Hmm in fact I was not thinking something that generic, as an approach. I think we need to distinguish two kinds of metrics:

counter-based metrics, such as our bytes/packet counters, that can be summed and which we compute rates on. For these metrics, on a given interval we don't "miss" data in the visualisation, since everything gets added to the counter.
ponctual measurements like latencies, that cannot be summed <= it's on these ones only that we want stats, because in a given interval we cannot represent all the variety of what we've got (low latencies, high latencies and their distribution) so we need stats like min/max/percentiles /...

So I think we should restrict these new charts to only latencies (RTT and DNS)

jpinsonneau · 2023-11-02T12:07:50Z

I think we should restrict these new charts to only latencies (RTT and DNS)

Sure, makes sense to me. I removed the new charts from rates ones and took the opportunity to save all the metrics in a single state so it will be easier to update in the future.
ae9bcf5

github-actions · 2023-11-06T12:50:31Z

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

It will expire after two weeks.

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

USER=netobserv VERSION=154f5c8 make set-plugin-image

openshift-ci-robot · 2023-11-07T14:55:14Z

openshift-ci-robot · 2023-11-07T14:57:45Z

openshift-ci-robot · 2023-11-09T12:02:52Z

openshift-ci-robot · 2023-11-09T12:02:56Z

openshift-ci-robot · 2023-11-10T09:33:55Z

openshift-ci-robot · 2023-11-10T09:37:17Z

openshift-ci-robot · 2023-11-10T10:37:08Z

Amoghrd · 2023-11-21T20:33:34Z

/label qe-approved

openshift-ci-robot · 2023-11-21T20:33:40Z

github-actions · 2023-12-05T09:29:00Z

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

It will expire after two weeks.

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

USER=netobserv VERSION=9c09670 make set-plugin-image

jotak · 2023-12-05T11:35:20Z

In topology when setting edge labels to DNS latencies, showing avg, max or p99 works but showing min or p90 doesn't:

I don't see any error in the console...

jotak · 2023-12-05T11:48:53Z

In topology side panel, I guess we should filter-out 0ms latencies prior to any calculation:

In this screenshot, it seems clear that lack of measurements show 0ms latencies and they are pulling down the averages as 0.1ms, 0.05ms or 0.5ms whereas I believe all averages on that capture should be 1ms

(I guess the 0ms come from the normalization of the metrics, which makes more sense with rate-like metrics than latencies/histograms. Not sure what would be the side-effects if we remove this NaN-to-0 normalization for latencies - it's either this, or filtering-out 0s I guess)

Also, a UX remark on the topology side-panel: as we are adding more metrics I find it not very convenient that the "metric type" setting is shared between topology view (graph edges) and that side panel. I didn't realize immediately that to see other metrics I had to go back in the advanced /display options. We could even show a bit more than just a single selected metric type. I can create a story to suggest an enhancement here, for another release.

jotak · 2023-12-05T12:27:06Z

done with the review, I've found just 2 issues, after that it's lgtm, nice work!

jpinsonneau · 2023-12-06T11:36:07Z

Thanks for your feedback @jotak !

It seems indeed that DnsLatencyMs contains zeros which was not expected:

  "DnsErrno": 0,
  "DnsFlags": 34048,
  "DnsFlagsResponseCode": "NoError",
  "DnsId": 64587,
  "DnsLatencyMs": 0,

That's why the graph were showing zeros on bottomk min_over_time queries. We should investigate why we get these zeros. In between I've added a filter on query side to ensure we get non zero DnsLatency as:

|~`"DnsLatencyMs`!~`DnsLatencyMs%22:0[,}]`

Also skipped the normalization for dns & rtt queries and forced n/a label where values are not available.

972c7a5

Also, a UX remark on the topology side-panel: as we are adding more metrics I find it not very convenient that the "metric type" setting is shared between topology view (graph edges) and that side panel. I didn't realize immediately that to see other metrics I had to go back in the advanced /display options. We could even show a bit more than just a single selected metric type. I can create a story to suggest an enhancement here, for another release.

Could you please create a followup for this ? I'm open to any suggestions here

github-actions · 2023-12-07T09:24:47Z

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

It will expire after two weeks.

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

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

Amoghrd · 2023-12-07T16:50:13Z

@jpinsonneau I still see the hole next to donut graphs, is that going to be fixed in another PR or in this itself?

Could also confirm the DNS metrics are now seen for all edge labels

jpinsonneau · 2023-12-08T09:41:51Z

@jpinsonneau I still see the hole next to donut graphs, is that going to be fixed in another PR or in this itself?

This PR is not solving that yet. I was waiting for a decision in that thread:
https://redhat-internal.slack.com/archives/C02939DP5L5/p1701778618284139

I'm fine merging as is and doing a followup if we are not fixed yet 😉

jpinsonneau · 2023-12-08T11:59:22Z

@Amoghrd @jotak I've made the overview cards more responsive:
a09bdfe

That rely on css so every case is manage here

Amoghrd · 2023-12-08T17:30:13Z

/ok-to-test

github-actions · 2023-12-08T17:33:06Z

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

It will expire after two weeks.

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

USER=netobserv VERSION=81e04ef make set-plugin-image

jotak

/lgtm

openshift-ci · 2023-12-11T09:49:27Z

[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 Oct 12, 2023

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

jpinsonneau force-pushed the 1227 branch from 8ca2a56 to e3b2402 Compare October 18, 2023 15:38

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

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

jotak changed the title ~~NETOBSERV-1227 Max / P90 / P99 graphs~~ NETOBSERV-1227: Max / P90 / P99 graphs Oct 31, 2023

openshift-ci-robot added the jira/valid-reference label Oct 31, 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 Nov 2, 2023

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

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

jpinsonneau changed the title ~~NETOBSERV-1227: Max / P90 / P99 graphs~~ NETOBSERV-1227 NETOBSERV-1388: Max / P90 / P99 graphs Nov 7, 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 Nov 7, 2023

jpinsonneau force-pushed the 1227 branch from 49a42f0 to 42467d5 Compare November 9, 2023 10:52

openshift-merge-robot removed the needs-rebase label Nov 9, 2023

jpinsonneau force-pushed the 1227 branch from 42467d5 to fab890e Compare November 9, 2023 12:02

jpinsonneau force-pushed the 1227 branch from fab890e to 23f7a38 Compare November 10, 2023 10:49

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

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 Dec 5, 2023

addressed feedback

972c7a5

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

jpinsonneau added the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Dec 7, 2023

This was referenced Dec 7, 2023

NETOBSERV-1285 Move "query is slow" info to query summary jpinsonneau/network-observability-console-plugin#2

Closed

NETOBSERV-1285 Move "query is slow" info to query summary #438

Merged

flex donut when single

a09bdfe

github-actions bot removed the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Dec 8, 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 Dec 8, 2023

jotak approved these changes Dec 11, 2023

View reviewed changes

openshift-ci bot assigned jotak Dec 11, 2023

openshift-ci bot added the lgtm label Dec 11, 2023

jpinsonneau added the approved label Dec 11, 2023

openshift-merge-bot bot merged commit 1d3b420 into netobserv:main Dec 11, 2023
10 checks passed

NETOBSERV-1227 NETOBSERV-1388: Max / P90 / P99 graphs #412

NETOBSERV-1227 NETOBSERV-1388: Max / P90 / P99 graphs #412

Conversation

jpinsonneau commented Oct 12, 2023 • edited Loading

Description

Dependencies

Checklist

codecov bot commented Oct 18, 2023 • edited Loading

Codecov Report

jpinsonneau commented Oct 27, 2023

github-actions bot commented Oct 31, 2023

openshift-ci-robot commented Oct 31, 2023 • edited by openshift-ci bot Loading

Description

Dependencies

Checklist

jotak commented Oct 31, 2023

jpinsonneau commented Nov 2, 2023

github-actions bot commented Nov 6, 2023

openshift-ci-robot commented Nov 7, 2023 • edited by openshift-ci bot Loading

Description

Dependencies

Checklist

openshift-ci-robot commented Nov 7, 2023 • edited by openshift-ci bot Loading

Description

Dependencies

Checklist

openshift-ci-robot commented Nov 9, 2023 • edited by openshift-ci bot Loading

Description

Dependencies

Checklist

openshift-ci-robot commented Nov 9, 2023 • edited by openshift-ci bot Loading

Description

Dependencies

Checklist

openshift-ci-robot commented Nov 10, 2023 • edited by openshift-ci bot Loading

Description

Dependencies

Checklist

openshift-ci-robot commented Nov 10, 2023 • edited by openshift-ci bot Loading

Description

Dependencies

Checklist

openshift-ci-robot commented Nov 10, 2023 • edited by openshift-ci bot Loading

Description

Dependencies

Checklist

Amoghrd commented Nov 21, 2023

openshift-ci-robot commented Nov 21, 2023 • edited by openshift-ci bot Loading

Description

Dependencies

Checklist

github-actions bot commented Dec 5, 2023

jotak commented Dec 5, 2023

jotak commented Dec 5, 2023 • edited Loading

jotak commented Dec 5, 2023

jpinsonneau commented Dec 6, 2023

github-actions bot commented Dec 7, 2023

Amoghrd commented Dec 7, 2023

jpinsonneau commented Dec 8, 2023

jpinsonneau commented Dec 8, 2023

Amoghrd commented Dec 8, 2023

github-actions bot commented Dec 8, 2023

jotak left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Dec 11, 2023

jpinsonneau commented Oct 12, 2023 •

edited

Loading

codecov bot commented Oct 18, 2023 •

edited

Loading

openshift-ci-robot commented Oct 31, 2023 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Nov 7, 2023 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Nov 7, 2023 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Nov 9, 2023 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Nov 9, 2023 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Nov 10, 2023 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Nov 10, 2023 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Nov 10, 2023 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Nov 21, 2023 •

edited by openshift-ci bot

Loading

jotak commented Dec 5, 2023 •

edited

Loading