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

Adds the ability the fetch label values from the store #1337

Merged
merged 2 commits into from
May 7, 2019

Conversation

cyriltovena
Copy link
Contributor

@cyriltovena cyriltovena commented Apr 18, 2019

Signed-off-by: Cyril Tovena cyril.tovena@gmail.com

This is required to advance grafana/loki#453 in Loki.

I also need to add LabelNames(metricName, start, end) but since this is my first PR I wanted to have a quick feedback on this first.

@bboreham
Copy link
Contributor

Thanks for the PR; it looks promising. This isn't the same as the Prometheus API LabelValues, right? Because that one doesn't have a metric name. I think you should mention that in a comment, and possibly use a different name.

I would probably also split the change into two commits containing the refactor of validateQueryTimeRange() and then the new code.

@cyriltovena
Copy link
Contributor Author

Not exactly like https://prometheus.io/docs/prometheus/latest/querying/api/#querying-label-values as it requires the metric name like you said.

I'll see how I can split the commit. I actually intentionally squash rebased it to a single commit, since I was used to it for other project.

Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
@cyriltovena
Copy link
Contributor Author

Could not find a better name but I did add a documentation.

PTAL

@bboreham
Copy link
Contributor

Given that we have LabelValuesForLabelName() elsewhere, how about LabelValuesForMetricName() here?

@bboreham
Copy link
Contributor

Besides the name, LGTM

Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
@tomwilkie
Copy link
Contributor

Nice work Cyril! LGTM too.

@tomwilkie tomwilkie merged commit b1777a5 into cortexproject:master May 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants