metrics: reduce cardinality of nodeconnectivity metrics #33103

jshr-w · 2024-06-12T19:01:09Z

This PR deprecates the existing metrics node_connectivity_latency_seconds and node_connectivity_status, intending to eventually replace them with lower-cardinality metrics node_health_connectivity_latency_seconds and node_health_connectivity_status.

Metric Description:

node_health_connectivity_latency_seconds is a histogram metric which records all valid latencies observed by the cluster
node_health_connectivity_status is a gauge metric that returns the number of nodes/endpoints in each status type.

Explanation of Lower Cardinality:
Prior to this, the original node connectivity metrics were exported with source node and target node information, with an O(n^2) cardinality. In the updated versions, labels related to target cluster & node are removed to provide a higher-level overview of latency/status information and improve the scalability of this metric.

The metrics use only the labels:

source node name
source cluster
type
status (for node_connectivity_status)

Other changes:

The corresponding output for cilium-health status --succinct has also been modified to represent the # of reachable endpoints / # of total endpoints.

Relates to issue #32820.

Example of metrics, single cluster with 2 nodes.

On start-up:

After deleting one node:

Example of metrics seen on a node in a clustermesh with 2 clusters, each with two nodes.

On start-up, before all endpoints come up:

Once all endpoints are ready:

After deleting one cluster:

`cilium-health status --succinct` output

Please ensure your pull request adheres to the following guidelines:

For first time contributors, read Submitting a pull request
All code is covered by unit and/or runtime tests where feasible.
All commits contain a well written commit description including a title,
description and a Fixes: #XXX line if the commit addresses a particular
GitHub issue.
If your commit description contains a Fixes: <commit-id> tag, then
please add the commit author[s] as reviewer[s] to this issue.
All commits are signed off. See the section Developer’s Certificate of Origin
Provide a title or release-note blurb suitable for the release notes.
Are you a user of Cilium? Please add yourself to the Users doc
Thanks for contributing!

metrics: deprecate node_connectivity metrics in favor of new lower-cardinality node_health_connectivity metrics

pkg/health/server/server_test.go

pkg/health/server/server.go

qmonnet

Doc change looks good; I only glanced quickly over the rest.

I'd recommend adding the motivation for the change to your commit description and PR description. It's not clear why we reduce the cardinality when reading the commit log; we have to go and check the related issue. I'd rather have the motivation for the patch clearly mentioned here.

marseel

Looks great, only small nits :)

pkg/health/client/client.go

pkg/health/server/server.go

jshr-w · 2024-08-22T02:17:54Z

Force push for constant timeout diff:

