Skip to content
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

[feat] add options to aggregate ipvs collector metrics #1708

Closed
Wing924 opened this issue May 19, 2020 · 9 comments · Fixed by #1709
Closed

[feat] add options to aggregate ipvs collector metrics #1708

Wing924 opened this issue May 19, 2020 · 9 comments · Fixed by #1709

Comments

@Wing924
Copy link
Contributor

Wing924 commented May 19, 2020

Background

Currently, ipvs collector (enabled by default) collect (local_address) x (local_port) x (proto) x (remote_address) x (remote_port) x 3 metrics (ex. node_ipvs_backend_connections_active).
When the servers are used as Load Balancer or k8s nodes, the cardinality of those metrics become very high.

For example, one of my server reports 18263 metics from node exporter while normal servers report around 700-800 metrics.

Proposal

Add --collector.ipvs.sum-by=XXX option to aggregate metrics for reducing the cardinality.
For example, add --collector.ipvs.sum-by=local_address,local_port will only reports sum by (local_address,local_port)(node_ipvs_XXXX).

@discordianfish
Copy link
Member

You could achieve that with metric relabling. Not sure if we need to change this in the node-exporter. But I could be convinced. It's certainly a corner case. @SuperQ wdyt?

@Wing924
Copy link
Contributor Author

Wing924 commented May 19, 2020

@discordianfish
To be honest, I'm not sure if metric relabeling can help. How do you do metric relabeling before store those metrics into prometheus?

It's certainly a corner case.

IMO, running node exporter on every k8s nodes and kube-proxy using ipvs mode are a very common use case.

@discordianfish
Copy link
Member

To be honest, I'm not sure if metric relabeling can help. How do you do metric relabeling before store those metrics into prometheus?
Yes: https://prometheus.io/docs/prometheus/latest/configuration/configuration/#metric_relabel_configs

It's certainly a corner case.

IMO, running node exporter on every k8s nodes and kube-proxy using ipvs mode are a very common use case.

I don't disagree with that, with corner case I meant that even though it's possible to work around this with metric relabling, we might consider helping with this on the exporter side.

@SuperQ
Copy link
Member

SuperQ commented May 19, 2020

I don't think this can be done with relabeling, as you can't sum() up values.

I don't know enough about the data and interaction. But I also think this might go badly. This would be fine for gauge values. But there are a number of counters like node_ipvs_connections_total. Summing them on the exporter side is invalid because of the need to rate before sum.

@Wing924
Copy link
Contributor Author

Wing924 commented May 19, 2020

Summing them on the exporter side is invalid because of the need to rate before sum.

Please correct me if I'm wrong.
The sum after rate works if it only aggregate on metrics belonging to the same instance because the counter won't reset partially.

@SuperQ
Copy link
Member

SuperQ commented May 19, 2020

Looking at the code more closely, there are only labels for the backend metrics, which are only gauges.

Adding this aggregation is possible.

@SuperQ
Copy link
Member

SuperQ commented May 19, 2020

"sum after rate" is always possible. The problem is "rate after sum". If there were per backend counters, and the list of backends changed on one node, it would become invalid to sum them.

@Wing924
Copy link
Contributor Author

Wing924 commented May 19, 2020

@SuperQ Do you mind if I send PR for this issue?

@SuperQ
Copy link
Member

SuperQ commented May 19, 2020

Go ahead, Looking at the code a bit, I'm wondering if the way to handle "which things to sum by" would be better done with a flag like:

--collector.ipvs.labels=local_address,local_port,remote_port,proto,local_mark

This way it's easier for the user to see exactly what labels are going to be kept in the output.

Wing924 added a commit to Wing924/node_exporter that referenced this issue May 20, 2020
oblitorum pushed a commit to shatteredsilicon/node_exporter that referenced this issue Apr 9, 2024
oblitorum pushed a commit to shatteredsilicon/node_exporter that referenced this issue Apr 9, 2024
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 a pull request may close this issue.

3 participants