Rate limit hedge requests

[cyriltovena]

Hello there,

What do we think about adding a way to rate limits hedged requests ?

For example if we suddenly seems to hedge all request because the tail latency changed, we may actually worsen the problem.

I suggest we add a percentage parameter which tells how often we can hedge request based on actual requested throughput.

For example if the value is 10%, then we can only hedge 1 request out of 10. Default could be 100%, all request can be hedged.

What do you think ?

[dannykopping]

I'm thinking that having a ceiling is going to be more useful in practice than a %.

Let's say we hedge 10%, and have a retry (upto) count of 2.

If the upstream service we're hedging requests for slows down drastically, we'll be re-issuing all 10% of our requests x 3 (1 initial request and 2 retries), which will triple our latency in 10% of requests.

If we're hedging more than a couple hundred requests a minute, that's probably pathological on the upstream service's side or we're being too aggressive.

[bboreham]

How about putting an actual rate limit on hedged requests, like "you can issue hedged requests up to 10 times per second".
E.g. http://pkg.go.dev/golang.org/x/time/rate

[cristaloleg]

Hi almost all Grafana team 😂 yeah, great idea, like it. I had something on my mind long time ago but left for the future.

But later left untouched 'cause now it's possible to substitute rate limited RoundTripper (https://github.com/cristalhq/hedgedhttp/blob/main/hedged.go#L53) which will take care not to overload targets.

type Transport struct {
	Transport http.RoundTripper // Used to make actual requests.
	Limiter   RateLimiter
}

func (t *Transport) RoundTrip(r *http.Request) (*http.Response, error) {
	if err := t.Limiter.Wait(r.Context()); err != nil {
		return nil, err
	}
	return t.Transport.RoundTrip(r)
}

In other words: passing Transport from above in NewRoundTripperAndStats should solve the problem. Objections?

[cyriltovena]

We actually just want to rate limit hedge requests and not other requests, so I don't think that works since it will rate limit everything right ?

[cristaloleg]

A-ha, got it. Well, yeah, probably there is no such way right now. Unless it's possible to make something with request so the RoundTripper can distinguish them and apply rate limit.

I also thought about something else: provide a param to NewRoundTripperAndStats (probably it's time to add Config type as a param and hide everything inside it) to provide RoundTripper ONLY for hedged requests.

So:

• no RoundTripper - default is used
• just RoundTripper - used for 1st request and following hedged request
• RoundTripper and hedged RoundTripper - obvious :)

Looks like this should solve the problem started above and probably will make observability easier. Objections? :)

[cyriltovena]

RoundTripper ONLY for hedged requests

Sounds like it would work perfectly.

[cristaloleg]

One more idea discussed with @storozhukBM in priv: help to distinguish 1st and following requests via context. Maybe func IsHedgedRequest(context.Context) bool could address this in a better way.

We pay small-ish price creating context with value but the underlying RoundTripper can decide what to do with a request, based on the result of IsHedgedRequest. Same can simplify observability things too, just all the logic will be hidden in RoundTripper.

Ah, and also tests can be simpler, again due to 1 helper. WDYT?

[cristaloleg]

Made a simple PR with IsHedgedRequest ^^^ so only for 2nd and next requests func will return true.
I'm very curious to bench current main vs this PR to see how many garbage will appear on heap but I think the price is still small, hopefully will bench it soon.

[cyriltovena]

I just realized that once a hedged request is fired it;s already too late. What we want actually is to control if we should hedged or not.

So while this help for instrumentation, I don't think it works for rate limiting.

[cristaloleg]

Well, it's created and passed to a RoundTripper but only RoundTripper decides to pass it further and send over a network. However we(you?) might want to catch it earlier.

Sounds like it would work perfectly.
so, looks like 2nd RoundTripper will also get already fired request, it doesn't solve the problem too :(

From another perspective: what is the problem with context approach? price of a request creation is small, metrics aren't skewed (https://github.com/cristalhq/hedgedhttp/blob/main/hedged.go#L117 is correct, request is failed due to rate limit), rate limit is correct and we can create such if with IsHedgedRequest where we can rate limit only hedged or all requests.

Am I missing something?

[cyriltovena]

We could definitively fails the hedge request what is the behaviour for the original requests ?

[cristaloleg]

The same as before - nothing happens.
I will make an example test with the rate limited RoundTripper, probably today-ish 😉

[cyriltovena]

ok so if a hedge request returns an error it's discarded.

[cristaloleg]

Yes, exactly.

[cyriltovena]

Then I think we're good to close this once the PR is merged.

I close the Prometheus one because your argument were good enough for me.

[cristaloleg]

Released https://github.com/cristalhq/hedgedhttp/releases/tag/v0.6.2
Will try to cover examples today.

[cyriltovena]

Thank you for the prompt release.

[annanay25]

Hi almost all Grafana team 😂

Tripping over this. Thanks @cristaloleg! Clearly we love this project over at Grafana.

[cristaloleg]

@annanay25 super glad to hear 😉 ❤️

[cristaloleg]

New release, examples and better README (and a bit better CI) https://github.com/cristalhq/hedgedhttp/releases/tag/v0.6.3 (diff v0.6.2...v0.6.3)