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

ratelimits: Remove a metric and some labels that we're not finding useful #7902

Merged
merged 6 commits into from
Dec 20, 2024

Conversation

beautifulentropy
Copy link
Member

Fixes #7879

@beautifulentropy beautifulentropy marked this pull request as ready for review December 18, 2024 20:45
@beautifulentropy beautifulentropy requested a review from a team as a code owner December 18, 2024 20:45
jsha
jsha previously approved these changes Dec 18, 2024
Copy link
Contributor

@jsha jsha left a comment

Choose a reason for hiding this comment

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

Nice cleanups!

We had talked in standup specifically about removing the batchset_entry, batchsetnotexisting_entry, and other observations that try to backformulate the per-request latency based on overall pipeline latency (which has some potential to be misleading since the costs don't divide out evenly). I see this PR goes one step farther and removes the whole ratelimits_latency metric.

I'm okay with that; we also have the ratelimits_spend_latency metric:

Help: fmt.Sprintf("Latency of ratelimit checks labeled by limit=[name] and decision=[%s|%s], in seconds", Allowed, Denied),
. That one is different in that it can span multiple calls (Get, BatchSetNotExisting, etc). But on balance it probably suffices. I see that the Redis client also has otel instrumentation which may be a nicer way to get per-Pipeline() latencies anyhow, once we start using otel internally.

@beautifulentropy
Copy link
Member Author

We had talked in standup specifically about removing the batchset_entry, batchsetnotexisting_entry, and other observations that try to backformulate the per-request latency based on overall pipeline latency (which has some potential to be misleading since the costs don't divide out evenly). I see this PR goes one step farther and removes the whole ratelimits_latency metric.

Oh, thanks for bringing this up! I actually thought that discussion started with a proposal to remove this metric. Removing just per_entry makes a whole lot of sense, I'm quite happy to revert most of these removals. Let me throw something together real quick.

jsha
jsha previously approved these changes Dec 18, 2024
@beautifulentropy beautifulentropy changed the title ratelimits: Remove a couple metrics that we're not finding useful ratelimits: Remove a metric and some labels that we're not finding useful Dec 19, 2024
ratelimits/limit.go Outdated Show resolved Hide resolved
@beautifulentropy beautifulentropy merged commit 6402a22 into main Dec 20, 2024
12 checks passed
@beautifulentropy beautifulentropy deleted the ratelimits-metrics-removal branch December 20, 2024 13:44
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.

limit.overrideKey is computed incorrectly
4 participants