NETOBSERV-1533: refine metrics for dashboard creation #281

jotak · 2024-02-27T15:21:01Z

Use just two shared metrics for eviction counters: on for eviction events and one for flows; shared across ringbuf/map implementations and labelled as such
Report more errors via metrics

- Use just two shared metrics for eviction counters: on for eviction events and one for flows; shared across ringbuf/map implementations and labelled as such - Report more errors via metrics

openshift-ci-robot · 2024-02-27T15:21:05Z

@jotak: This pull request references NETOBSERV-1533 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.16.0" version, but no target version was set.

In response to this:

Use just two shared metrics for eviction counters: on for eviction events and one for flows; shared across ringbuf/map implementations and labelled as such

Report more errors via metrics

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

jotak · 2024-02-27T15:24:18Z

@msherif1234 as I'm working on the dashboard and I'm bringing a few changes here.
Here's a screenshot of what I get with these metrics:

Some text counters in the top bar:

Some ebpf stats:

(something looks wrong in the eviction rate when it evicts because of full map)

codecov · 2024-02-27T15:26:02Z

Codecov Report

Attention: Patch coverage is 69.10569% with 38 lines in your changes are missing coverage. Please review.

Project coverage is 36.26%. Comparing base (b3c02b8) to head (4a58e68).
Report is 2 commits behind head on main.

Files	Patch %	Lines
pkg/agent/agent.go	35.48%	20 Missing ⚠️
pkg/ebpf/tracer.go	0.00%	9 Missing ⚠️
pkg/flow/tracer_ringbuf.go	50.00%	5 Missing ⚠️
pkg/exporter/kafka_proto.go	50.00%	2 Missing ⚠️
pkg/exporter/grpc_proto.go	90.90%	1 Missing ⚠️
pkg/flow/account.go	93.33%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #281      +/-   ##
==========================================
+ Coverage   36.08%   36.26%   +0.18%     
==========================================
  Files          42       42              
  Lines        3786     3794       +8     
==========================================
+ Hits         1366     1376      +10     
+ Misses       2342     2340       -2     
  Partials       78       78

Flag	Coverage Δ
unittests	`36.26% <69.10%> (+0.18%)`	⬆️

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.

jotak · 2024-02-27T15:26:02Z

pkg/metrics/metrics.go

+	// MapSize = defineMetric(
+	// 	"ebpf_map_size",
+	// 	"size of the eBPF maps",
+	// 	TypeGauge,
+	// )


I tried to create a gauge to track the hashmap size but I didn't find a function to get the used size in bpflib...

we can do dummy iterate like the one we use today and inside increment the map size ?

i'd prefer to avoid any additional processing .. no worries I'll delete this commented code

jotak · 2024-02-27T16:54:26Z

pkg/flow/tracer_map.go

 	m.evictionCond.Broadcast()
+	m.evictionCounter.ForSourceAndReason("hashmap", reason).Inc()


@msherif1234 I think what I did here doesn't work: the eviction counter is incremented everytime Flush is called, but it seems to be called many times before sync.Condition makes it go to the actual evict. So I end up with a counter value much bigger than it should. I'm not sure if this is expected that Flush is called so often without having an actual eviction. But to "fix" this counter I would need to move the increment back where you set it before, in evictFlows, but the downside is that we loose then the reason label that I wanted to have

Look at this: there's a peak to 300K evictions / second due to this increase, while the flows per second is only at 3.5K. Isn't it fishy?

ok I understand. In tracer_map I'm seeing not flows but individual packets. So a different metrics should be used for that. I'll do it surely tomorrow.
Also, I fixed a bug in my last commit, not all metrics were correctly showing up, now it looks like this:

- Use a single metrics object that holds shared metrics - Create shared metrics at startup

- accounter and deduper buffer gauges - use eviction metrics for exporters - eviction from deduper - limiter drops counter

msherif1234 · 2024-02-28T18:50:20Z

/lgtm

github-actions · 2024-02-29T08:26:25Z

New image:
quay.io/netobserv/netobserv-ebpf-agent:12b1753

It will expire after two weeks.

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

USER=netobserv VERSION=12b1753 make set-agent-image

jotak · 2024-03-04T09:00:10Z

@msherif1234 I think this task is done I've got all metrics needed to build a nice dashboard - can you take a second look & review please?

github-actions · 2024-03-04T09:02:15Z

New image:
quay.io/netobserv/netobserv-ebpf-agent:26076c3

It will expire after two weeks.

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

USER=netobserv VERSION=26076c3 make set-agent-image

msherif1234 · 2024-03-04T12:35:19Z

pkg/ebpf/tracer.go

 		}
 		// We observed that eBFP PerCPU map might insert multiple times the same key in the map
 		// (probably due to race conditions) so we need to re-join metrics again at userspace
 		// TODO: instrument how many times the keys are is repeated in the same eviction
 		flows[id] = append(flows[id], metrics...)
 	}
+	met.BufferSizeGauge.WithBufferName("hashmap-total").Set(float64(count))
+	met.BufferSizeGauge.WithBufferName("hashmap-unique").Set(float64(len(flows)))


aren't those counters are dup to what we we already have in tracer_map ?

it's different , the counters are used to compute rates, like number of flows evicted per second, but this allows to get the size of the maps. In fact (to answer your next question) they have been incredibly useful to me as it's thanks to these gauges that I understood exactly what was going wrong here with LookupAndDelete: the "hashmap-total" was growing to 100K elements ie. the full map size even when it was not supposed to be full, while the "hashmap-unique" gauge was much smaller, something like 2K. So that's what led me to find the issue about deleting within the iteration.

msherif1234 · 2024-03-04T12:37:32Z

pkg/flow/deduper.go

 			}
+			m.BufferSizeGauge.WithBufferName("deduper-list").Set(float64(cache.entries.Len()))
+			m.BufferSizeGauge.WithBufferName("deduper-map").Set(float64(len(cache.ifaces)))


do we really need metrics for this isn't debugging enough ?

msherif1234 · 2024-03-04T12:39:44Z

pkg/metrics/metrics.go

-		"Sampling rate seconds",
+	samplingRate = defineMetric(
+		"sampling_rate",
+		"Sampling rate",


adding the unis was in the metrics guide line u shared with me ?

but this one is the sampling rate (ratio) like 1:50 or 1:1 it's not measuring time

msherif1234 · 2024-03-04T12:41:05Z

overall looks great to me small comments/questions inline up to you to address nothing critical
/lgtm

jotak · 2024-03-04T14:56:09Z

/approve

openshift-ci · 2024-03-04T14:56:15Z

[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

NETOBSERV-1533: refine metrics for dashboard creation

424b5a1

- Use just two shared metrics for eviction counters: on for eviction events and one for flows; shared across ringbuf/map implementations and labelled as such - Report more errors via metrics

jotak requested a review from msherif1234 February 27, 2024 15:21

openshift-ci-robot added the jira/valid-reference label Feb 27, 2024

jotak commented Feb 27, 2024

View reviewed changes

jotak mentioned this pull request Feb 27, 2024

NETOBSERV-1533: add eBPF metrics to health dashboard - improve design netobserv/network-observability-operator#583

Merged

10 tasks

jotak commented Feb 27, 2024

View reviewed changes

jotak added 2 commits February 27, 2024 19:41

Fix shared metrics

d2a8e47

- Use a single metrics object that holds shared metrics - Create shared metrics at startup

Add more metrics:

ef67fd0

- accounter and deduper buffer gauges - use eviction metrics for exporters - eviction from deduper - limiter drops counter

openshift-ci bot assigned msherif1234 Feb 28, 2024

openshift-ci bot added the lgtm label Feb 28, 2024

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

Use a different metric for ringbuf eviction counter

63fb0b5

openshift-ci bot removed the lgtm label Feb 29, 2024

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

Add map size gauges

4a58e68

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

msherif1234 reviewed Mar 4, 2024

View reviewed changes

openshift-ci bot added the lgtm label Mar 4, 2024

openshift-ci bot added the approved label Mar 4, 2024

openshift-merge-bot bot merged commit 1d85464 into netobserv:main Mar 4, 2024
10 checks passed

jotak deleted the refine-metrics branch March 21, 2024 07:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NETOBSERV-1533: refine metrics for dashboard creation #281

NETOBSERV-1533: refine metrics for dashboard creation #281

jotak commented Feb 27, 2024

openshift-ci-robot commented Feb 27, 2024 •

edited by openshift-ci bot

Loading

jotak commented Feb 27, 2024 •

edited

Loading

codecov bot commented Feb 27, 2024 •

edited

Loading

jotak Feb 27, 2024

msherif1234 Feb 27, 2024

jotak Feb 27, 2024

msherif1234 Feb 27, 2024

jotak Feb 27, 2024 •

edited

Loading

jotak Feb 27, 2024

jotak Feb 27, 2024

msherif1234 commented Feb 28, 2024

github-actions bot commented Feb 29, 2024

jotak commented Mar 4, 2024

github-actions bot commented Mar 4, 2024

msherif1234 Mar 4, 2024

jotak Mar 4, 2024

msherif1234 Mar 4, 2024

msherif1234 Mar 4, 2024

jotak Mar 4, 2024

msherif1234 commented Mar 4, 2024

jotak commented Mar 4, 2024

openshift-ci bot commented Mar 4, 2024

		m.evictionCond.Broadcast()
		m.evictionCounter.ForSourceAndReason("hashmap", reason).Inc()

NETOBSERV-1533: refine metrics for dashboard creation #281

NETOBSERV-1533: refine metrics for dashboard creation #281

Conversation

jotak commented Feb 27, 2024

openshift-ci-robot commented Feb 27, 2024 • edited by openshift-ci bot Loading

jotak commented Feb 27, 2024 • edited Loading

codecov bot commented Feb 27, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jotak Feb 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msherif1234 commented Feb 28, 2024

github-actions bot commented Feb 29, 2024

jotak commented Mar 4, 2024

github-actions bot commented Mar 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msherif1234 commented Mar 4, 2024

jotak commented Mar 4, 2024

openshift-ci bot commented Mar 4, 2024

openshift-ci-robot commented Feb 27, 2024 •

edited by openshift-ci bot

Loading

jotak commented Feb 27, 2024 •

edited

Loading

codecov bot commented Feb 27, 2024 •

edited

Loading

jotak Feb 27, 2024 •

edited

Loading