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

Implement limited per-second hedging #41

Closed
wants to merge 2 commits into from

Conversation

dannykopping
Copy link
Contributor

@dannykopping dannykopping commented Aug 8, 2023

This PR is an upstreaming of an implementation we have within Grafana Labs.

We need to restrict the number of hedged requests on a per-process level, in order to prevent large bursts of hedged requests when we hit the upstream service's limits and all requests start to become slow.

I've imported a dependency, which I believe violates one of the features, but I believe in this case it is warranted and the dependency can be trusted (it's an experimental package from the Go team, and may be upstreamed into the stdlib later).

I still need to write docs for usage, but wanted to get your thoughts on this first.

Credit to @cyriltovena for the initial implementation.

Danny Kopping added 2 commits August 8, 2023 07:32
Signed-off-by: Danny Kopping <danny.kopping@grafana.com>
Signed-off-by: Danny Kopping <danny.kopping@grafana.com>
@dannykopping dannykopping marked this pull request as draft August 8, 2023 05:38
@cristaloleg
Copy link
Member

Hey, thanks for the PR.

I understand your intention to get this merged but this reminds me our old discussion #18 (and the code example https://github.com/cristalhq/hedgedhttp/blob/main/examples_test.go#L89).

Looks like everything can be done via custom http.RoundTripper implementation, which can rate-limit requests. I'm not sure what will be solved by adding it to the library. Can you clarify please? (Forgive me my intentions to keep lib as simple as possible, but not simpler).

My 2c: keeping rate-limited http.RoundTripper outside of cristalhq/hedgedhttp somewhere in grafana/dskit/httputil make more sense, 'cause reusing it across all Grafana projects will be very simple and intuitive.

@dannykopping
Copy link
Contributor Author

@cristaloleg haha I completely forgot about that discussion.

One benefit I see of having it included in the lib is the addition of the RateLimitedRoundTrips stat, but that's probably not a strong enough argument for something that could be passed in instead.

Thanks for the fast review! I'll close this.

@dannykopping
Copy link
Contributor Author

My 2c: keeping rate-limited http.RoundTripper outside of cristalhq/hedgedhttp somewhere in grafana/dskit/httputil make more sense, 'cause reusing it across all Grafana projects will be very simple and intuitive.

Yes, that's a good idea actually

@cristaloleg
Copy link
Member

Glad to hear that you find my idea useful, have a good day 👋

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.

2 participants