udp_queues_linux.go: Expose UDP drops via gauge analogous to queue sizes #2993

cleeland · 2024-04-18T20:44:34Z

Since a common reason for monitoring UDP queue sizes can be in teasing out performance and QoS issues, it would be convenient to have drops available in the same frames of data. Since the underlying source of the data provides that information, this PR exposes drops modeled as a gauge just as queue sizes are.

@discordianfish @SuperQ

cleeland · 2024-04-19T20:08:11Z

@discordianfish @SuperQ I'm assuming that the failing circleci tests are because I need to update the fixtures (via make update_fixtures)? I'm having problems doing that running natively on macos. Is updating fixtures something that only reliably works on linux? If so, I'll make that happen in docker or a VM.

cleeland · 2024-04-22T17:16:59Z

I'm not sure why test_docker is failing, and I can't pull the base image to run the test manually. It complains that I am unauthorized. From what I can tell, quay.io does not have a no-cost tier.

SuperQ

Thanks!

SuperQ

Wait, no, this seems incorrect. Drops appears to be a counter, so this needs to be a new metric, not a label on an existing metric.

cleeland · 2024-04-22T20:27:27Z

Wait, no, this seems incorrect. Drops appears to be a counter, so this needs to be a new metric, not a label on an existing metric.

Drat; you're right. Let me see if I can fix this quickly.

nicolas-laduguie · 2025-01-06T09:18:26Z

Hello,
Is there any progress on this work ?
We would also need this new metric being exposed by the node exporter :)

…zes. Signed-off-by: Chris Cleeland <[email protected]>

Update e2e test output to include the drops. Signed-off-by: Chris Cleeland <[email protected]>

Don't use a pointer without checking it first. Signed-off-by: Chris Cleeland <[email protected]>

I think it makes sense for "drops" to sit alongside the queue length gauges in prom stats because they are all neighbors in procfs (source of these stats). Moreover, in reading the commit log message for the original creating work for udp_queues, I think there may have been some misreading or confusion between the word "state" and the common short-form of "stats" to mean "statistics". The original author "chose the name 'udp_queue' instead of 'udpstat' as UDP has no state"; I believe that 'udpstat' might actually be the more appropriate name. Signed-off-by: Chris Cleeland <[email protected]>

collector/fixtures/e2e-64k-page-output.txt

collector/fixtures/e2e-output.txt

Signed-off-by: Chris Cleeland <[email protected]>

cleeland · 2025-01-06T19:10:15Z

