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

ratelimit: masked_remote_address action don't return descriptor_entry with statsd key words #36275

Closed

Conversation

zirain
Copy link
Contributor

@zirain zirain commented Sep 21, 2024

Commit Message: For envoyproxy/gateway#4281, descriptor_entry with . make it's hard to export metric in prometheus in RateLimit server.
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

Signed-off-by: zirain <[email protected]>
Signed-off-by: zirain <[email protected]>
@zirain
Copy link
Contributor Author

zirain commented Sep 23, 2024

/retest

// For Ipv4, masked_address is returned in a format similar to '192.168.1.0/32', '.' It's a key
// word in statsd, This makes it difficult to export measurements in Promethean format in the
// RateLimit server.
masked_address = absl::StrReplaceAll(masked_address, {{".", "_"}});
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like it might be a breaking change for many people.

Envoy supports many consumers of stats and I feel like we shouldn't tweak naming in a common subsystem to tune to just one of them.

Regarding Prometheus: why not just generate Prom format directly? Why go via statsd?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This seems like it might be a breaking change for many people.

we can either add a runtime feature flag or add an optional API field for this.

Envoy supports many consumers of stats and I feel like we shouldn't tweak naming in a common subsystem to tune to just one of them.

this problem mainly related to envoyproxy/ratelimit, I spend a lot of time in the past trying to bring native promemtheus to it but failed, becase it requires to change a lot of libs used by envoyproxy/ratelimit.

Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like this change is going to spawn a lot of other changes as well. THere are a ton of places where IP addresses and other things with dots show up in stat names and I'm unsure why this one is different.

Copy link
Contributor

Choose a reason for hiding this comment

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

In general I'd like to see specific needs of individual sinks addressed in the sink, ideally, not in code that generates stat names that would also be used by other sinks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, you're right. After rethinking, any ratelimit action that support custom descriptor value will have same problem when user specify ..

let me try to fix this in evnoyproxy/ratelimit.

@jmarantz jmarantz self-assigned this Sep 23, 2024
@jmarantz
Copy link
Contributor

/wait

@zirain
Copy link
Contributor Author

zirain commented Sep 26, 2024

superseded by envoyproxy/ratelimit#716

@zirain zirain closed this Sep 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants