Re: [PR] prometheus: fix scrape duration growing unbounded (#13667) [cloudstack]
boring-cyborg[bot] commented on PR #13696: URL: https://github.com/apache/cloudstack/pull/13696#issuecomment-5183006370 Awesome work, congrats on your first merged pull request! -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] prometheus: fix scrape duration growing unbounded (#13667) [cloudstack]
DaanHoogland merged PR #13696: URL: https://github.com/apache/cloudstack/pull/13696 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] prometheus: fix scrape duration growing unbounded (#13667) [cloudstack]
PrashantBhanage commented on PR #13696: URL: https://github.com/apache/cloudstack/pull/13696#issuecomment-5180285478 @DaanHoogland Merged in your rebase, all checks are green now except PR Merge Conflict Check / triage which still shows failing, looks stale since the live conflict status below confirms no conflicts and clean merge. Could you re-run that one when you get a chance? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] prometheus: fix scrape duration growing unbounded (#13667) [cloudstack]
github-actions[bot] commented on PR #13696: URL: https://github.com/apache/cloudstack/pull/13696#issuecomment-5177832552 This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] prometheus: fix scrape duration growing unbounded (#13667) [cloudstack]
PrashantBhanage commented on PR #13696: URL: https://github.com/apache/cloudstack/pull/13696#issuecomment-5090096453 @DaanHoogland One Simulator CI job (test_project_usage / test_protocol_number_security_group) looks hung, stuck on "Start CloudStack Management Server" for 2+ hours while every other job finished normally. Might need a re-run when you get a chance. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] prometheus: fix scrape duration growing unbounded (#13667) [cloudstack]
DaanHoogland commented on code in PR #13696:
URL: https://github.com/apache/cloudstack/pull/13696#discussion_r3655360983
##
plugins/integrations/prometheus/src/main/java/org/apache/cloudstack/metrics/PrometheusExporterServerImpl.java:
##
@@ -108,6 +113,10 @@ public boolean stop() {
httpServer.stop(0);
logger.debug("Stopped Prometheus exporter http server");
}
+if (httpExecutor != null) {
+httpExecutor.shutdownNow();
+logger.debug("Shut down Prometheus exporter http executor");
+}
Review Comment:
let’s not add the thread configurability untill prometheus users complain ;)
thanks @PrashantBhanage
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] prometheus: fix scrape duration growing unbounded (#13667) [cloudstack]
PrashantBhanage commented on code in PR #13696:
URL: https://github.com/apache/cloudstack/pull/13696#discussion_r3650170715
##
plugins/integrations/prometheus/src/main/java/org/apache/cloudstack/metrics/PrometheusExporterServerImpl.java:
##
@@ -108,6 +113,10 @@ public boolean stop() {
httpServer.stop(0);
logger.debug("Stopped Prometheus exporter http server");
}
+if (httpExecutor != null) {
+httpExecutor.shutdownNow();
+logger.debug("Shut down Prometheus exporter http executor");
+}
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] prometheus: fix scrape duration growing unbounded (#13667) [cloudstack]
PrashantBhanage commented on code in PR #13696:
URL: https://github.com/apache/cloudstack/pull/13696#discussion_r3649951306
##
plugins/integrations/prometheus/src/main/java/org/apache/cloudstack/metrics/PrometheusExporterServerImpl.java:
##
@@ -108,6 +113,10 @@ public boolean stop() {
httpServer.stop(0);
logger.debug("Stopped Prometheus exporter http server");
}
+if (httpExecutor != null) {
+httpExecutor.shutdownNow();
+logger.debug("Shut down Prometheus exporter http executor");
+}
Review Comment:
Both fair points:
- `setExecutor(null)`: not functionally needed since `start()` always
creates a fresh `HttpServer` instance (the old one, executor included,
just gets discarded/GC'd — and JDK's `HttpServer` can't be restarted
after `stop()` anyway). But happy to add it defensively, no downside.
- `FixedThreadPool(2)`: kept deliberately small since the TTL guard
means most scrapes now skip `updateMetrics()` 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] prometheus: fix scrape duration growing unbounded (#13667) [cloudstack]
codecov[bot] commented on PR #13696: URL: https://github.com/apache/cloudstack/pull/13696#issuecomment-5077766868 ## [Codecov](https://app.codecov.io/gh/apache/cloudstack/pull/13696?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report :white_check_mark: All modified and coverable lines are covered by tests. :white_check_mark: Project coverage is 4.14%. Comparing base ([`7835881`](https://app.codecov.io/gh/apache/cloudstack/commit/78358812b2b7d11e7c7805322b1d70badbc0cdcb?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`8e3694b`](https://app.codecov.io/gh/apache/cloudstack/commit/8e3694b1184c1192da361a6a5889447cceea82bd?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)). > :exclamation: There is a different number of reports uploaded between BASE (7835881) and HEAD (8e3694b). Click for more details. > > HEAD has 3 uploads less than BASE > >| Flag | BASE (7835881) | HEAD (8e3694b) | >|--|--|--| >|uitests|2|1| >|unittests|2|0| > Additional details and impacted files ```diff @@ Coverage Diff @@ ## ghi13586-prometheusDrainage-20 #13696 +/- ## = - Coverage 16.26%4.14% -12.12% = Files 5667 405 -5262 Lines50073233025 -467707 Branches 60804 5898-54906 = - Hits 81436 1370-80066 + Misses 41018931479 -378710 + Partials 9107 176 -8931 ``` | [Flag](https://app.codecov.io/gh/apache/cloudstack/pull/13696/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | | |---|---|---| | [uitests](https://app.codecov.io/gh/apache/cloudstack/pull/13696/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `4.14% <ø> (ø)` | | | [unittests](https://app.codecov.io/gh/apache/cloudstack/pull/13696/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more. [:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/cloudstack/pull/13696?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache). :loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache). :rocket: New features to boost your workflow: - :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems. - :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] prometheus: fix scrape duration growing unbounded (#13667) [cloudstack]
DaanHoogland commented on code in PR #13696:
URL: https://github.com/apache/cloudstack/pull/13696#discussion_r3649903337
##
plugins/integrations/prometheus/src/main/java/org/apache/cloudstack/metrics/PrometheusExporterServerImpl.java:
##
@@ -108,6 +113,10 @@ public boolean stop() {
httpServer.stop(0);
logger.debug("Stopped Prometheus exporter http server");
}
+if (httpExecutor != null) {
+httpExecutor.shutdownNow();
+logger.debug("Shut down Prometheus exporter http executor");
+}
Review 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] prometheus: fix scrape duration growing unbounded (#13667) [cloudstack]
DaanHoogland commented on code in PR #13696:
URL: https://github.com/apache/cloudstack/pull/13696#discussion_r3649897787
##
plugins/integrations/prometheus/src/main/java/org/apache/cloudstack/metrics/PrometheusExporterServerImpl.java:
##
@@ -108,6 +113,10 @@ public boolean stop() {
httpServer.stop(0);
logger.debug("Stopped Prometheus exporter http server");
}
+if (httpExecutor != null) {
+httpExecutor.shutdownNow();
+logger.debug("Shut down Prometheus exporter http executor");
+}
Review Comment:
should `httpExecutor` also be set to null on `httpServer`?
something like
```suggestion
if (httpExecutor != null) {
httpExecutor.shutdownNow();
logger.debug("Shut down Prometheus exporter http executor");
}
httpServer.setExecutor(null); // ¡Did not test!
```
?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] prometheus: fix scrape duration growing unbounded (#13667) [cloudstack]
PrashantBhanage closed pull request #13678: prometheus: fix scrape duration growing unbounded (#13667) URL: https://github.com/apache/cloudstack/pull/13678 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] prometheus: fix scrape duration growing unbounded (#13667) [cloudstack]
PrashantBhanage commented on PR #13678: URL: https://github.com/apache/cloudstack/pull/13678#issuecomment-5072939431 @DaanHoogland Thanks for catching that. I recreated the branch from ghi13586-prometheusDrainage-20 and opened a clean PR containing only the Prometheus fix (1 commit / 4 files changed). I'll close this PR to avoid the unrelated commits. Thanks! -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] prometheus: fix scrape duration growing unbounded (#13667) [cloudstack]
DaanHoogland commented on PR #13678: URL: https://github.com/apache/cloudstack/pull/13678#issuecomment-5071759387 > > @PrashantBhanage , did you base your changes on my branch from #13311 (ghi13586-prometheusDrainage-20)? If so, can you redirect your PR to that branch (so we can merge them together and handle conflicts early)? > > @DaanHoogland Done! I have switched the base branch of this PR to your ghi13586-prometheusDrainage-20 branch as requested. quote, that went wrong @PrashantBhanage , look at the commit list in this pull. It contains a lot of undesirable extras. also 3000+ files changesd can’t be good. Can you have another look? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] prometheus: fix scrape duration growing unbounded (#13667) [cloudstack]
PrashantBhanage commented on PR #13678: URL: https://github.com/apache/cloudstack/pull/13678#issuecomment-5071516827 > @PrashantBhanage , did you base your changes on my branch from #13311 (ghi13586-prometheusDrainage-20)? If so, can you redirect your PR to that branch (so we can merge them together and handle conflicts early)? @DaanHoogland Done! I have switched the base branch of this PR to your ghi13586-prometheusDrainage-20 branch as requested. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] prometheus: fix scrape duration growing unbounded (#13667) [cloudstack]
DaanHoogland commented on PR #13678: URL: https://github.com/apache/cloudstack/pull/13678#issuecomment-5069034627 @PrashantBhanage , did you base your changes on my branch from #13311 (ghi13586-prometheusDrainage-20)? If so, can you redirect your PR to that branch (so we can merge them together and handle conflicts early)? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
