Added DefaultCpuHealthChecker #4673

Bue-von-hon · 2023-02-14T14:55:26Z

Motivation:

Resolves part of #1854

Some users may want to use CPU usage to determine if their instance is healthy.
From now on, Users can perform health checks in the following ways

HealthChecker cpuHealthChecker = HealthChecker.ofCpu(80, 80);
cpuHealthChecker.isHealthy();

Modifications:

Added DefaultCpuHealthChecker that checks cpu-usage by OperatingSystemMXBean

Result:

A User can health-check with system-cpu-usage

p.s.
Implemented this by referencing ProcessorMetrics.java in the micrometer core.

Motivation: Resolves part of line#1854 Some users may want to use CPU usage to determine if their instance is healthy. From now on, Users can perform health checks in the following ways ``` DefaultCpuHealthChecker cpuHealthChecker = new DefaultCpuHealthChecker(20.0); cpuHealthChecker.isHealthy(); ``` Modifications: - Added DefaultCpuHealthChecker that checks cpu-usage by OperatingSystemMXBean Result: A User can health-check with system-cpu-usage p.s. Implemented this by referencing ProcessorMetrics.java in the micrometer core.

jrhee17

Looks good overall! Left some minor comments

jrhee17 · 2023-02-15T01:25:59Z

core/src/main/java/com/linecorp/armeria/server/healthcheck/DefaultCpuHealthChecker.java

+import java.lang.reflect.Method;
+import java.util.Arrays;
+import java.util.List;
+


Can you also leave a link where this class was forked from?

Suggested change

// Forked from https://github.com/micrometer-metrics/micrometer/blob/8339d57bef8689beb8d7a18b429a166f6595f2af/micrometer-core/src/main/java/io/micrometer/core/instrument/binder/system/ProcessorMetrics.java

jrhee17 · 2023-02-15T01:45:23Z

core/src/main/java/com/linecorp/armeria/server/healthcheck/DefaultCpuHealthChecker.java

