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

Remote storage should not be used by federation #4270

Open
iksaif opened this Issue Jun 14, 2018 · 0 comments

Comments

Projects
None yet
2 participants
@iksaif
Copy link
Contributor

iksaif commented Jun 14, 2018

Capturing IRC discussion:

10:52 < iksaif> hey, do we really want /federate to use remote read?
13:18 < bbrazil> I'm not sure what the current behaviour is, but it should not
13:26 < jrv> turns out this problem exists since 2.0.0... adding a remote_read endpoint there and then hitting /federate causes a nil pointer crash as well
13:27 < jrv> in 1.x I handed down some special parameters to a query that indicated that only local storage should be consulted (for rules and federation), but that didn't make it into 2.x
13:30 < jrv> in 1.x it was done like this: https://github.com/prometheus/prometheus/blob/release-1.8/cmd/prometheus/main.go#L129
13:48 < jrv> In 2.x, would such a (only use local storage) hint now go into the `SelectParams`?
13:53 < bbrazil> jrv: no, you should use a storage that doesn't include the remote ones
13:53 < bbrazil> so you'd just be using the local storage rather than a fanout storage that included everything
13:54 < jrv> bbrazil: ok, so we should hand in two storages into the web layer, a local-only one for federation and a combined one for normal queries
13:55 < jrv> but even for normal queries at some point we want the user to be able to set an argument that indicates whether to also hit remote storage or not... same for rules. Might be cleaner to abstract 
             that away into a query option and a common place (like fanin in 1.x) then decides which one of the underlying queriers to use?
13:56 < jrv> so we don't put that logic and hand out two storages into each of the places that have to make this decision
13:56 < bbrazil> jrv: something like that, but it doesn't belong as an option to the storage itself
13:56 < bbrazil> we've only 2 places that need this, no need to get too fancy
13:58 < jrv> bbrazil: yeah, not to the storage itself, but the fanin layer above it then
13:58 < jrv> like in 1.x
13:59 < bbrazil> I see us passing in two storages, and chosing which to use in the api/rules code
14:02 < jrv> not sure how that's better, but works too
14:02 < jrv> I think from a cleanliness perspective I prefer passing in the fanin one and making that aware of what's local and what's remote
14:09 < bbrazil> to me that's making it harder to dig down to find the relevant if statement
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.