Improve Bucket LabelValues Call with matchers

[yeya24]

Describe the solution you'd like

Upstream Prometheus merges the improvement about LabelValues with matches today prometheus/prometheus#8400. It would be nice to have this support in Thanos as well.

AC:

1. Add matchers support to TSDB store with the new LabelValues interface.
2. For Prometheus store (sidecar), we can add matchers to the LabelValues query and send it to Prometheus. This is tricky because we also need to check the version of Prometheus. If the version doesn't meet, we fallback to series query.
3. Implement this new LabelValues interface in Store gateway.

These steps can be done in different PRs.

[Namanl2001]

If nobody is working on this then I and @Abhishek357 wants to take this up.

[yeya24]

@Namanl2001 That would be great! Please go ahead. It would be easier to start with the first and the second point.

[bwplotka]

From Contributor Office Hour: Why porting LabelValues matchers, how it's improving performance?

Reason:

1. First: Isolation. Our multitenancy querying pattern is based on injecting label-matcher for tenant label which is usually part external labels.
2. Performance: You can return less data.

[Namanl2001]

I am working on task 1: Add matchers support to TSDB store.
As the Prometheus version is updated in Thanos, I am blocked with few questions:

• Are we talking about matchers contained in storepb.LabelValuesRequest?
• Passing matchers along with r.label here would give the expected results?

PS: It would be beneficial to get some hints for further debugging/testing.

[yeya24]

Are we talking about matchers contained in storepb.LabelValuesRequest?

Yes, you can use that.

Passing matchers along with r.label here would give the expected results?

Yes. As the matchers in the storepb.LabelValuesRequest are not Prometheus matchers type, you need to first convert them to the correct struct. You can use the snippet below.

matchers, err := storepb.MatchersToPromMatchers(r.Matchers...)
// TODO: Handle the err above.
res, _, err := q.LabelValues(r.Label, matchers...)

I think that's all for the code change, but please also add some tests to the thanos/pkg/store/tsdb_test.go file.
Let me know if you have other questions.

[Namanl2001]

Thanks!
Few questions regarding testing

• Should we add matchers in the existing test data or create new test_data to test matchers support separately?
• I guess matchers should look like as shown below for this test case. Please correct me If I'm wrong.

Matchers: []storepb.LabelMatcher{
				{Type: storepb.LabelMatcher_EQ, Name: "foo", Value: "test"},
			}

• What should be Name & Value values of matchers for this type of test case, where there are two expected values?

[yeya24]

Should we add matchers in the existing test data or create new test_data to test matchers support separately?

It is up to you. If you feel more data is required you can add more.

I guess matchers should look like as shown below for this test case. Please correct me If I'm wrong.

That's correct.

What should be Name & Value values of matchers for this type of test case, where there are two expected values?

Are you asking this specific test case? In this test case, after adding series with labels {foo="test1}", now it contains two sereis: {foo="test}", `{foo="test1}".

If you query the label values of foo, you can get two values test and test1 without adding any matchers.
You can definitely filter the values by adding some matchers like {foo!="test1"}, though it doesn't make sense in this example.

[Namanl2001]

Now, I'm on Task 2: For Prometheus store (sidecar), add matchers to the LabelValues query and send it to Prometheus. I've few questions:

• Could we pass r.Matchers in this function in place of nil, for the results we are looking for?
• Is a check for the Prometheus version still needed? As it's already updated in Thanos.

[yeya24]

Could we pass r.Matchers in this function in place of nil, for the results we are looking for?

Yes. That's for Prometheus version that meets the requirement.

Is a check for the Prometheus version still needed? As it's already updated in Thanos.

Yes, that's different. We support this in Thanos, but it is just the client side, which means matchers=xxx will be added to the HTTP request we send to Prometheus. But if the Prometheus version is too low, it cannot recognize that param and just returns all label values. So a check is still needed.
We need to fallback to series call if the version is too low.

[Namanl2001]

Yes, that's different. We support this in Thanos, but it is just the client side, which means matchers=xxx will be added to the HTTP request we send to Prometheus. But if the Prometheus version is too low, it cannot recognize that param and just returns all label values. So a check is still needed.
We need to fallback to series call if the version is too low.

This sounds good to me. But I'm not getting the code block where this check should be added.

[yeya24]

You can add that to pkg/api/query/v1.go labelValues method. If matchers are set, then we need to get Prometheus's version and see if it meets our requirements.

For getting Prometheus's version, you can take a look at this API https://prometheus.io/docs/prometheus/latest/querying/api/#build-information.

[Namanl2001]

Facing problem in getting the prometheus version. Are we using any other prometheus API already, from which I could get some idea to implement this GET /api/v1/status/buildinfo. Thanks.

[yeya24]

@Namanl2001 You can take a look at promclient package. You can also find me on Slack if you have further questions regarding this task.