Skip to content

Add UDP drops collector #3224

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nicolas-laduguie
Copy link

No description provided.

Signed-off-by: Nicolas Laduguie <[email protected]>
@cleeland
Copy link
Contributor

cleeland commented Jan 8, 2025

IMHO, this should be named udp_stats and should serve as an umbrella under which to expose all relevant UDP-related stats--including those exposed by udp_queues. By doing that, you don't create a breaking change, but you do offer a place to extend udp-related stats exposition in the future, e.g., retransmits, without having to create yet another specific collector.

At some point you could deprecate udp_queues.

@nicolas-laduguie
Copy link
Author

Thanks for the feedback @cleeland .
Just to make sure, are you talking about the collector name?
If so, the problem with this approach is the metrics naming then.
A the moment we have node_udp_queues metric which is served by udp_queues collector.
If we create a new collector called udp_stats, the metrics of that collector should be named differently than the ones of udp_queues and other collectors.
I feel the current name node_udp_queues is accurate and adding one more metric with less accurate name but with same value and same origin would be confusing IMHO.
Even if the amount of collector is significant, it allows very granular activation/deactivation of metric by the collectors.

@cleeland
Copy link
Contributor

cleeland commented Jan 8, 2025

are you talking about the collector name?

Yes.

If we create a new collector called udp_stats, the metrics of that collector should be named differently than the ones of udp_queues and other collectors.

The way I was thinking out it, nothing would change under udp_queues. A new collector, udp_stats would look much like what I did in the original PR, except that the collector name would be udp_stats rather than simply udp. Thus, the metrics it would expose would be:

  • node_udp_stats_queues {version, direction}
  • node_udp_stats_drops { version }
  • etc

As a user, I think it makes more sense for node_exporter to follow the structure of the kernel's stats as much as possible. It makes it easier for me to find things and make educated guesses at where to look.

@discordianfish
Copy link
Member

The metric name doesn't have to match the collector name, so I'm leaning towards calling the collector udp_stats or just udp but leave name as here (node_udp_drops_total). @SuperQ wdyt?

@SuperQ
Copy link
Member

SuperQ commented Feb 15, 2025

I think we should create a new udp collector that includes the udp_queues collector. We could call it udp_stats, but simply udp is just fine.

I'm thinking about releasing one or maybe two more minor releases before 2.0. So we can deal with the deprecation quickly.

It's also possible that we can set this up in a backwards compatible way.

  • Flip udp_queues to defaultDisabled.
  • Add udp as defaultEnabled.
  • If users explicitly enable --collector.udp_queues without --no-collector.udp or --collector.disable-defaults, we simply ignore flag and print a deprecation warning.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants