6178 matchers with label values #7496

eklockare · 2020-07-01T15:38:43Z

This PR is for issue #6178

This adds the ability to pass the ´match[]´ parameter to ´/api/v1/label/<label_name>/values´ endpoint.

Example:

curl -s 'http://localhost:9090/api/v1/label/code/values?match[]=promhttp_metric_handler_requests_total%7Bcode%3D%22200%22%7D'

( url decoded: promhttp_metric_handler_requests_total{code="200"} )

{
  "status": "success",
  "data": [
    "200"
  ]
}

Questions:

Should labelValuesByMatchers be a method of the queriers to be consistent with LabelValues? I tried that out but ran in to issues, but could have another go at it.
Is a seperate unittest for labelValuesByMatchers a good idea? From what I see tests are not run on specific functions of api.go. I did feel like having more labels in my tests but also didn't want to mess with all the other tests cases in api_test.go
Build fails, but seems to be unrelated?

Unable to find image 'quay.io/prometheus/golang-builder:1.14-base' locally

Error response from daemon: Get https://quay.io/v2/prometheus/golang-builder/manifests/1.14-base: Get https://quay.io/v2/auth?scope=repository%3Aprometheus%2Fgolang-builder%3Apull&service=quay.io: net/http: request canceled while waiting for connection (Client.Timeout exceeded while awaiting headers)

!! exit status 1

Signed-off-by: Erik Klockare <eklockare@gmail.com>

brian-brazil

I think this is likely to run us into the performance problems in #5547, so we probably want my fix for that in before trying to further use Select purely for metadata.

brian-brazil · 2020-07-01T16:59:08Z

docs/querying/api.md

@@ -320,6 +320,8 @@ URL query parameters:

 - `start=<rfc3339 | unix_timestamp>`: Start timestamp. Optional.
 - `end=<rfc3339 | unix_timestamp>`: End timestamp. Optional.
+- `match[]=<series_selector>`: Repeated series selector argument that selects the
+  series from which to read the label's values 


label values.

brian-brazil · 2020-07-01T17:00:48Z

web/api/v1/api.go

