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

Add encode implementations for SocketAddr and IpAddr #87

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

rnijveld
Copy link

@rnijveld rnijveld commented Sep 8, 2022

Something I encounter quite often is having metrics based on connection information, so I frequently find myself wanting to use either a SocketAddr or IpAddr in a label.

Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general I am not opposed to this change. Though I would guess that in most cases, this is not what one wants. In case the number of unique IP addresses is large, this can easily lead to a cardinality explosion.

https://www.robustperception.io/cardinality-is-key/

What do you think of adding a warning doc comment to the implementations to make sure users are aware of what they are doing?

Also would you mind resolving the merge conflicts?

@mxinden
Copy link
Member

mxinden commented Oct 2, 2022

Friendly ping @rnijveld.

@rnijveld
Copy link
Author

@mxinden Sorry about the slow response, life got in the way a little. I definitely agree with not wanting to use this for high cardinality situations. Although of course such cases could happen just as easily with other label types, I do agree that IP addresses might more quickly result in a cardinality explosion when not used correctly. I've added a little warning text to each of the implementations, I hope that resolves your concerns, let me know if there's anything else I can do!

Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay here.

Thanks for adding the warning. One confusion on my end.

Comment on lines +211 to +212
/// distinct values is low (i.e. low cardinality). In all other cases you should
/// combine your metrics into a single metric instead. Especially bad examples
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In all other cases you should combine your metrics into a single metric instead.

I don't understand this. How is combining multiple metrics into one solving the issue of a cardinality explosion?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, as in dropping the label?

Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Friendly ping @rnijveld. I would like to get this merged into master.

Comment on lines +211 to +212
/// distinct values is low (i.e. low cardinality). In all other cases you should
/// combine your metrics into a single metric instead. Especially bad examples
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, as in dropping the label?

@08d2
Copy link

08d2 commented Jan 10, 2023

It is effectively never appropriate to use an address as a label, for reasons of cardinality already discussed.

@mxinden
Copy link
Member

mxinden commented Jan 10, 2023

I don't think "never" is correct. One use-case I can come up with is exposing ones listening address as an info metric. See e.g. kubernetes/kube-state-metrics#498 as an example on how far one can push cardinality in Prometheus with info metrics.

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.

3 participants