+import java.util.Arrays;
+import java.util.List;
+
+public class DefaultCpuHealthChecker implements HealthChecker{


Can we also add integration tests for this class?

I'm struggling with writing tests.😭
To do the test, need to modify the CPU usage of the system and it is very difficult.
I'm considering passing a Method class through the constructor.(Dependency Injection with @VisibleForTesting annotation).
p.s.
I looked at micrometer's ProcessorMetricsTest.java and spring-boot's AvailabilityStateHealthIndicatorTests.java but didn't get much help.😭

To do the test, need to modify the CPU usage of the system and it is very difficult.

I don't think this is necessary 😅 I think just verifying that DefaultCpuHealthChecker gathers system cpu usage information is enough.
We may expose targetCpuUsage, targetProcessCpuLoad (with @VisibleForTesting) and check that they are not NaN for example.

jrhee17 · 2023-02-15T02:03:15Z

core/src/main/java/com/linecorp/armeria/server/healthcheck/DefaultCpuHealthChecker.java

+
+    private final double targetCpuUsage;
+
+    public DefaultCpuHealthChecker(final double targetCpuUsage) {


Should we accept arguments for both targetSystemCpuLoad and targetProcessCpuLoad so that users can configure limits separately?

core/src/main/java/com/linecorp/armeria/server/healthcheck/DefaultCpuHealthChecker.java

jrhee17 · 2023-02-15T02:03:59Z

core/src/main/java/com/linecorp/armeria/server/healthcheck/DefaultCpuHealthChecker.java

+        }
+    }
+
+    private Class<?> getFirstClassFound(final List<String> classNames) {


Suggested change

private Class<?> getFirstClassFound(final List<String> classNames) {

@Nullable

private Class<?> getFirstClassFound(final List<String> classNames) {

Modifications: - Fixed DefaultCpuHealthChecker added copyright header, added javadocs, Change health check from range-based to ratio-based, added nullable annotations, added requireNonNull method for nullable method calls

Bue-von-hon

@jrhee17 Fixed based on a code review!

p.s.
Another PR on the same issue commented on the ratio-based.
I thought it was reasonable so applied it!

Bue-von-hon · 2023-02-28T10:44:32Z

core/src/main/java/com/linecorp/armeria/server/healthcheck/DefaultCpuHealthChecker.java

+import java.lang.reflect.Method;
+import java.util.Arrays;
+import java.util.List;
+


Bue-von-hon · 2023-02-28T10:48:39Z

core/src/main/java/com/linecorp/armeria/server/healthcheck/DefaultCpuHealthChecker.java

+
+    private final double targetCpuUsage;
+
+    public DefaultCpuHealthChecker(final double targetCpuUsage) {


Bue-von-hon · 2023-02-28T15:07:30Z

core/src/main/java/com/linecorp/armeria/server/healthcheck/DefaultCpuHealthChecker.java

+import java.util.Arrays;
+import java.util.List;
+
+public class DefaultCpuHealthChecker implements HealthChecker{


I'm struggling with writing tests.😭
To do the test, need to modify the CPU usage of the system and it is very difficult.
I'm considering passing a Method class through the constructor.(Dependency Injection with @VisibleForTesting annotation).
p.s.
I looked at micrometer's ProcessorMetricsTest.java and spring-boot's AvailabilityStateHealthIndicatorTests.java but didn't get much help.😭

core/src/main/java/com/linecorp/armeria/server/healthcheck/DefaultCpuHealthChecker.java

Bue-von-hon · 2023-02-28T15:20:49Z

core/src/main/java/com/linecorp/armeria/server/healthcheck/DefaultCpuHealthChecker.java

+        }
+    }
+
+    private Class<?> getFirstClassFound(final List<String> classNames) {


added ending period into first sentence of Javadoc

core/src/main/java/com/linecorp/armeria/server/healthcheck/DefaultCpuHealthChecker.java

jrhee17 · 2023-03-06T09:27:49Z

core/src/main/java/com/linecorp/armeria/server/healthcheck/DefaultCpuHealthChecker.java

+        this.targetCpuUsage = (double) cpuUsage / cpuIdle;
+        this.targetProcessCpuLoad = (double) processCpuUsage / processCpuIdle;


Question) I'm unsure why we're dividing usage by idleness. Can't we just receive two doubles targetCpuUsage, targetProcessCpuLoad as parameters?

Using two is more readable. fixed it!

core/src/main/java/com/linecorp/armeria/server/healthcheck/DefaultCpuHealthChecker.java

jrhee17 · 2023-03-06T09:35:12Z

core/src/main/java/com/linecorp/armeria/server/healthcheck/DefaultCpuHealthChecker.java

+    public DefaultCpuHealthChecker(final int cpuUsage, final int cpuIdle,
+                                   final int processCpuUsage, final int processCpuIdle) {


Instead of exposing this public constructor, can we use a static factory method to instantiate this checker?

Suggested change

public DefaultCpuHealthChecker(final int cpuUsage, final int cpuIdle,

final int processCpuUsage, final int processCpuIdle) {

private DefaultCpuHealthChecker(final int cpuUsage, final int cpuIdle,

final int processCpuUsage, final int processCpuIdle) {

@jrhee17 i Fixed it! And moved the factory method I created earlier to DefaultCpuHealthChecker.

core/src/main/java/com/linecorp/armeria/server/healthcheck/DefaultCpuHealthChecker.java

trustin · 2023-03-07T11:10:18Z

core/src/main/java/com/linecorp/armeria/server/healthcheck/DefaultCpuHealthChecker.java

+/**
+ * Forked from <a href="https://github.com/micrometer-metrics/micrometer/blob/8339d57bef8689beb8d7a18b429a166f6595f2af/micrometer-core/src/main/java/io/micrometer/core/instrument/binder/system/ProcessorMetrics.java">ProcessorMetrics.java</a> in the micrometer core.
+ */


This shouldn't be part of Javadoc. Could you use // instead?

Sure. Fixed!

core/src/main/java/com/linecorp/armeria/server/healthcheck/DefaultCpuHealthChecker.java

ikhoon · 2023-03-09T15:25:05Z

core/src/main/java/com/linecorp/armeria/server/healthcheck/DefaultCpuHealthChecker.java

+
+    private double invoke(final Method method) {
+        try {
+            return (double) method.invoke(operatingSystemBean);


This method will be frequently invoked whenever HealthCheckService receives a request, hence MethodHandles might be a better choice.

Using the lookup#unreflect method, extracted the MethodHandle object from the Method class!

core/src/main/java/com/linecorp/armeria/server/healthcheck/DefaultCpuHealthChecker.java

ikhoon · 2023-03-09T15:30:29Z

core/src/main/java/com/linecorp/armeria/server/healthcheck/DefaultCpuHealthChecker.java

+/**
+ * Forked from <a href="https://github.com/micrometer-metrics/micrometer/blob/8339d57bef8689beb8d7a18b429a166f6595f2af/micrometer-core/src/main/java/io/micrometer/core/instrument/binder/system/ProcessorMetrics.java">ProcessorMetrics.java</a> in the micrometer core.
+ */
+public class DefaultCpuHealthChecker implements HealthChecker {


If we hide the implementation from the public API, we can get more freedom on changes.
Should we add a factory method of this class to HealthChecker and make this package-private?

// The simplest one. HealthChecker.ofCpu(cpuUsage); // A complex configuation. HealthChecker.ofCpu(cpuUsage, cpuIdle...);

Aha, so let's make this class pakage-private and add a factory method to HealthChecker!

…aultCpuHealthChecker.java Co-authored-by: jrhee17 <[email protected]>

…aultCpuHealthChecker.java Co-authored-by: Ikhun Um <[email protected]>

…aultCpuHealthChecker.java Co-authored-by: jrhee17 <[email protected]>

…aultCpuHealthChecker.java Co-authored-by: Trustin Lee <[email protected]>

…aultCpuHealthChecker.java Co-authored-by: jrhee17 <[email protected]>

…est class for cpuHealthChecker, factory method into HealthChecker Motivation: Apply readability and performance improvements suggested in code reviews. Added comment on public method. Added test for DefaultCpuHealthChecker. Modifications: - Fixed DefaultCpuHealthChecker#invoke method to use methodHandlers instead of reflection for better performance. - Fixed a complex constructor to be more readable. - Fixed the systemCpuUsage and processCpuUsage's access modifier for testing. - Added javadoc for public method in DefaultCpuHealthChecker. - Added Factory Method into HealthChecker. - Added test class for DefaultCpuHealthChecker. Result: better performing DefaultCpuHealthChecker and tests.

Bue-von-hon

@jrhee17 I sent a new commit based on the code review.
Hope you like it🙂

Bue-von-hon · 2023-03-11T06:01:49Z

core/src/main/java/com/linecorp/armeria/server/healthcheck/DefaultCpuHealthChecker.java

+/**
+ * Forked from <a href="https://github.com/micrometer-metrics/micrometer/blob/8339d57bef8689beb8d7a18b429a166f6595f2af/micrometer-core/src/main/java/io/micrometer/core/instrument/binder/system/ProcessorMetrics.java">ProcessorMetrics.java</a> in the micrometer core.
+ */


Sure. Fixed!

Bue-von-hon · 2023-03-11T06:19:00Z

core/src/main/java/com/linecorp/armeria/server/healthcheck/DefaultCpuHealthChecker.java

+        this.targetCpuUsage = (double) cpuUsage / cpuIdle;
+        this.targetProcessCpuLoad = (double) processCpuUsage / processCpuIdle;


Using two is more readable. fixed it!

Bue-von-hon · 2023-03-11T07:36:16Z

core/src/main/java/com/linecorp/armeria/server/healthcheck/DefaultCpuHealthChecker.java

+/**
+ * Forked from <a href="https://github.com/micrometer-metrics/micrometer/blob/8339d57bef8689beb8d7a18b429a166f6595f2af/micrometer-core/src/main/java/io/micrometer/core/instrument/binder/system/ProcessorMetrics.java">ProcessorMetrics.java</a> in the micrometer core.
+ */
+public class DefaultCpuHealthChecker implements HealthChecker {


Aha, so let's make this class pakage-private and add a factory method to HealthChecker!

Bue-von-hon · 2023-03-11T07:38:39Z

core/src/main/java/com/linecorp/armeria/server/healthcheck/DefaultCpuHealthChecker.java

+
+    private double invoke(final Method method) {
+        try {
+            return (double) method.invoke(operatingSystemBean);


Using the lookup#unreflect method, extracted the MethodHandle object from the Method class!

Modification: - Move the static factory method to DefaultCpuHealthChecker.

Bue-von-hon · 2023-05-13T12:08:42Z

Gentle Ping @trustin @ikhoon @jrhee17

core/src/main/java/com/linecorp/armeria/server/healthcheck/DefaultCpuHealthChecker.java

core/src/test/java/com/linecorp/armeria/server/healthcheck/DefaultCpuHealthCheckerTest.java

…HealthChecker.java Co-authored-by: jrhee17 <[email protected]>

Bue-von-hon

I misunderstood your comment about testing.
I've rewritten the test correctly.
PTAL @jrhee17

core/src/main/java/com/linecorp/armeria/server/healthcheck/CpuHealthChecker.java

Bue-von-hon · 2024-03-07T00:42:48Z

@jrhee17 gentle ping

Bue-von-hon · 2024-03-15T05:32:18Z

@jrhee17 gentle ping

Bue-von-hon · 2024-03-19T05:17:44Z

@jrhee17 gentle ping

jrhee17

Sorry about the delay 😅 Looks like almost the last round for me!

core/src/main/java/com/linecorp/armeria/server/healthcheck/CpuHealthChecker.java

jrhee17 · 2024-03-19T06:31:07Z

core/src/main/java/com/linecorp/armeria/server/healthcheck/CpuHealthChecker.java

+        return null;
+    }
+
+    static CpuHealthChecker of(double targetCpuUsage, double targetProcessCpuLoad) {


What do you think of:

Moving this to HealthChecker.java

Renaming this method from of to ofCpu

Making this method public so that users can actually use this class

Adding javadocs explaining what this health checker does, and what each parameter does

I saw this comment late:

#4673 (comment)

I'm fine if this is handled separately, I would just like to note that users won't be able to access this method since it is not public

ikhoon

It looks great once @jrhee17 comments are addressed.
Thanks for your effort on this work! ❤️🚀

…HealthChecker.java Co-authored-by: jrhee17 <[email protected]>

Motivation: Add a factory method so that users can actually use the CPU health checker. Modification: - Fix(HealthChecker): added a factory method. - Fix(CpuHealthChecker): removed factory method and added a little more clarity in the description. - Fix(CpuHealthCheckerTest): renamed the method and modified it to use the newly created factory method. Result: Users will be able to use the newly created health checker, and a detailed description will improve usability.

Bue-von-hon · 2024-03-22T05:59:37Z

@jrhee17 I've addressed everything you commented on.

The fact that the factory method was mentioned twice, I felt it was important. I implemented it by adding a static method to the interface. It was the best I could do, as a way to keep compatibility across subclasses as much as possible.

p.s.
I'm concerned about the class(CpuHealthChecker) being exposed outside of its intended scope. The IDE is also warning me about this.

jrhee17

Added a minor commit mostly fixing javadocs, but looks good to me! Thanks for your patience @Bue-von-hon ! 🙇 👍 🙇

minwoox · 2024-03-27T05:55:38Z

Thanks a lot for your patience, @Bue-von-hon!

Bue-von-hon requested review from ikhoon, jrhee17, minwoox and trustin as code owners February 14, 2023 14:55

Bue-von-hon mentioned this pull request Feb 14, 2023

More HealthChecker implementations #1854

Open

jrhee17 added this to the 1.23.0 milestone Feb 15, 2023

jrhee17 added the new feature label Feb 15, 2023

jrhee17 reviewed Feb 15, 2023

View reviewed changes

Fixed javadoc and lint based on code review

08c706e

Modifications: - Fixed DefaultCpuHealthChecker added copyright header, added javadocs, Change health check from range-based to ratio-based, added nullable annotations, added requireNonNull method for nullable method calls

Bue-von-hon commented Feb 28, 2023

View reviewed changes

Fix Lint

551263a

added ending period into first sentence of Javadoc

jrhee17 reviewed Mar 6, 2023

View reviewed changes

trustin reviewed Mar 7, 2023

View reviewed changes

ikhoon reviewed Mar 9, 2023

View reviewed changes

Bue-von-hon and others added 8 commits March 11, 2023 14:58

Update core/src/main/java/com/linecorp/armeria/server/healthcheck/Def…

afd1843

…aultCpuHealthChecker.java Co-authored-by: jrhee17 <[email protected]>

Update core/src/main/java/com/linecorp/armeria/server/healthcheck/Def…

5e5069e

…aultCpuHealthChecker.java Co-authored-by: Ikhun Um <[email protected]>

Update core/src/main/java/com/linecorp/armeria/server/healthcheck/Def…

e4132b4

…aultCpuHealthChecker.java Co-authored-by: jrhee17 <[email protected]>

Update core/src/main/java/com/linecorp/armeria/server/healthcheck/Def…

35da7bc

…aultCpuHealthChecker.java Co-authored-by: jrhee17 <[email protected]>

Update core/src/main/java/com/linecorp/armeria/server/healthcheck/Def…

a0c04bb

…aultCpuHealthChecker.java Co-authored-by: jrhee17 <[email protected]>

Update core/src/main/java/com/linecorp/armeria/server/healthcheck/Def…

dd68de9

…aultCpuHealthChecker.java Co-authored-by: jrhee17 <[email protected]>

Update core/src/main/java/com/linecorp/armeria/server/healthcheck/Def…

8934066

…aultCpuHealthChecker.java Co-authored-by: Trustin Lee <[email protected]>

Update core/src/main/java/com/linecorp/armeria/server/healthcheck/Def…

32fd4f7

…aultCpuHealthChecker.java Co-authored-by: jrhee17 <[email protected]>

jrhee17 modified the milestones: 1.23.0, 1.24.0 Mar 17, 2023

Bue-von-hon commented Mar 18, 2023

View reviewed changes

Fix Lint, Construct method's access level

3b1770b

Modification: - Move the static factory method to DefaultCpuHealthChecker.

trustin requested changes May 30, 2023

View reviewed changes

ikhoon modified the milestones: 1.27.0, 1.28.0 Jan 16, 2024

Update core/src/main/java/com/linecorp/armeria/server/healthcheck/Cpu…

f6ef458

…HealthChecker.java Co-authored-by: jrhee17 <[email protected]>

Bue-von-hon marked this pull request as draft January 30, 2024 23:08

Bue-von-hon and others added 4 commits January 31, 2024 08:09

Update core/src/main/java/com/linecorp/armeria/server/healthcheck/Cpu…

a766919

…HealthChecker.java Co-authored-by: jrhee17 <[email protected]>

Merge branch 'main' into cpu-based-health-checker

f469ac7

Resolves code review

fad1155

Rewrite the test correctly

29048d3

Bue-von-hon commented Feb 26, 2024

View reviewed changes

core/src/main/java/com/linecorp/armeria/server/healthcheck/CpuHealthChecker.java Show resolved Hide resolved

Bue-von-hon marked this pull request as ready for review February 26, 2024 07:37

jrhee17 reviewed Mar 19, 2024

View reviewed changes

ikhoon approved these changes Mar 20, 2024

View reviewed changes

Merge branch 'main' into cpu-based-health-checker

20e3188

Bue-von-hon marked this pull request as draft March 21, 2024 07:34

Bue-von-hon and others added 6 commits March 22, 2024 12:57

Update core/src/main/java/com/linecorp/armeria/server/healthcheck/Cpu…

5535d6a

…HealthChecker.java Co-authored-by: jrhee17 <[email protected]>

Update core/src/main/java/com/linecorp/armeria/server/healthcheck/Cpu…

759763b

…HealthChecker.java Co-authored-by: jrhee17 <[email protected]>

Update core/src/main/java/com/linecorp/armeria/server/healthcheck/Cpu…

fb59ec4

…HealthChecker.java Co-authored-by: jrhee17 <[email protected]>

Update core/src/main/java/com/linecorp/armeria/server/healthcheck/Cpu…

d1a06e3

…HealthChecker.java Co-authored-by: jrhee17 <[email protected]>

Update core/src/main/java/com/linecorp/armeria/server/healthcheck/Cpu…

1dda317

…HealthChecker.java Co-authored-by: jrhee17 <[email protected]>

Bue-von-hon marked this pull request as ready for review March 22, 2024 05:59

supplement javadocs

d619208

jrhee17 approved these changes Mar 26, 2024

View reviewed changes

minwoox approved these changes Mar 27, 2024

View reviewed changes

jrhee17 merged commit 1be0dd1 into line:main Mar 28, 2024
18 checks passed


	// Forked from https://github.com/micrometer-metrics/micrometer/blob/8339d57bef8689beb8d7a18b429a166f6595f2af/micrometer-core/src/main/java/io/micrometer/core/instrument/binder/system/ProcessorMetrics.java


		private final double targetCpuUsage;

		public DefaultCpuHealthChecker(final double targetCpuUsage) {

	private Class<?> getFirstClassFound(final List<String> classNames) {
	@Nullable
	private Class<?> getFirstClassFound(final List<String> classNames) {

		this.targetCpuUsage = (double) cpuUsage / cpuIdle;
		this.targetProcessCpuLoad = (double) processCpuUsage / processCpuIdle;

		public DefaultCpuHealthChecker(final int cpuUsage, final int cpuIdle,
		final int processCpuUsage, final int processCpuIdle) {

Added DefaultCpuHealthChecker #4673

Added DefaultCpuHealthChecker #4673

Conversation

Bue-von-hon commented Feb 14, 2023 • edited by jrhee17 Loading

jrhee17 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrhee17 Mar 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bue-von-hon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bue-von-hon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bue-von-hon commented May 13, 2023

Bue-von-hon left a comment

Choose a reason for hiding this comment

Bue-von-hon commented Mar 7, 2024

Bue-von-hon commented Mar 15, 2024

Bue-von-hon commented Mar 19, 2024

jrhee17 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ikhoon left a comment

Choose a reason for hiding this comment

Bue-von-hon commented Mar 22, 2024

jrhee17 left a comment • edited Loading

Choose a reason for hiding this comment

minwoox commented Mar 27, 2024 • edited Loading

Bue-von-hon commented Feb 14, 2023 •

edited by jrhee17

Loading

jrhee17 Mar 6, 2023 •

edited

Loading

jrhee17 left a comment •

edited

Loading

minwoox commented Mar 27, 2024 •

edited

Loading