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

LowResourceMonitor.getReasons should include detailed reason instead of hard-coded message #9337

Closed
jluehe opened this issue Feb 9, 2023 · 1 comment · Fixed by #9338
Closed
Labels
Bug For general bugs on Jetty side

Comments

@jluehe
Copy link
Contributor

jluehe commented Feb 9, 2023

Jetty version(s)
10.0.13

Description
LowResourceMonitor.getReasons returns a hard-coded string, namely the string representation of the LowResourceCheck that is low on resources. It should contain the detailed low-resource reason instead, which would be more useful.

For example, it should return Server low on threads: x, idleThreads: y vs the current Check if the ThreadPool from monitored connectors are lowOnThreads and if all connections number is higher than the allowed maxConnection.

Proposed diffs:

diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/LowResourceMonitor.java b/jetty-server/src/main/java/org/eclipse/jetty/server/LowResourceMonitor.java
index 07c7ab427d..7529404b17 100644
--- a/jetty-server/src/main/java/org/eclipse/jetty/server/LowResourceMonitor.java
+++ b/jetty-server/src/main/java/org/eclipse/jetty/server/LowResourceMonitor.java
@@ -280,7 +280,7 @@ public class LowResourceMonitor extends ContainerLifeCycle
         {
             if (lowResourceCheck.isLowOnResources())
             {
-                reasons = lowResourceCheck.toString();
+                reasons = lowResourceCheck.getReason();
                 break;
             }
         }

If needed, the LowResourceCheck string representation could be appended to the reason.

@jluehe
Copy link
Contributor Author

jluehe commented Feb 9, 2023

On a separate note, it looks like even if multiple LowResourceChecks are low on resources, only one of them will contribute to the LowResourceMonitor reasons (plural): https://github.com/eclipse/jetty.project/blob/jetty-10.0.13/jetty-server/src/main/java/org/eclipse/jetty/server/LowResourceMonitor.java#L279-L286. Perhaps they should all be concatenated?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For general bugs on Jetty side
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant