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.

Sign up for GitHub

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

Jump to bottom

Reduce number of core threads in HTTPServer to one #786

Merged
merged 1 commit into from
May 19, 2022
Merged

Reduce number of core threads in HTTPServer to one #786

merged 1 commit into from
May 19, 2022

Conversation

fstab
Copy link
Member

@fstab fstab commented May 19, 2022

In a typical scenario with a Prometheus server scraping every 15 seconds, one thread should be enough in HTTPServer. Reduce the number of core threads from 5 to 1.

@fstab
Reduce number of core threads in HTTPServer to one
c0bf6d9 
Signed-off-by: Fabian Stäber <fabian@fstab.de>
@fstab fstab merged commit 2f31b96 into master May 19, 2022
@fstab fstab deleted the core-threads branch May 19, 2022 21:44
@fstab fstab mentioned this pull request May 19, 2022
Closed
@brian-brazil
Copy link
Contributor

brian-brazil commented May 20, 2022 via email

@fstab
Copy link
Member Author

fstab commented May 20, 2022 •
edited

Thanks for keeping an eye on PRs! In this case I think there is just a misunderstanding with the weird terminology of ThreadPoolExecutor:

  • corePoolSize: the number of threads to keep in the pool, even if they are idle
  • maximumPoolSize the maximum number of threads to allow in the pool

This PR sets the corePoolSize to 1, but leaves the maximumPoolSize at 5. It's still possible to run 5 requests in parallel, but the executor won't keep 5 threads running when they're idle.

@brian-brazil
Copy link
Contributor

Thanks for the clarification, that makes perfect sense to do.

@michaeljmarshall michaeljmarshall mentioned this pull request Jul 14, 2022
Merged
1 task
@shoothzj shoothzj mentioned this pull request Jul 14, 2022
Merged
1 task
shoothzj added a commit to apache/pulsar that referenced this pull request Jul 14, 2022
@shoothzj
Bump prometheus client version from 0.15.0 to 0.16.0 (#16591)
948000b 
### Motivation
prometheus client 0.16.0 contains some approvements that we can benefit from. Thanks for @dave2wave @michaeljmarshall  the reminder and pointing out.

> [ENHANCEMENT] Reduce the number of core threads in HTTPServer from 5 to 1. The HTTPServer will still start up to 5 threads on demand if there are parallel requests, but it will use only 1 thread as long as requests are sequential (prometheus/client_java#786).
[ENHANCEMENT] Optimize metric name sanitization: Replace the regular expression with a hard-coded optimized algorithm to improve performance (prometheus/client_java#777). Thanks @fwbrasil

See https://github.com/prometheus/client_java/releases

### Modifications

Bump prometheus client version from 0.15.0 to 0.16.0

### Documentation

Check the box below or label this PR directly.

Need to update docs? 
  
- [x] `doc-not-needed` 
dependency updates, no need doc
amuraru added a commit to amuraru/jmx_exporter that referenced this pull request Jul 20, 2022
@amuraru
Increased http server executor size to address connectivity issues
6b0a4ce 
prometheus/client_java#786 reduced the core pool
size to 1 for the http executor size.
This had adverse effects in our environment leading to connectivity issues to metrics port

This patch overrides that behaviour reverting back to 5 persistent threads in the executor pool
@ctrlaltluc
Copy link

ctrlaltluc commented Jul 21, 2022 •
edited

@fstab we have a setup using the Prometheus client including this change, and we started to experience some inconveniences.

Our setup is Kafka on Kubernetes, running prometheus/jmx_exporter as an agent attached to the Kafka process. Our scrape duration is about 12 seconds.
The Kafka brokers are fronted by Envoy reverse proxies, which have periodic health-checks (with 1s timeouts) calling the /-/healthy endpoint provided by the JMX exporter.

After upgrading the Prometheus client to include this fix, we started experiencing health-check network timeouts. Investigations showed us that the latency when calling /-/healthy increases from 10-60ms (with the core pool size of 5) to 10ms-15s (with the core pool size of 1). Since this change is the only one causing these timeouts, we attribute this increase to the overhead of creating a new thread, if the pool has no available one to serve /-/healthy.

We normally thought about increasing the timeout, which is one of the options beside rolling back the Prometheus Java client version to 0.16.
I just wanted to raise this here, because the latency distribution tail increased from under 100ms to 15s seems pretty big.

@fstab
Copy link
Member Author

fstab commented Jul 22, 2022

@ctrlaltluc thanks a lot for pointing this out. This was a bug. Apparently the description of corePoolSize and maximumPoolSize is misleading, as explained in this Blog post.

I pushed a fix, switching to a cached thread pool executor.

@ctrlaltluc
Copy link

@fstab nice guy, Sun!

This explains the behavior of /-/healthy timing out on our side too. The health-checks were served, but when the core thread managed to get the task from the queue.

Thanks!

Jason918 pushed a commit to apache/pulsar that referenced this pull request Sep 2, 2022
@shoothzj @Jason918
Bump prometheus client version from 0.15.0 to 0.16.0 (#16591)
69332a0 
### Motivation
prometheus client 0.16.0 contains some approvements that we can benefit from. Thanks for @dave2wave @michaeljmarshall  the reminder and pointing out.

> [ENHANCEMENT] Reduce the number of core threads in HTTPServer from 5 to 1. The HTTPServer will still start up to 5 threads on demand if there are parallel requests, but it will use only 1 thread as long as requests are sequential (prometheus/client_java#786).
[ENHANCEMENT] Optimize metric name sanitization: Replace the regular expression with a hard-coded optimized algorithm to improve performance (prometheus/client_java#777). Thanks @fwbrasil

See https://github.com/prometheus/client_java/releases

### Modifications

Bump prometheus client version from 0.15.0 to 0.16.0

### Documentation

Check the box below or label this PR directly.

Need to update docs?

- [x] `doc-not-needed`
dependency updates, no need doc

(cherry picked from commit 948000b)
amuraru added a commit to amuraru/jmx_exporter that referenced this pull request Sep 13, 2022
@amuraru
Increased http server executor size to address connectivity issues
a62a253 
prometheus/client_java#786 reduced the core pool
size to 1 for the http executor size.
This had adverse effects in our environment leading to connectivity issues to metrics port

This patch overrides that behaviour reverting back to 5 persistent threads in the executor pool
amuraru added a commit to amuraru/jmx_exporter that referenced this pull request Jul 30, 2023
@amuraru
Increased http server executor size to address connectivity issues
b4c7d43 
prometheus/client_java#786 reduced the core pool
size to 1 for the http executor size.
This had adverse effects in our environment leading to connectivity issues to metrics port

This patch overrides that behaviour reverting back to 5 persistent threads in the executor pool
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@fstab @brian-brazil @ctrlaltluc