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

storeAPI: Extend ExternalLabels Selector for LabelsNames and LabelValues #3351

Closed
bwplotka opened this issue Oct 21, 2020 · 7 comments · Fixed by #3566
Closed

storeAPI: Extend ExternalLabels Selector for LabelsNames and LabelValues #3351

bwplotka opened this issue Oct 21, 2020 · 7 comments · Fixed by #3566

Comments

@bwplotka
Copy link
Member

This would be really awesome for further multi-tenancy support with https://github.com/prometheus-community/prom-label-proxy

Since tenants never share the same TSDB block or DB we can easily allow LabelValues and LabelNames API selection support for tenant's data.

The only open question is compatibility. Do we want extra check to ensure if server supports this or not? Without such check we know nothing: The lack of selection might be transparent causing tenant data leak.

cc @pracucci @brancz @yeya24 @kakkoyun

@brancz
Copy link
Member

brancz commented Oct 21, 2020

I think this would be best solved with a combination of prometheus/prometheus#6178 (and implementing the equivalent in Thanos) and support for enforcing a matcher for this API in prom-label-proxy.

@bwplotka
Copy link
Member Author

Nice, the only problem - or rather a potential improvement I can see here is that we can do it much faster IF the selectors match external label. But we can implement that easily indeed.

@brancz
Copy link
Member

brancz commented Oct 22, 2020

Yeah sounds pretty much equivalent to labelset in series queries ("definitely not in set").

@bwplotka
Copy link
Member Author

👍 👍

@yeya24
Copy link
Contributor

yeya24 commented Dec 8, 2020

Hi, I am looking at the implementation at prometheus/prometheus#7496.
For labels query with matchers, it actually send a Series call first and filters the labels from the series sets.
Do we want to do the same thing in Thanos? It should work but is performance a problem (though we support skip-chunk) ?

@brancz
Copy link
Member

brancz commented Dec 9, 2020

@yeya24 have we validated that it is indeed a problem performance wise? I can see what you mean of course, just want to make sure we base this on measurements and not fear :)

@yeya24
Copy link
Contributor

yeya24 commented Dec 9, 2020

@yeya24 have we validated that it is indeed a problem performance wise? I can see what you mean of course, just want to make sure we base this on measurements and not fear :)

You are right and I haven't benchmarked it.
We can do a Series based version first to get this feature working.
After Prometheus supports these matchers in the TSDB level, we can change our code and do the matchers push down to leaf nodes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants