Inconsistency of the behaviors of handling labelValues requests and external labels

[yeya24]

We are using different ways to handle LabelValues requests for each store.

For PrometheusStore which is the sidecar, it checks whether the label we want exists in external labels, if so just return the value.

func (p *PrometheusStore) LabelValues(ctx context.Context, r *storepb.LabelValuesRequest) (*storepb.LabelValuesResponse, error) {
	externalLset := p.externalLabelsFn()

	// First check for matching external label which has priority.
	if l := externalLset.Get(r.Label); l != "" {
		return &storepb.LabelValuesResponse{Values: []string{l}}, nil
	}

	vals, err := p.client.LabelValuesInGRPC(ctx, p.base, r.Label, nil, r.Start, r.End)

For BucketStore, we get the label values from the block's index and attach the external label value if it matches.

			// Do it via index reader to have pending reader registered correctly.
			res, err := indexr.block.indexHeaderReader.LabelValues(req.Label)
			if err != nil {
				return errors.Wrap(err, "index header label values")
			}

			// Add the external label value as well.
			if extLabelValue, ok := extLabels[req.Label]; ok {
				res = strutil.MergeSlices(res, []string{extLabelValue})
			}

For TSDBStore, we just ignore the external labels. https://github.com/thanos-io/thanos/blob/master/pkg/store/tsdb.go#L221-L235.

I think we need to define the correct logic of the external labels handling for Labels API to make the whole thing more clear. As we plan to support matchers #3637 so it would be more complex.

[bwplotka]

Yup, agree. I think we should just inject them always.

WDYT @brancz @kakkoyun @pracucci @yeya24 ? 🤗

[yeya24]

So for LabelNames, we need to always inject all the external label names?
For example, if a LabelNames query is outside the store's time range or the matchers don't match any series, then the original response would be empty, but we still want to inject the external labels?

[pracucci]

For example, if a LabelNames query is outside the store's time range or the matchers don't match any series, then the original response would be empty, but we still want to inject the external labels?

I think we should pick up external labels only for blocks within the query time range. In your example, if the LabelNames query is outside the store's time range then we shouldn't inject any external label.

[stale]

Hello 👋 Looks like there was no activity on this issue for the last two months.
Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity in the next two weeks, this issue will be closed (we can always reopen an issue if we need!). Alternatively, use remind command if you wish to be reminded at some point in future.

[stale]

Closing for now as promised, let us know if you need this to be reopened! 🤗

[stale]

Hello 👋 Looks like there was no activity on this issue for the last two months.
Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity in the next two weeks, this issue will be closed (we can always reopen an issue if we need!). Alternatively, use remind command if you wish to be reminded at some point in future.

[stale]

Closing for now as promised, let us know if you need this to be reopened! 🤗

[stale]

Hello 👋 Looks like there was no activity on this issue for the last two months.
Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity in the next two weeks, this issue will be closed (we can always reopen an issue if we need!). Alternatively, use remind command if you wish to be reminded at some point in future.

[stale]

Closing for now as promised, let us know if you need this to be reopened! 🤗

[GiedriusS]

Fixed in #6816