-
Notifications
You must be signed in to change notification settings - Fork 187
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
Bucket level metric #542
Bucket level metric #542
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Just one comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm overall, left one comment
BlobSize: encodedSize, | ||
Rate: globalRates.TotalUnauthThroughput, | ||
RequesterID: key, | ||
RequesterName: systemAccountKey, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't these be requesterName
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm I see how it's confusing but in this case we are calculating the system rate limit metric so we want to make sure the metric that is being updated corresponds to the name "system".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think to Ian's point, this might be a good reason to name these something like MetricsId
and MetricsFlag
, so that the intended usage is clear to the library user. Not super critical though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm
Why are these changes needed?
Adds bucket level metrics to the rate limiter library. Currently I'm making an assumption that if the RequesterName is not empty then we've authenticated the request is some way.
Checks