Skip to content

Don't create one ingester write request per rule-generated sample #569

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

Closed
juliusv opened this issue Sep 24, 2017 · 5 comments · Fixed by #583
Closed

Don't create one ingester write request per rule-generated sample #569

juliusv opened this issue Sep 24, 2017 · 5 comments · Fixed by #583

Comments

@juliusv
Copy link
Contributor

juliusv commented Sep 24, 2017

See:

https://github.com/weaveworks/cortex/blob/7c1475b7878b2f7a913ef011c90e4b1b7ba2dbe3/pkg/ruler/compat.go#L28

which calls into...

https://github.com/weaveworks/cortex/blob/7c1475b7878b2f7a913ef011c90e4b1b7ba2dbe3/pkg/distributor/distributor.go#L276

Every sample generated from a rule currently gets sent as an individual write request to the ingesters, which must be super inefficient. We should batch up samples from a rule before sending them out.

This might be easier after/with #555 as the Prometheus 2.0 storage.Appender interface has a Commit() method, upon which we could flush. Commit is called once per rule, so the number of samples buffered up in an appender wouldn't get too huge.

@rade
Copy link

rade commented Sep 24, 2017

must be super inefficient.

Is it? The ingesters themselves do batching. And the network connections to them should all be persistent. So I doubt there's a big difference between the ruler sending individual samples and batching them.

It's still worth making that change, of course, especially when it doesn't take much code.

@juliusv
Copy link
Contributor Author

juliusv commented Sep 24, 2017

Sure, I haven't measured it, so my intuition may be off here. But: Even with persistent connections, the per-request overhead seems high. The whole code in https://github.com/weaveworks/cortex/blob/7c1475b7878b2f7a913ef011c90e4b1b7ba2dbe3/pkg/distributor/distributor.go#L276 has to be processed per sample instead of per batch, each sample gets packaged in a request, and each sample also needs a round trip between ruler and ingester before we send the next sample.

@rade
Copy link

rade commented Sep 24, 2017

each sample also needs a round trip between ruler and ingester before we send the next sample.

Ouch, that's terrible. You have convinced me.

@jml
Copy link
Contributor

jml commented Sep 25, 2017

This is unfortunate, since I'd like to make the ruler less stateful rather than more (c.f. #310 (comment))

@juliusv
Copy link
Contributor Author

juliusv commented Sep 25, 2017

I don't think this is a concern in terms of ruler statefulness. We get all the samples of a rule evaluation at the same time anyway, so we may as well send them as one request instead of N. Nothing we need to persist over a long time. I'm already doing that for my Prometheus 2.0 port, where Add() just adds samples to the appender internally and Commit() sends them out.

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 a pull request may close this issue.

3 participants