Adds the ability to hedge storage requests.

[cyriltovena]

Hedges GCS/S3/Azure/Swift requests using this library.

There's 2 minor caveats:

• Doesn't work for the ruler.
• For Openstack the implementation also hedge auth request. Required an upstream patch if we don't want to.

What this PR does / why we need it:

This allow to reduce the tail latency see paper Tail at Scale by Jeffrey Dean, Luiz André Barroso. In short: the client first sends one request, but then sends an additional request after a timeout if the previous hasn't returned an answer in the expected time. The client cancels remaining requests once the first result is received.

Special notes for your reviewer:

Checklist

• Documentation added
• Tests updated
• Add an entry in the CHANGELOG.md about the changes.

[dannykopping]

I really want this feature and I think the implementation looks solid; I've added a few small questions.

I think we should not merge this though until we have a way to deal with the following situation:

GCS/S3/whatever is experiencing a partial outage, which is causing its latency to spike. This is adding 200ms to all requests. This additional latency causes all of our requests to get hedged, which actually does more harm than good here since it increases tail latencies across the board ironically.

AFAICS from this PR, there is no protection against this.
I'm also not sure if any metrics have been added to see how many requests are hedged - did I miss something?

One dumb solution would be to add a config option for the maximum number of hedged requests per querier per second/minute.

[cyriltovena]

I really want this feature and I think the implementation looks solid; I've added a few small questions.

I think we should not merge this though until we have a way to deal with the following situation:

GCS/S3/whatever is experiencing a partial outage, which is causing its latency to spike. This is adding 200ms to all requests. This additional latency causes all of our requests to get hedged, which actually does more harm than good here since it increases tail latencies across the board ironically.

AFAICS from this PR, there is no protection against this. I'm also not sure if any metrics have been added to see how many requests are hedged - did I miss something?

One dumb solution would be to add a config option for the maximum number of hedged requests per querier per second/minute.

Unfortunately all those ask require improvement of the library upstream. I guess we could do that.

If we all think we should do it, then I can look into it.

[dannykopping]

I really want this feature and I think the implementation looks solid; I've added a few small questions.
I think we should not merge this though until we have a way to deal with the following situation:
GCS/S3/whatever is experiencing a partial outage, which is causing its latency to spike. This is adding 200ms to all requests. This additional latency causes all of our requests to get hedged, which actually does more harm than good here since it increases tail latencies across the board ironically.
AFAICS from this PR, there is no protection against this. I'm also not sure if any metrics have been added to see how many requests are hedged - did I miss something?
One dumb solution would be to add a config option for the maximum number of hedged requests per querier per second/minute.

Unfortunately all those ask require improvement of the library upstream. I guess we could do that.

If we all think we should do it, then I can look into it.

Does it have to be changed upstream? We could create a layer of indirection which both tracks all hedged requests and cancels it if there have been too many.

Having it upstream would be nice, too.

[cyriltovena]

Does it have to be changed upstream? We could create a layer of indirection which both tracks all hedged requests and cancels it if there have been too many.

Having it upstream would be nice, too.

The library uses the http.Roundtripper pattern:

• if you wrap after you don't know if you're a hedge request or a normal one.
• if you wrap before you can't predict if the request would be hedge or not.

It might be possible by wrapping before AND after, but that seems complex. I'll give a go if you don't hear from me on that matter means I couldn;t :)

[owen-d]

This looks good and I like @dannykopping's suggestion to limit the number of hedged requests to a certain percentage of total reqs, but I also don't want perfect to be the enemy of good and think this is beneficial enough on it's own.

Instead of making this a per-store option, could we implement it a level higher by wrapping the storage client interface to create a hedging client? That would also allow us to expose only one hedging config block, rather than one per backend.

[cyriltovena]

FYI I asked this before submitting this PR.

cristalhq/hedgedhttp#17

[cyriltovena]

Instead of making this a per-store option, could we implement it a level higher by wrapping the storage client interface to create a hedging client? That would also allow us to expose only one hedging config block, rather than one per backend.

I hesitated to do this although I realized it won't be applicable to some other backend like grpc or local. But if we think that doesn't matter I'm in for making it broader.

[dannykopping]

Instead of making this a per-store option, could we implement it a level higher by wrapping the storage client interface to create a hedging client? That would also allow us to expose only one hedging config block, rather than one per backend.

Yeah I really like that, if possible

[cyriltovena]

Yeah I really like that, if possible

Done ✨

[dannykopping]

Approving, we'll add the hedging rate-limiting in a follow-up PR