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

Remove remote_port label from flow_traffic metrics #95

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

Conversation

slrtbtfs
Copy link

Fixes #94.

However it theoretically has the potential to break some existing usecases, where the
distiction between remote ports is relevant.

Signed-off-by: Tobias Guggenmos tobias.guggenmos@uni-ulm.de

Fixes cloudflare#94.

However it theoretically has the potential to break some existing usecases, where the
distiction between remote ports is relevant.

Signed-off-by: Tobias Guggenmos <tobias.guggenmos@uni-ulm.de>
@debugloop
Copy link
Contributor

I was a bit alarmed by this, but the IOS-XR flow export we use does not randomize the source port regularly, may be even only on chassis reload. Still, this seems like a good precaution, I don't really see what you'd use that label for.

Copy link
Contributor

@SuperQ SuperQ left a comment

Choose a reason for hiding this comment

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

LGTM, I agree, tracking remote_port is too high a cardinality risk.

@SuperQ
Copy link
Contributor

SuperQ commented Apr 3, 2021

Ping @lspgn. 😄 Any chance we can get some reviews and a release?

@lspgn
Copy link
Contributor

lspgn commented Apr 3, 2021

Hello,
Thank you for the PR, looks good to me.
But I won't be able to merge yet (see #97).
Thank you for your patience.

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.

Cardinality explosion in flow_traffic metrics
4 participants