diff --git a/pkg/health/server/server.go b/pkg/health/server/server.go
index b4764fca53..595bc6a79d 100644
--- a/pkg/health/server/server.go
+++ b/pkg/health/server/server.go
`@@ -70,6 +70,10 @@ type Server struct {
        localStatus  *healthModels.SelfStatus
 }
 
+const (
+       probeTimeout = 60
+)
+
 // DumpUptime returns the time that this server has been running.
 func (s *Server) DumpUptime() string {
        return time.Since(s.startTime).String()
@@ -317,7 +321,7 @@ func collectConnectivityMetric(status *healthModels.ConnectivityStatus, labels .
                        metrics.NodeConnectivityLatency.WithLabelValues(labels...).Set(metricValue)
                        metrics.NodeHealthConnectivityLatency.WithLabelValues(healthLabels...).Observe(metricValue)
                } else {
-                       metrics.NodeHealthConnectivityLatency.WithLabelValues(healthLabels...).Observe(60)
+                       metrics.NodeHealthConnectivityLatency.WithLabelValues(healthLabels...).Observe(probeTimeout)
                }
        } else {
                metrics.NodeConnectivityLatency.WithLabelValues(labels...).Set(-1)

pchaigno · 2024-08-22T07:39:49Z

/test

marseel

One small comment, expect for that lgtm 🎉

pkg/health/server/server.go

pkg/metrics/registry.go

jshr-w · 2024-08-23T03:05:03Z

/test

marseel

one hopefully last blocking comment, I must have missed that on the previous review (sorry!)

pkg/health/server/server.go

jshr-w · 2024-08-23T22:19:40Z

/test

jshr-w · 2024-08-24T23:29:50Z

/test

jshr-w · 2024-08-25T23:44:05Z

/test

tommyp1ckles

looks good, thanks for the changes 🙏

marseel

Looks great, thanks!

This PR deprecates the existing metrics `node_connectivity_latency_seconds` and `node_connectivity_status`, intending to eventually replace them with lower-cardinality metrics `node_health_connectivity_latency_seconds` and `node_health_connectivity_status`. Metric Description: `node_health_connectivity_latency_seconds` is a histogram metric which records all valid latencies observed by the cluster `node_health_connectivity_status` is a gauge metric that returns the number of nodes/endpoints in each status type. Explanation of Lower Cardinality: Prior to this, the original node connectivity metrics were exported with source node and target node information, with an O(n^2) cardinality. In the updated versions, labels related to target cluster & node are removed to provide a higher-level overview of latency/status information and improve the scalability of this metric. The metrics use only the labels: - source node name - source cluster - type - status (for node_connectivity_status) Other changes: The corresponding output for `cilium-health status --succinct` has also been modified to represent the # of reachable endpoints / # of total endpoints. Signed-off-by: jshr-w <shjayaraman@microsoft.com>

jshr-w · 2024-08-26T14:32:25Z

/test

maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jun 12, 2024

github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Jun 12, 2024

jshr-w force-pushed the jshr/healthmetriccardinality branch from 0e20555 to 3a934d5 Compare June 12, 2024 19:04

jshr-w commented Jun 12, 2024

View reviewed changes

pkg/health/server/server_test.go Outdated Show resolved Hide resolved

marseel reviewed Jun 13, 2024

View reviewed changes

pkg/health/server/server.go Show resolved Hide resolved

jshr-w force-pushed the jshr/healthmetriccardinality branch 4 times, most recently from aceafc2 to cfb6d8f Compare June 20, 2024 01:56

jshr-w marked this pull request as ready for review June 20, 2024 02:01

jshr-w requested review from a team as code owners June 20, 2024 02:01

jshr-w requested review from tommyp1ckles, doniacld, qmonnet and marseel June 20, 2024 02:01

qmonnet added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Jun 20, 2024

maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jun 20, 2024

qmonnet approved these changes Jun 20, 2024

View reviewed changes

This comment was marked as outdated.

Sign in to view

jshr-w force-pushed the jshr/healthmetriccardinality branch 2 times, most recently from b8f1baa to 5f549a9 Compare June 20, 2024 19:36

qmonnet added the dont-merge/wait-until-release Freeze window for current release is blocking non-bugfix PRs label Jun 20, 2024

This comment was marked as outdated.

Sign in to view

marseel requested changes Jun 24, 2024

View reviewed changes

pkg/health/client/client.go Outdated Show resolved Hide resolved

pkg/health/client/client.go Show resolved Hide resolved

pkg/health/server/server.go Outdated Show resolved Hide resolved

jshr-w force-pushed the jshr/healthmetriccardinality branch 2 times, most recently from 38b29b1 to d81bff4 Compare June 26, 2024 15:45

jshr-w requested a review from marseel June 26, 2024 15:47

marseel requested changes Aug 22, 2024

View reviewed changes

pkg/health/server/server.go Outdated Show resolved Hide resolved

pkg/metrics/registry.go Outdated Show resolved Hide resolved

This comment was marked as resolved.

Sign in to view

maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Aug 23, 2024

jshr-w force-pushed the jshr/healthmetriccardinality branch from 86d2e79 to 6be5857 Compare August 23, 2024 03:00

This comment was marked as resolved.

Sign in to view

jshr-w force-pushed the jshr/healthmetriccardinality branch from 6be5857 to 72ff857 Compare August 23, 2024 03:02

maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Aug 23, 2024

jshr-w requested a review from marseel August 23, 2024 03:02

marseel requested changes Aug 23, 2024

View reviewed changes

pkg/health/server/server.go Show resolved Hide resolved

jshr-w force-pushed the jshr/healthmetriccardinality branch 2 times, most recently from 4263608 to cfb1970 Compare August 23, 2024 22:19

jshr-w force-pushed the jshr/healthmetriccardinality branch from ef1008d to 5dd290a Compare August 24, 2024 23:29

tommyp1ckles approved these changes Aug 26, 2024

View reviewed changes

marseel approved these changes Aug 26, 2024

View reviewed changes

jshr-w force-pushed the jshr/healthmetriccardinality branch from 5dd290a to aa13a7b Compare August 26, 2024 14:29

pchaigno enabled auto-merge August 26, 2024 18:05

pchaigno added this pull request to the merge queue Aug 26, 2024

maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Aug 26, 2024

Merged via the queue into cilium:main with commit fee6107 Aug 26, 2024
68 checks passed

christarazi added the affects/v1.16 This issue affects v1.16 branch label Jan 27, 2025

jshr-w mentioned this pull request May 23, 2025

pkg/health: remove deprecated health metrics #39694

Merged

metrics: reduce cardinality of nodeconnectivity metrics #33103

metrics: reduce cardinality of nodeconnectivity metrics #33103

Uh oh!

Conversation

jshr-w commented Jun 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

qmonnet left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

marseel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jshr-w commented Aug 22, 2024

Uh oh!

pchaigno commented Aug 22, 2024

Uh oh!

marseel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

This comment was marked as resolved.

This comment was marked as resolved.

jshr-w commented Aug 23, 2024

Uh oh!

marseel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jshr-w commented Aug 23, 2024

Uh oh!

jshr-w commented Aug 24, 2024

Uh oh!

jshr-w commented Aug 25, 2024

Uh oh!

tommyp1ckles left a comment

Choose a reason for hiding this comment

Uh oh!

marseel left a comment

Choose a reason for hiding this comment

Uh oh!

jshr-w commented Aug 26, 2024

Uh oh!

Uh oh!

Uh oh!

jshr-w commented Jun 12, 2024 •

edited

Loading