Return Canceled when a LabelNames or LabelValues request to a store-gateway is cancelled by the caller, and return Internal otherwise for all requests.

[charleskorn]

What this PR does

See #4007 for background.

Which issue(s) this PR fixes or relates to

Follow on from #4007.

Checklist

• Tests updated
• [n/a] Documentation added
• CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

[pracucci]

The changes LGTM!

I've opened this as a draft because the tests that I've added in this PR currently fail ~50% of the time due to a race here where the context cancellation competes with the loading of expanded postings. I can't find a nice way to make these tests robust - would love some suggestions.

An option is to hit the context canceled in a different path. For example, you could configure the store WithIndexCache() and pass an implementation which returns the context.Canceled error. This way you don't have to directly pass a canceled context to the LabelNames() function itself.

[charleskorn]

An option is to hit the context canceled in a different path. For example, you could configure the store WithIndexCache() and pass an implementation which returns the context.Canceled error. This way you don't have to directly pass a canceled context to the LabelNames() function itself.

Good idea. Unfortunately, we log + ignore errors from caches, so the best other place to do this seems to be in the object storage implementation, which is what I've done here.

I'm wondering whether it makes sense to modify github.com/thanos-io/objstore/providers/filesystem.Bucket to respect context cancellations, which would remove the need for cancellationRespectingBucket here and also bring the implementation in line with how other object storage implementations behave with respect to context cancellations - what do you think?

[pracucci]

I'm wondering whether it makes sense to modify github.com/thanos-io/objstore/providers/filesystem.Bucket to respect context cancellations, which would remove the need for cancellationRespectingBucket here and also bring the implementation in line with how other object storage implementations behave with respect to context cancellations - what do you think?

Would be a cleaner solution. Let's try to offer the upstream change to filesystem.Bucket and see if they accept it.

[charleskorn]

I'm wondering whether it makes sense to modify github.com/thanos-io/objstore/providers/filesystem.Bucket to respect context cancellations, which would remove the need for cancellationRespectingBucket here and also bring the implementation in line with how other object storage implementations behave with respect to context cancellations - what do you think?

Would be a cleaner solution. Let's try to offer the upstream change to filesystem.Bucket and see if they accept it.

I've raised thanos-io/objstore#43, let's see what they think.

[charleskorn]

thanos-io/objstore#43 has been merged 🎉. Once #4136 is merged, I'll update this PR to take advantage of it.

[pracucci]

Nice, thanks!