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

Provide optional matchers to LabelValuesForMetricName. #4933

Merged
merged 11 commits into from Dec 20, 2021

Conversation

jeschkies
Copy link
Contributor

@jeschkies jeschkies commented Dec 15, 2021

What this PR does / why we need it:
In some cases we want to fetch label values from a limited set of series. This can be done by passing label matchers for the series to the LabelValuesForMetricName method. The matchers are applied first to filter out all chunk ids that matcher all of them. Then we fetch all label values and filter out those that are part of the chunks.

Checklist

  • Documentation added
  • Tests updated
  • Add an entry in the CHANGELOG.md about the changes.

fooChunk1,
fooChunk2,
}); err != nil {
t.Fatal(err)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test was copied from chunk_store_test.go but fails with series_store_test.go:70: table not found: cortex_chunks2710. Not sure what's going on.

@jeschkies jeschkies marked this pull request as ready for review December 15, 2021 15:50
@jeschkies jeschkies requested a review from a team as a code owner December 15, 2021 15:50
pkg/storage/chunk/chunk_store.go Outdated Show resolved Hide resolved
pkg/storage/chunk/series_store_test.go Outdated Show resolved Hide resolved
@jeschkies
Copy link
Contributor Author

jeschkies commented Dec 16, 2021

The test passes when run individually. However, it fails when all tests are run together. The store.Put() method fails with table not found: cortex_chunks2711. @cyriltovena, would you have an idea where to look? Some test must mutate some state but I cannot figure out which one.

Just checked. If the test runs at the beginning by renaming it aaa_series_store_test.go then it works.

Comment on lines 26 to +27
mtime.NowForce(baseTime)
defer mtime.NowReset()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, this was nasty. Could we inject a time interface instead similar to injecting a logger?

if err != nil {
return nil, err
}
seriesIDsSet := make(map[string]struct{}, len(seriesIDs))
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: not sure using a map will be better. We can take a look at this later.

Copy link
Contributor

@cyriltovena cyriltovena left a comment

Choose a reason for hiding this comment

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

LGTM

jeschkies and others added 2 commits December 20, 2021 13:15
Co-authored-by: Cyril Tovena <cyril.tovena@gmail.com>
@cyriltovena cyriltovena merged commit a5c5f16 into grafana:main Dec 20, 2021
@jeschkies jeschkies deleted the karsten/lbac branch December 20, 2021 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants