prometheus: fix scrape duration growing unbounded (#13667)#13696
prometheus: fix scrape duration growing unbounded (#13667)#13696PrashantBhanage wants to merge 2 commits into
Conversation
Three changes scoped exclusively to the prometheus exporter plugin: 1. Bounded HTTP executor — give the HttpServer a FixedThreadPool(2) instead of the JDK default single-threaded executor; shut it down cleanly in stop(). 2. TTL guard on updateMetrics() — add a synchronized guard with a last-run timestamp so scrapes arriving faster than the configurable minimum interval (prometheus.exporter.metrics.min.refresh.interval, default 5 s) reuse the previously computed metrics instead of triggering a new recomputation. 3. Timing instrumentation — wrap updateMetrics() body with System.nanoTime() start/end and log elapsed wall-clock time at info level on every run (including exception path), so slow sub-collectors can be identified from logs. Fixes: apache#13667 Ref: apache#13586
| if (httpExecutor != null) { | ||
| httpExecutor.shutdownNow(); | ||
| logger.debug("Shut down Prometheus exporter http executor"); | ||
| } |
There was a problem hiding this comment.
should httpExecutor also be set to null on httpServer?
something like
| if (httpExecutor != null) { | |
| httpExecutor.shutdownNow(); | |
| logger.debug("Shut down Prometheus exporter http executor"); | |
| } | |
| if (httpExecutor != null) { | |
| httpExecutor.shutdownNow(); | |
| logger.debug("Shut down Prometheus exporter http executor"); | |
| } | |
| httpServer.setExecutor(null); // ¡¡¡¡¡Did not test!!!!! |
?
There was a problem hiding this comment.
btw, I genuinely don’t understand this part of the change, I’ll read the co-pilot comment on the original issue as well. So if the executor is shutdown(), can start() still be called on it? (for instance)
And why a newFixedThreadPool of 2?
There was a problem hiding this comment.
Both fair points:
setExecutor(null): not functionally needed sincestart()always
creates a freshHttpServerinstance (the old one, executor included,
just gets discarded/GC'd — and JDK'sHttpServercan't be restarted
afterstop()anyway). But happy to add it defensively, no downside.FixedThreadPool(2): kept deliberately small since the TTL guard
means most scrapes now skipupdateMetrics()entirely — 2 threads is
just enough to stop one slow request blocking others, without
over-engineering. Can make it configurable if you'd prefer.
Will push a follow-up commit nulling out the executor per your suggestion.
There was a problem hiding this comment.
btw, I genuinely don’t understand this part of the change, I’ll read the co-pilot comment on the original issue as well. So if the executor is shutdown(), can start() still be called on it? (for instance) And why a newFixedThreadPool of 2?
Pushed 8637b6d — scoped setExecutor(null) inside the existing
httpServer null-check (your suggested diff had it unconditional, which
would NPE if start() never created a server) and nulled out
httpExecutor too.
DaanHoogland
left a comment
DaanHoogland
left a comment
There was a problem hiding this comment.
thanks @PrashantBhanage your code looks mostly good. some small thing i want to find out before including it in the other PR.
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## ghi13586-prometheusDrainage-20 #13696 +/- ##
=====================================================================
- Coverage 16.26% 4.14% -12.12%
=====================================================================
Files 5667 405 -5262
Lines 500732 33025 -467707
Branches 60804 5898 -54906
=====================================================================
- Hits 81436 1370 -80066
+ Misses 410189 31479 -378710
+ Partials 9107 176 -8931
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Scope httpServer.setExecutor(null) inside the existing httpServer null-check to avoid a NullPointerException if start() never created a server (e.g. exporter disabled, or the IOException path was taken). Also null out the httpExecutor field after shutdown for defensive hygiene, per review feedback on PR apache#13696.
2 participants
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
N/A — backend-only change, no UI impact.
How Has This Been Tested?
How did you try to break this feature and the system with this change?
/metricsscrape requests to verify the bounded executor and synchronization prevent overlapping recomputation.stop().prometheus.exporter.metrics.min.refresh.intervalat runtime changes the refresh behavior without requiring a restart.