What I thought would be a couple of easy commits/pushes is turning into a bunch of work (maybe because I'm running on macos and not on linux?) In any case, I have to set this aside for awhile.

nicolas-laduguie · 2025-01-06T19:42:15Z

What I thought would be a couple of easy commits/pushes is turning into a bunch of work (maybe because I'm running on macos and not on linux?) In any case, I have to set this aside for awhile.

@cleeland can you summarize what you tried and what was the issue?
Would be helpful if someone can help you or if I open myself a pr to get that new metric in.

cleeland · 2025-01-06T20:54:34Z

What I thought would be a couple of easy commits/pushes is turning into a bunch of work (maybe because I'm running on macos and not on linux?) In any case, I have to set this aside for awhile.

@cleeland can you summarize what you tried and what was the issue? Would be helpful if someone can help you or if I open myself a pr to get that new metric in.

The biggest impediment is that I can't run the makefiles on ventura 13.7.1. I thought I might still have a dockerfile or lima setup from when I initially did this PR, but I've not found it. I think once I get a chance to spin up lima with proper dev environment I'll be able to make progress.

cleeland · 2025-01-07T00:44:30Z

@nicolas-laduguie I think I finally understand the issue I'm having: changing drops from a gauge to a counter requires that drops no longer be a label under udp_queues, and instead it has to be its own metric, e.g.,
node_udp_queues_drops_total
with labels of either v4 or v6.

To do this I think I'll have to change the creation in NewUDPqueuesCollector() to create a separate metric? This is an area I don't quite understand, so I'll likely get it wrong a bunch of times before I get it right. Happy to accept a little coaching.

I was able to use lima to set up two linux machines (amd64 and arm64) to use as runtime environments.

Signed-off-by: Chris Cleeland <[email protected]>

This separates the gauges related to udp_queues and the counter related to the total number of drops, but keeps them under the same namespace. Signed-off-by: Chris Cleeland <[email protected]>

cleeland · 2025-01-07T05:25:51Z

@nicolas-laduguie I got something that achieves my goal, though I'm not sure if it's necessarily the best choice. Feel free to critique and/or modify. Most importantly, things that are gauge-ish are gauges, and things that are counter-ish are counters, so @SuperQ 's original issue is addressed.

discordianfish · 2025-01-07T12:25:17Z

README.md

@@ -158,7 +158,7 @@ thermal | Exposes thermal statistics like `pmset -g therm`. | Darwin
 thermal\_zone | Exposes thermal zone & cooling device statistics from `/sys/class/thermal`. | Linux
 time | Exposes the current system time. | _any_
 timex | Exposes selected adjtimex(2) system call stats. | Linux
-udp_queues | Exposes UDP total lengths of the rx_queue and tx_queue from `/proc/net/udp` and `/proc/net/udp6`. | Linux
+udp | Exposes UDP statistics from `/proc/net/udp` and `/proc/net/udp6`. | Linux


Guess it makes sense to rename this but:

We should rename the file as well

Technically its a breaking change, e.g people might enable/disable the collector explicitly so when upgrading it will refused to start

@SuperQ wdyt? maybe we should give registerCollector a list of aliases and have it print a deprecation warning if any of them are used?

Or to just get this merged, maybe just not rename it for now..

Yeah, I figured this wasn't an ideal solution, but I recognize that I'm too ignorant to know what sorts of API-ish guarantees are in place. I opted for "make sure that the metric names stay the same", although since I changed the namespace from "udp_queues" to "udp" it's possible I didn't even achieve that.

FWIW, I chose this after looking at some of the other metrics, and settled on something similar to the pattern used by cpu.

Hi guys, I contributed this PR based on your work @cleeland .
As @discordianfish said, the existing exporter name udp_queues might be already used by people disabling that collector.
So it's worth to not change it otherwise it would be a breaking change and would need to wait a major release from node exporter.
If it's fine for you, let's work in my PR?

@nicolas-laduguie thanks! I left my thoughts over on the new PR. Thanks for continuing to help shepherd this.

cleeland force-pushed the expose-udp-drops branch 2 times, most recently from 084bd27 to b297b22 Compare April 19, 2024 16:45

cleeland force-pushed the expose-udp-drops branch from b297b22 to 271bcee Compare April 22, 2024 14:44

SuperQ approved these changes Apr 22, 2024

View reviewed changes

SuperQ requested changes Apr 22, 2024

View reviewed changes

cleeland force-pushed the expose-udp-drops branch 2 times, most recently from ecf14b8 to 7d1e8c6 Compare January 6, 2025 15:55

cleeland and others added 4 commits January 6, 2025 09:58

udp_queues_linux.go: Expose UDP drops via gauge analogous to queue si…

392f9ac

…zes. Signed-off-by: Chris Cleeland <[email protected]>

Fix failing e2e test.

619f013

Update e2e test output to include the drops. Signed-off-by: Chris Cleeland <[email protected]>

Fix failing docker test.

5a060c2

Don't use a pointer without checking it first. Signed-off-by: Chris Cleeland <[email protected]>

cleeland force-pushed the expose-udp-drops branch from 7d1e8c6 to 1a67c62 Compare January 6, 2025 15:58

nicolas-laduguie reviewed Jan 6, 2025

View reviewed changes

collector/fixtures/e2e-64k-page-output.txt Show resolved Hide resolved

collector/fixtures/e2e-output.txt Show resolved Hide resolved

Update e2e test expectations.

146d5e8

Signed-off-by: Chris Cleeland <[email protected]>

cleeland force-pushed the expose-udp-drops branch from 9b5b3ce to 146d5e8 Compare January 6, 2025 18:41

cleeland force-pushed the expose-udp-drops branch from 1c9044e to e7b372f Compare January 6, 2025 20:54

cleeland added 2 commits January 6, 2025 23:08

Fix yamllint error due to trailing whitespace.

62b30d6

Signed-off-by: Chris Cleeland <[email protected]>

Break udp_queues into udp_{queues,drops_total}

13f019f

This separates the gauges related to udp_queues and the counter related to the total number of drops, but keeps them under the same namespace. Signed-off-by: Chris Cleeland <[email protected]>

cleeland force-pushed the expose-udp-drops branch from e7b372f to 13f019f Compare January 7, 2025 05:16

discordianfish reviewed Jan 7, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

udp_queues_linux.go: Expose UDP drops via gauge analogous to queue sizes #2993

udp_queues_linux.go: Expose UDP drops via gauge analogous to queue sizes #2993

cleeland commented Apr 18, 2024

cleeland commented Apr 19, 2024

cleeland commented Apr 22, 2024

SuperQ left a comment

SuperQ left a comment

cleeland commented Apr 22, 2024

nicolas-laduguie commented Jan 6, 2025

cleeland commented Jan 6, 2025

nicolas-laduguie commented Jan 6, 2025

cleeland commented Jan 6, 2025

cleeland commented Jan 7, 2025

cleeland commented Jan 7, 2025

discordianfish Jan 7, 2025

cleeland Jan 7, 2025

nicolas-laduguie Jan 8, 2025

cleeland Jan 8, 2025

udp_queues_linux.go: Expose UDP drops via gauge analogous to queue sizes #2993

Are you sure you want to change the base?

udp_queues_linux.go: Expose UDP drops via gauge analogous to queue sizes #2993

Conversation

cleeland commented Apr 18, 2024

cleeland commented Apr 19, 2024

cleeland commented Apr 22, 2024

SuperQ left a comment

Choose a reason for hiding this comment

SuperQ left a comment

Choose a reason for hiding this comment

cleeland commented Apr 22, 2024

nicolas-laduguie commented Jan 6, 2025

cleeland commented Jan 6, 2025

nicolas-laduguie commented Jan 6, 2025

cleeland commented Jan 6, 2025

cleeland commented Jan 7, 2025

cleeland commented Jan 7, 2025

discordianfish Jan 7, 2025

Choose a reason for hiding this comment

cleeland Jan 7, 2025

Choose a reason for hiding this comment

nicolas-laduguie Jan 8, 2025

Choose a reason for hiding this comment

cleeland Jan 8, 2025

Choose a reason for hiding this comment