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

proposal: increase default value for max_samples_per_send #5166

Closed
valyala opened this issue Jan 31, 2019 · 18 comments · Fixed by #5267
Closed

proposal: increase default value for max_samples_per_send #5166

valyala opened this issue Jan 31, 2019 · 18 comments · Fixed by #5267

Comments

@valyala
Copy link

valyala commented Jan 31, 2019

Proposal

Default value for max_samples_per_send - 100 - is too low for any non-idle Prometheus setup with remote_write enabled. It results in too frequent requests to remote storage if Prometheus scrapes more than a few hundred metrics per second. High requests' frequency wastes resources on both Prometheus and remote storage sides, so users have to increase max_samples_per_send after the first attempt to write metrics to remote storage.

It would be great if default value for max_samples_per_send is increased from 100 to 1000 or even 10K. This would simplify remote_write configuration for the majority of users.

@cstyan
Copy link
Member

cstyan commented Feb 1, 2019

Default value for max_samples_per_send - 100 - is too low for any non-idle Prometheus setup with remote_write enabled. It results in too frequent requests to remote storage if Prometheus scrapes more than a few hundred metrics per second.

Can you elaborate? I don't think we're seeing any issues like this with any of our Prometheus instances.

@valyala
Copy link
Author

valyala commented Feb 1, 2019

  • Github users usually use high values for max_samples_per_send. This suggests that the default value is too low.
  • See this and this issue. They contain suggestions for increasing max_samples_per_send to 1000 in order to fix performance issues.

@juliusv
Copy link
Member

juliusv commented Feb 4, 2019

Yeah, 100 seems a bit low.

@bboreham
Copy link
Member

bboreham commented Feb 4, 2019

Another datapoint: Weaveworks customer config is set to 1000.

@valyala a lot of those configs on GitHub also have max_shards: 10000 which suggests they haven't thought this through...

@valyala
Copy link
Author

valyala commented Feb 4, 2019

Yeah, default max_shards should be lowered to appropriate value when increasing default max_samples_per_send value

@beorn-
Copy link

beorn- commented Apr 17, 2019

The system CPU usage was quite high after adding a significant amount of metrics.

We were about to scale the platform and we noticed the CPU system usage was high. After profiling the system we've noticed that the kernel was spending a very significant amount of time handling lookups in established TCP connections kernel hashtable.

After checking things i've seen that in our setup we were pushing 150k datapoints/s which meant 1500 tcp connections/s : Hence a serious amount of TIME_WAIT connections (perfectly normal).

Load was through the roof (more than 30 on a 12 cores server) and rules evaluation time exploded( 30s instead of the usual milliseconds)

We have ended up with

    queue_config:
      capacity: 300000
      max_shards: 100
      max_samples_per_send: 10000

It now works fine with 2-3 load and very good rules evaluations time

One problem still remains. it eats up more memory, if i'm not mistaken and the prometheus code is not really meant for big max_samples_per_send values. Maybe fixing keep-alive through http 1.1 might be a quick win too ?

@beorn-
Copy link

beorn- commented Apr 17, 2019

About keep-alived connections @elwinar and I came up with elwinar@a153ee9

It seems to fix the issue.

@bboreham
Copy link
Member

FYI “http pipelining” means something different to keep-alive, and is not relevant here. See https://en.m.wikipedia.org/wiki/HTTP_pipelining

This doesn’t impact your suggestion; I just like to keep the terminology clear.

@beorn-
Copy link

beorn- commented Apr 17, 2019

I stand corrected. To avoid any unneeded confusion i have edited my past comments. Thanks @bboreham .

elwinar added a commit to elwinar/prometheus that referenced this issue Apr 18, 2019
From the documentation:
> The default HTTP client's Transport may not
> reuse HTTP/1.x "keep-alive" TCP connections if the Body is
> not read to completion and closed.

This effectively enable keep-alive for the fixed requests.

Signed-off-by: Romain Baugue <romain.baugue@elwinar.com>
brian-brazil pushed a commit that referenced this issue Apr 18, 2019
From the documentation:
> The default HTTP client's Transport may not
> reuse HTTP/1.x "keep-alive" TCP connections if the Body is
> not read to completion and closed.

This effectively enable keep-alive for the fixed requests.

Signed-off-by: Romain Baugue <romain.baugue@elwinar.com>
brian-brazil pushed a commit that referenced this issue Apr 24, 2019
From the documentation:
> The default HTTP client's Transport may not
> reuse HTTP/1.x "keep-alive" TCP connections if the Body is
> not read to completion and closed.

This effectively enable keep-alive for the fixed requests.

Signed-off-by: Romain Baugue <romain.baugue@elwinar.com>
@csmarchbanks
Copy link
Member

@tomwilkie I am curious, what are the strong reasons for 1k shards are you mention here?

@cstyan cstyan mentioned this issue Nov 18, 2019
8 tasks
@csmarchbanks
Copy link
Member

We came to this in our bug scrub today. I would happily change defaults to a better value to help many people, but would like to find a value people can agree on to avoid continuously changing it.

Generally, I agree that 100 is too low, however, I have seen systems that break trying to send 1k+ samples per request. I would prefer to have defaults that work for most people than optimize for some use-cases. What would everyone think of a default of 500 samples per request, and max shards of 200 to keep total throughput similar?

@valyala
Copy link
Author

valyala commented Apr 20, 2020

What would everyone think of a default of 500 samples per request, and max shards of 200 to keep total throughput similar?

This sounds reasonable!

@beorn-
Copy link

beorn- commented Apr 20, 2020

Based on a study i've made for our needs. LTS services are handling this (sometimes very) differently based on their architecture.

if the LTS API handles well batching, then reducing max_shards and raising the max_samples_per_send var from 500 up to 1000 was a minimum on all the setups i have tried : victoria metrics, m3, cortex, metrictank (when it supported prom remote_write) and another i can't remember.

i ended up to keep victoria metrics, even though it has some quirks. the configuration is

    queue_config:
      capacity: 300000
      max_shards: 100
      max_samples_per_send: 10000

Not a single problem ever since

@csmarchbanks
Copy link
Member

@beorn- From your above comment, it looks like your comment above of 1500 req/s would have been using 100 max samples per send I would expect 500 to be much better. In addition, significant improvement in sharding has been implemented in the last year, I believe during April of last year was the time where a bug was causing shards to constantly swap between min and max causing all sorts of isues. Would you be willing to try a newer version of Prometheus with 500 max_samples_per_send to get more recent data?

@beorn-
Copy link

beorn- commented Apr 21, 2020

sure. i'll give it a try asap.

NB: we're currently pushing 225k datapoints/s per prom instance

@csmarchbanks
Copy link
Member

@beorn- Any updates on trying 500 samples per send?

@beorn-
Copy link

beorn- commented May 7, 2020

i'll try right away. just popped out of my mind sorry !

@beorn-
Copy link

beorn- commented May 7, 2020

Does not seem to have any effect whatsoever

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

Successfully merging a pull request may close this issue.

7 participants