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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Logiraptor
Copy link
Contributor

@Logiraptor Logiraptor commented Jun 20, 2024

What this PR does

When querying for cardinality by some label, the result may not add up to the total cardinality. This is because some series may be missing that label entirely. Instead, this change will include the empty value in the result. As an optimization, the empty value is skipped if it would be filtered out by matchers.

Which issue(s) this PR fixes or relates to

Reported as a nice-to-have for the cardinality dashboards, and it's a trivial change so why not 😃

Checklist

  • Tests updated.
  • Documentation added.
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX].
  • about-versioning.md updated with experimental features.

When querying for cardinality by some label, the
result may not add up to the total cardinality.
This is because some series may be missing that
label entirely. Instead, this change will include
the empty value in the result. As an optimization,
the empty value is skipped if it would be filtered
out by matchers.
@Logiraptor Logiraptor marked this pull request as ready for review June 20, 2024 20:07
@Logiraptor Logiraptor requested a review from a team as a code owner June 20, 2024 20:07
CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Mauro Stettler <[email protected]>
Copy link
Contributor

@replay replay left a comment

Choose a reason for hiding this comment

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

thx

@Logiraptor Logiraptor enabled auto-merge (squash) June 20, 2024 20:11
}

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

// 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.

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

@dimitarvdimitrov
Copy link
Contributor

The CHANGELOG has just been cut to prepare for the next Mimir release. Please rebase main and eventually move the CHANGELOG entry added / updated in this PR to the top of the CHANGELOG document. Thanks!

Copy link
Contributor

github-actions bot commented Jan 7, 2025

Thank you for your contribution. This pull request has been marked as stale because it has had no activity in the last 150 days. It will be closed in 30 days if there is no further activity. If you need more time, you can add a comment to the PR.

@github-actions github-actions bot added the stale label Jan 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants