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

Improve error message for stream rate limit. #4207

Merged
merged 8 commits into from
Sep 30, 2021
Merged

Improve error message for stream rate limit. #4207

merged 8 commits into from
Sep 30, 2021

Conversation

cstyan
Copy link
Contributor

@cstyan cstyan commented Aug 23, 2021

Logs the rate limit, stream labels, and bytes for that push in the error/error message.

Signed-off-by: Callum Styan callumstyan@gmail.com

@cstyan cstyan requested a review from a team as a code owner August 23, 2021 14:56
Copy link
Member

@owen-d owen-d left a comment

Choose a reason for hiding this comment

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

While I do think it will be nice to have some more introspection here, I'd like to minimize costs on the hot path. Additionally, it'd be nice to use the bytesize types in the error messages to use human readable forms when possible.

pkg/ingester/stream.go Outdated Show resolved Hide resolved
pkg/ingester/stream.go Outdated Show resolved Hide resolved
pkg/ingester/stream.go Outdated Show resolved Hide resolved
pkg/ingester/stream.go Outdated Show resolved Hide resolved
@cstyan
Copy link
Contributor Author

cstyan commented Sep 13, 2021

Rebased off main but tests are still failing, @owen-d if the error message improvement is still relevant I will fix the issues with tests, otherwise we can close.

Copy link
Member

@owen-d owen-d left a comment

Choose a reason for hiding this comment

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

This is looking good, but since it's the hot path, I'd like to avoid unnecessarily re-checking the limit (which uses a mutex under the hood).

pkg/ingester/stream.go Outdated Show resolved Hide resolved
@cstyan
Copy link
Contributor Author

cstyan commented Sep 21, 2021

Test failures are unrelated, see: #4344

@dannykopping
Copy link
Contributor

Test failures are unrelated, see: #4344

@cstyan apologies, if you rebase then CI should be working since that PR has been merged.

Signed-off-by: Callum Styan <callumstyan@gmail.com>
Signed-off-by: Callum Styan <callumstyan@gmail.com>
print human readable rate limits and log line byte lengths.

Signed-off-by: Callum Styan <callumstyan@gmail.com>
Signed-off-by: Callum Styan <callumstyan@gmail.com>
Signed-off-by: Callum Styan <callumstyan@gmail.com>
directly since the rate limit error is an RPC error.

Signed-off-by: Callum Styan <callumstyan@gmail.com>
Signed-off-by: Callum Styan <callumstyan@gmail.com>
Comment on lines 222 to 223
// on each entry in the push (hot path) and we only use this value when logging entries that
// passed the rate limit.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// on each entry in the push (hot path) and we only use this value when logging entries that
// passed the rate limit.
// on each entry in the push (hot path) and we only use this value when logging entries over
// the rate limit.

Signed-off-by: Callum Styan <callumstyan@gmail.com>
@owen-d owen-d merged commit 2516347 into grafana:main Sep 30, 2021
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.

3 participants