querier: Add /api/v1/labels support

[jojohappy]

Signed-off-by: jojohappy sarahdj0917@gmail.com

Changes

Implements: #702

Add /api/v1/labels api to thanos query.

Verification

Run curl <query url>/api/v1/labels to fetch all of labels in Prometheus. Noticed that the api is added in Prometheus 2.6.

[povilasv]

I like the idea of this PR.Good stuff 👍

[povilasv]

Check here for partial response here and return it if it's enabled

[jojohappy]

I have checked partial response at the ProxyStore.LabelNames(), should I do the same things here?

[povilasv]

I might have misunderstood things, the whole point is to make partialResponse work so once you finish the tests this might be not needed :)

[bwplotka]

hm. I don't get you comment @povilasv either as literally there is no way this will return error :

defer runutil.CloseWithLogOnErr(s.logger, indexr, "label names")

 			res := indexr.LabelNames()

 			mtx.Lock()
			sets = append(sets, res)
			mtx.Unlock()
return nil

[bwplotka]

so partial response does not exists here 👍

[jojohappy]

@povilasv maybe g.Wait() will not return an error when the timeout context is cancelled. So in this case, there is no way to return errors in g.Go() block.

[povilasv]

So g.Go() needs to check ctx.Err(), before executing stuff.

	g.Go(func() error {
			defer runutil.CloseWithLogOnErr(s.logger, indexr, "label names")
                        if ctx.Err() != nil {
                        //handle partial data
                        }
 			res := indexr.LabelNames()

 			mtx.Lock()
			sets = append(sets, res)
			mtx.Unlock()

 			return nil
		})

Imagine the case where user canceld the request, and we still have tons of blocks to go thru.

[povilasv]

@jojohappy you forgot to address this ^

[jojohappy]

Sorry, I had updated at this moment, I tried to check partial response enabled when ctx was timeout or cancelled.

[bwplotka]

Quite confused by this thread, and sorry for delayed answer. I think partial response has really small reason of being handled here in store gw. It might just create confusion and being volation of YAGNI as no one requested every to ignore if block errors out. It is totally different to Querier partial response, where query might be still true if one StoreAPI is down

[jojohappy]

@povilasv Thanks for your awesome reivew. I have updated the PR, PTAL.

[bwplotka]

LGTM, just small suggestions.

Plus, let's add CHANGELOG.md entry for Added section (:

[bwplotka]

I think at some point we need to make this more readable.

Either abstract away this or rename as I don't know now if we block Every gRPC call now or what. I needed to dive really deep to tell. But that's not for this PR.

[jojohappy]

So we can create a ticket to record the issue.

[bwplotka]

hm. I don't get you comment @povilasv either as literally there is no way this will return error :

defer runutil.CloseWithLogOnErr(s.logger, indexr, "label names")

 			res := indexr.LabelNames()

 			mtx.Lock()
			sets = append(sets, res)
			mtx.Unlock()
return nil

[bwplotka]

so partial response does not exists here 👍

[jojohappy]

@bwplotka @povilasv Thanks for your review. It is ready for second review iteration.

[povilasv]

Is there a possability that both data and error are returned together? if yes then handle partial response, if no then close this :D

[bwplotka]

This is same for other requests as well we do here 💩 We need to agree on something consistently.

[jojohappy]

@povilasv I will be double check the response from Prometheus. Thanks!

[jojohappy]

@povilasv Yes, I had checked and updated the code at this moment below.

[povilasv]

Sorry for being a bit PITA but it's hard to handle correctly partial response and contexts ⛰️

I do like this PR and functionality

[jojohappy]

@povilasv Sorry for the delay, I will update the PR today ASAP.

[jojohappy]

@povilasv Done.

[povilasv]

@jojohappy you forgot to address this ^

[jojohappy]

@povilasv All comments had been addressed, PTAL, thanks!

[povilasv]

@jojohappy there are a couple left:

https://github.com/improbable-eng/thanos/pull/905/files#r268086099
https://github.com/improbable-eng/thanos/pull/905/files#r270318400

[bwplotka]

Ok, so overall I love this PR, good work @jojohappy @povilasv .. We are quite close to merging this.

However, in recent commits you added PartialResponse handling everywhere, this is wrong, but let's discuss. It should be only in Querier and I mentioned reasons in my comments.

[bwplotka]

Please kill this, just aborting is fine. We are not in Querier.

Essentially If we have error here, means something serious on store gateway indexes and our version is not TSDB compatibile.

[bwplotka]

Quite confused by this thread, and sorry for delayed answer. I think partial response has really small reason of being handled here in store gw. It might just create confusion and being volation of YAGNI as no one requested every to ignore if block errors out. It is totally different to Querier partial response, where query might be still true if one StoreAPI is down

[bwplotka]

Mixed feelings, as IMO it is early, micro optimization that only increases code complexity.

There is no much of the point in checking ctx.Err in every place. It takes more cycles to check this IF than rest of this function not being cancelled (: I guess we wanted to save that memory in this slice when request is cancelled right? I would drop it IMO.

[bwplotka]

Please kill this.

No point in this masking actual error when we have ZERO data to return. No point, and really bad anti-pattern.

If Prometheus errors it means we have no way to tell anything - so error is OK.

Let's agree on something: PartialResponse can be handled and triggered only by querier. It's clear that Partial Response is really tricky to handle and reason about, so let's limit this to basic minimum (Querier proxy StoreAPI only). IMO no value in having it here and on store.

[bwplotka]

Kill this as commented above.

[bwplotka]

What does it mean NoContent? Do you think Prometheus return that ?

What if the resp status is something not 200 though? I am pretty sure we need to check for non 200 response as well before even parsing body to sturct. Here and in other methods.

[jojohappy]

What if the resp status is something not 200 though? I am pretty sure we need to check for non 200 response as well before even parsing body to sturct. Here and in other methods.

Sorry, I forgot to handle the server error response, would be added.

What does it mean NoContent? Do you think Prometheus return that ?

I think we should handle it, because Promethues API will return the http header of StatusNoContent without data. Maybe it would be failed for parsing body, but not sure.

[bwplotka]

IMO this is the only valid partial response handling/triggering in this PR (:

[bwplotka]

Unsorted ?

[jojohappy]

Yes, the names from all stores could not be guaranteed sorted, so should we made a sort in all stores or just do it in proxy?

[povilasv]

HMM prometheus docs don't state the need for sorting, but all examples are sorted so I guess let's do sorting. I would do it in proxy :)

https://prometheus.io/docs/prometheus/latest/querying/api/#getting-label-names

[jojohappy]

The MergeUnsortedSlices would do sorting if the slices were not ordered yet. so the method is ok.

[bwplotka]

Wow the name is so confusing then! ;p

[jojohappy]

@povilasv @bwplotka Done, PTAL, Thanks!

[povilasv]

Add sorting and IMO we are good to go.

@jojohappy sorry for misguiding you on partial response. I owe you 🍺 if we meet in some conference or smth :)

[jojohappy]

@povilasv It's ok, and I must give respects for your careful code review. Thank you very much.

[bwplotka]

Some one said 🍺 ? ❤️

[povilasv]

Awesome!

[bwplotka]

Awesome, thanks!