+// LabelValuesByMatchers uses matchers to filter out matching series, then label values are extracted.
+func labelValuesByMatchers(sets []storage.SeriesSet, name string) ([]string, storage.Warnings, error) {
+	set := storage.NewMergeSeriesSet(sets, storage.ChainedSeriesMerge)
+	labelValuesMap := make(map[string]string)


A map[string]struct{} is the usual way to do a set.

Aha, didn't know, changed to map[string]struct{} 👍

brian-brazil · 2020-07-01T17:01:14Z

web/api/v1/api.go

+	for set.Next() {
+		series := set.At()
+		labelValue := series.Labels().Get(name)
+		// This is to avoid duplicates.


You can do a simple assignment and not worry about it.

True, fixed

Signed-off-by: Erik Klockare <eklockare@gmail.com>

eklockare · 2020-07-01T20:43:34Z

@brian-brazil I'm not familiar with the inner workings of the data loading yet. Does your hint solution make it so that less (or no) data is loaded (i.e. no chunks)? So that we get a larger proportion of metadata over actual data

brian-brazil · 2020-07-02T11:51:10Z

web/api/v1/api.go

-		_, ok := labelValuesMap[labelValue]
-		if !ok && labelValue != ""  {
-			labelValuesMap[labelValue] = labelValue
+		if labelValue != ""  {


The label value can't be empty, if it is that's a bug in the tsdb.

brian-brazil · 2020-07-02T11:54:02Z

Does your hint solution make it so that less (or no) data is loaded (i.e. no chunks)?

It makes it so the chunks don't end up trashing the page cache. Basically due to this the feature in this PR could cause severe performance problems, so I think we need #5547 fixed first to be safe.

Signed-off-by: Erik Klockare <eklockare@gmail.com>

bwplotka

I would say, let's go for it. It's worth to mention it's not as fast and non-matcher function, but it will do the work just fine.

This PR looks fine to me LGTM! ❤️

I guess we want the same on label_name as well (:

bwplotka · 2020-10-21T16:32:30Z

Plus we need to fix tests (:

bwplotka · 2020-10-21T16:32:52Z

are you available @eklockare ? sorry for long delay! (:

brian-brazil · 2020-10-21T17:07:57Z

web/api/v1/api.go

+		// Get all series which match matchers.
+		var sets []storage.SeriesSet
+		for _, mset := range matcherSets {
+			s := q.Select(false, nil, mset...)


You need to pass in a hint here with the series function, otherwise this could trash the page cache.

yeya24 · 2020-10-21T17:24:29Z

web/api/v1/api.go

+		return nil, warnings, set.Err()
+	}
+	// Convert the map to an array.
+	labelValues := []string{}


Maybe we can use labelValues := make([]string, 0, len(labelValueSet)) here.

brancz · 2020-12-07T08:59:12Z

Hey @eklockare, very nice contribution! This seems very close to completion. Do you think you'll have time to resolve the remaining comments? Otherwise I'm happy to continue your work! :)

bwplotka · 2020-12-08T16:03:56Z

web/api/v1/api.go

+			s := q.Select(false, nil, mset...)
+			sets = append(sets, s)
+		}
+		vals, warnings, err = labelValuesByMatchers(sets, name)


Can we add TODO to extend LabelValues and LabelNames with matches?

I would also prefer if this was done on TSDB level. Last time we discussed this we decided against it, but I'm happy to have that discussion again (I realize that TSDB doesn't work that way today).

brancz · 2020-12-09T09:54:19Z

I guess we want the same on label_name as well (:

Side note, label-names are going to work slightly differently, as we need extra logic to ignore label-names that we have an exact matcher for.

brian-brazil · 2020-12-09T09:57:31Z

Side note, label-names are going to work slightly differently, as we need extra logic to ignore label-names that we have an exact matcher for.

Why's that?

codesome

I was discussing this offline with @bwplotka and we think this should be part of TSDB to make it more efficient. Maybe introducing hints for LabelValues similar to Select() which remote storages can also make use of.

brancz · 2020-12-09T12:33:34Z

@brian-brazil it doesn't make sense to return a label name that we pass an exact matcher for, isn't that the whole point of passing matchers?

brian-brazil · 2020-12-09T13:25:48Z

If I have a set of series per a selector, why wouldn't I return all of the label names that have non-empty values among those series?
What you propose doesn't make sense to me semantically. That it appeared as a matcher doesn't change that it is a relevant label name.

brancz · 2020-12-10T18:46:45Z

Yeah, thought about it again, and I agree.

bwplotka · 2020-12-16T21:34:09Z

Looks like this PR is not active anymore, I heard @yeya24 want to continue this to get this merged 🤗

Hope @eklockare, you are ok with this, we will use your commits as a start 👍🏽

brian-brazil · 2020-12-23T18:45:47Z

Superseded by #8301

eklockare added 3 commits July 1, 2020 11:07

implemenents matchers to label values endpoint

e8610ca

Signed-off-by: Erik Klockare <eklockare@gmail.com>

added more tests

4840b31

Signed-off-by: Erik Klockare <eklockare@gmail.com>

Fixed formatting on comments

d5829a4

Signed-off-by: Erik Klockare <eklockare@gmail.com>

brian-brazil reviewed Jul 1, 2020

View reviewed changes

eklockare added 2 commits July 1, 2020 19:23

fixed documentation typo

7a7ac4c

Signed-off-by: Erik Klockare <eklockare@gmail.com>

label values set is now a string struct map

65e7571

Signed-off-by: Erik Klockare <eklockare@gmail.com>

eklockare requested a review from brian-brazil July 1, 2020 20:43

brian-brazil reviewed Jul 2, 2020

View reviewed changes

removed label value empty check

42fa927

Signed-off-by: Erik Klockare <eklockare@gmail.com>

eklockare force-pushed the 6178-matchers-with-label-values branch from a889c62 to 42fa927 Compare July 8, 2020 11:39

krasi-georgiev mentioned this pull request Aug 31, 2020

WIP: Filter labels for matching series by name #6837

Closed

stale bot added the stale label Sep 6, 2020

bwplotka approved these changes Oct 21, 2020

View reviewed changes

stale bot removed the stale label Oct 21, 2020

brian-brazil reviewed Oct 21, 2020

View reviewed changes

yeya24 reviewed Oct 21, 2020

View reviewed changes

yeya24 mentioned this pull request Dec 8, 2020

storeAPI: Extend ExternalLabels Selector for LabelsNames and LabelValues thanos-io/thanos#3351

Closed

bwplotka reviewed Dec 8, 2020

View reviewed changes

codesome reviewed Dec 9, 2020

View reviewed changes

yeya24 mentioned this pull request Dec 17, 2020

Support matchers for Labels API #8301

Merged

brian-brazil closed this Dec 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

6178 matchers with label values #7496

6178 matchers with label values #7496

eklockare commented Jul 1, 2020 •

edited

brian-brazil left a comment

brian-brazil Jul 1, 2020

eklockare Jul 1, 2020

brian-brazil Jul 1, 2020

eklockare Jul 1, 2020

brian-brazil Jul 1, 2020

eklockare Jul 1, 2020

eklockare commented Jul 1, 2020

brian-brazil Jul 2, 2020

eklockare Jul 3, 2020

brian-brazil commented Jul 2, 2020

bwplotka left a comment

bwplotka commented Oct 21, 2020

bwplotka commented Oct 21, 2020

brian-brazil Oct 21, 2020

yeya24 Oct 21, 2020

brancz commented Dec 7, 2020

bwplotka Dec 8, 2020

brancz Dec 9, 2020

brancz commented Dec 9, 2020

brian-brazil commented Dec 9, 2020

codesome left a comment

brancz commented Dec 9, 2020

brian-brazil commented Dec 9, 2020

brancz commented Dec 10, 2020

bwplotka commented Dec 16, 2020

brian-brazil commented Dec 23, 2020

6178 matchers with label values #7496

6178 matchers with label values #7496

Conversation

eklockare commented Jul 1, 2020 • edited

brian-brazil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eklockare commented Jul 1, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brian-brazil commented Jul 2, 2020

bwplotka left a comment

Choose a reason for hiding this comment

bwplotka commented Oct 21, 2020

bwplotka commented Oct 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brancz commented Dec 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brancz commented Dec 9, 2020

brian-brazil commented Dec 9, 2020

codesome left a comment

Choose a reason for hiding this comment

brancz commented Dec 9, 2020

brian-brazil commented Dec 9, 2020

brancz commented Dec 10, 2020

bwplotka commented Dec 16, 2020

brian-brazil commented Dec 23, 2020

eklockare commented Jul 1, 2020 •

edited