Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Cardinality API] Include missing labels in label values #8450

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@
* [ENHANCEMENT] Query-frontend: added `remote_read` to `op` supported label values for the `cortex_query_frontend_queries_total` metric. #8412
* [ENHANCEMENT] Query-frontend: log the overall length and start, end time offset from current time for remote read requests. The start and end times are calculated as the miminum and maximum times of the individual queries in the remote read request. #8404
* [ENHANCEMENT] Storage Provider: Added option `-<prefix>.s3.dualstack-enabled` that allows disabling S3 client from resolving AWS S3 endpoint into dual-stack IPv4/IPv6 endpoint. Defaults to true. #8405
* [ENHANCEMENT] Cardinality API: Include series counts for series without a value in label_values endpoint #8450
* [BUGFIX] Distributor: prometheus retry on 5xx and 429 errors, while otlp collector only retry on 429, 502, 503 and 504, mapping other 5xx errors to the retryable ones in otlp endpoint. #8324 #8339
* [BUGFIX] Distributor: make OTLP endpoint return marshalled proto bytes as response body for 4xx/5xx errors. #8227
* [BUGFIX] Rules: improve error handling when querier is local to the ruler. #7567
Expand Down
4 changes: 3 additions & 1 deletion pkg/ingester/ingester_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3842,6 +3842,7 @@ func TestIngester_LabelValuesCardinality(t *testing.T) {
"200": 1,
"300": 1,
"500": 1,
"": 1,
},
},
{
Expand All @@ -3855,6 +3856,7 @@ func TestIngester_LabelValuesCardinality(t *testing.T) {
LabelName: "env",
LabelValueSeries: map[string]uint64{
"prod": 2,
"": 2,
},
},
},
Expand All @@ -3867,7 +3869,7 @@ func TestIngester_LabelValuesCardinality(t *testing.T) {
expectedItems: []*client.LabelValueSeriesCount{
{
LabelName: "status",
LabelValueSeries: map[string]uint64{"300": 1},
LabelValueSeries: map[string]uint64{"300": 1, "": 1},
},
},
},
Expand Down
19 changes: 19 additions & 0 deletions pkg/ingester/label_names_and_values.go
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,25 @@ func labelValuesCardinality(
if err != nil {
return err
}

// Check if empty value for label is allowed according to matchers. This
// is an optimization only to avoid fetching postings, since the posting
// list would be empty anyway.
emptyValueAllowedByMatchers := true
for _, matcher := range matchers {
if matcher.Name != lblName {
continue
}
if !matcher.Matches("") {
emptyValueAllowedByMatchers = false
break
}
}

if emptyValueAllowedByMatchers {
lblValues = append(lblValues, "")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this would hugely increase the scope of the endpoint when requesting negative matchers (pod!="X"). So the ingester will have to also find all series without the pod label, which means taking the union of all the lists for all values of pod and subtract them from the list of postings for this call.

Is it an option to just subtract the sum of all label values from the number of all series that match the matchers and add this value to the result instead?

}
Comment on lines +139 to +156
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick if you can move this into a tiny function of its own because labelValuesCardinality is becoming long with time


// For each value count total number of series storing the result into cardinality response item.
var respItem *client.LabelValueSeriesCount

Expand Down
14 changes: 10 additions & 4 deletions pkg/ingester/label_names_and_values_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -166,11 +166,11 @@ func TestIngester_LabelValuesCardinality_SentInBatches(t *testing.T) {
require.Equalf(t, maxBatchLabelValues, len(mockServer.SentResponses[0].Items[0].LabelValueSeries), "First label of first response should contain max amount of labels %d", maxBatchLabelValues)

require.Equal(t, 2, len(mockServer.SentResponses[1].Items), "Second server response should contain two label names")
require.Equal(t, 1, len(mockServer.SentResponses[1].Items[0].LabelValueSeries), "First label of second response should contain one value")
require.Equal(t, 2, len(mockServer.SentResponses[1].Items[0].LabelValueSeries), "First label of second response should contain two values")
require.Equalf(t, maxBatchLabelValues-1, len(mockServer.SentResponses[1].Items[1].LabelValueSeries), "Second label of second response should contain %d values", maxBatchLabelValues-1)

require.Equal(t, 1, len(mockServer.SentResponses[2].Items), "Third response should contain only one label")
require.Equal(t, 1, len(mockServer.SentResponses[2].Items[0].LabelValueSeries), "First label of third response should contain one label")
require.Equal(t, 2, len(mockServer.SentResponses[2].Items[0].LabelValueSeries), "First label of third response should contain two values")
}

func TestIngester_LabelValuesCardinality_AllValuesToBeReturnedInSingleMessage(t *testing.T) {
Expand All @@ -194,6 +194,8 @@ func TestIngester_LabelValuesCardinality_AllValuesToBeReturnedInSingleMessage(t
},
expectedItems: []*client.LabelValueSeriesCount{
{LabelName: "label-a", LabelValueSeries: map[string]uint64{"a-0": 1}},
// label-b is not present, but it was requested, so it should be returned with an empty value.
{LabelName: "label-b", LabelValueSeries: map[string]uint64{"": 1}},
},
},
"all values returned in a single message if response size is less then batch size": {
Expand All @@ -206,11 +208,11 @@ func TestIngester_LabelValuesCardinality_AllValuesToBeReturnedInSingleMessage(t
expectedItems: []*client.LabelValueSeriesCount{
{
LabelName: "label-a",
LabelValueSeries: map[string]uint64{"a-0": 1, "a-1": 2, "a-2": 3},
LabelValueSeries: map[string]uint64{"a-0": 1, "a-1": 2, "a-2": 3, "": 3},
},
{
LabelName: "label-b",
LabelValueSeries: map[string]uint64{"b-0": 1, "b-1": 2},
LabelValueSeries: map[string]uint64{"b-0": 1, "b-1": 2, "": 6},
},
},
},
Expand All @@ -226,6 +228,10 @@ func TestIngester_LabelValuesCardinality_AllValuesToBeReturnedInSingleMessage(t
LabelName: "label-a",
LabelValueSeries: map[string]uint64{"a-1": 2, "a-2": 3},
},
{
LabelName: "label-b",
LabelValueSeries: map[string]uint64{"": 5},
},
},
},
}
Expand Down
Loading