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

Added DefaultCpuHealthChecker #4673

Merged
merged 41 commits into from
Mar 28, 2024
Merged
Changes from 1 commit
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
f08cb8b
Added DefaultCpuHealthChecker
Bue-von-hon Feb 14, 2023
08c706e
Fixed javadoc and lint based on code review
Bue-von-hon Feb 28, 2023
551263a
Fix Lint
Bue-von-hon Mar 1, 2023
afd1843
Update core/src/main/java/com/linecorp/armeria/server/healthcheck/Def…
Bue-von-hon Mar 11, 2023
5e5069e
Update core/src/main/java/com/linecorp/armeria/server/healthcheck/Def…
Bue-von-hon Mar 11, 2023
e4132b4
Update core/src/main/java/com/linecorp/armeria/server/healthcheck/Def…
Bue-von-hon Mar 11, 2023
35da7bc
Update core/src/main/java/com/linecorp/armeria/server/healthcheck/Def…
Bue-von-hon Mar 11, 2023
a0c04bb
Update core/src/main/java/com/linecorp/armeria/server/healthcheck/Def…
Bue-von-hon Mar 11, 2023
dd68de9
Update core/src/main/java/com/linecorp/armeria/server/healthcheck/Def…
Bue-von-hon Mar 11, 2023
8934066
Update core/src/main/java/com/linecorp/armeria/server/healthcheck/Def…
Bue-von-hon Mar 11, 2023
32fd4f7
Update core/src/main/java/com/linecorp/armeria/server/healthcheck/Def…
Bue-von-hon Mar 11, 2023
0a0f815
Fixed javadoc, access modifier, invoke method's algorithm and Added T…
Bue-von-hon Mar 18, 2023
3b1770b
Fix Lint, Construct method's access level
Bue-von-hon Apr 5, 2023
c8c1664
Update core/src/main/java/com/linecorp/armeria/server/healthcheck/Def…
Bue-von-hon Jun 8, 2023
eda64c4
Fixed base on code review
Bue-von-hon Jun 9, 2023
a4733f9
Merge branch 'main' into cpu-based-health-checker
Bue-von-hon Jun 24, 2023
8be0675
Merge branch 'main' into cpu-based-health-checker
Bue-von-hon Jul 2, 2023
07c4f02
Merge branch 'main' into cpu-based-health-checker
Bue-von-hon Jul 24, 2023
d5f162f
Merge branch 'main' into cpu-based-health-checker
Bue-von-hon Aug 8, 2023
760496c
Merge branch 'main' into cpu-based-health-checker
Bue-von-hon Aug 28, 2023
a1df8fa
Resolves code review
Bue-von-hon Aug 28, 2023
3f4a5fc
Merge branch 'main' into cpu-based-health-checker
Bue-von-hon Sep 7, 2023
0b14883
Fix lint
Bue-von-hon Sep 7, 2023
667a0bf
Merge remote-tracking branch 'Bue-von-hon/cpu-based-health-checker' i…
Bue-von-hon Sep 7, 2023
2dae89c
Merge branch 'main' into cpu-based-health-checker
Bue-von-hon Oct 1, 2023
9d7cd92
Optimize CPU Health Check Implementations
Bue-von-hon Nov 15, 2023
69df576
Merge branch 'main' into cpu-based-health-checker
Bue-von-hon Nov 21, 2023
8f2b4a1
Merge branch 'main' into cpu-based-health-checker
Bue-von-hon Nov 22, 2023
f6ef458
Update core/src/main/java/com/linecorp/armeria/server/healthcheck/Cpu…
Bue-von-hon Jan 30, 2024
a766919
Update core/src/main/java/com/linecorp/armeria/server/healthcheck/Cpu…
Bue-von-hon Jan 30, 2024
f469ac7
Merge branch 'main' into cpu-based-health-checker
Bue-von-hon Feb 26, 2024
fad1155
Resolves code review
Bue-von-hon Feb 26, 2024
29048d3
Rewrite the test correctly
Bue-von-hon Feb 26, 2024
20e3188
Merge branch 'main' into cpu-based-health-checker
Bue-von-hon Mar 21, 2024
5535d6a
Update core/src/main/java/com/linecorp/armeria/server/healthcheck/Cpu…
Bue-von-hon Mar 22, 2024
759763b
Update core/src/main/java/com/linecorp/armeria/server/healthcheck/Cpu…
Bue-von-hon Mar 22, 2024
fb59ec4
Update core/src/main/java/com/linecorp/armeria/server/healthcheck/Cpu…
Bue-von-hon Mar 22, 2024
d1a06e3
Update core/src/main/java/com/linecorp/armeria/server/healthcheck/Cpu…
Bue-von-hon Mar 22, 2024
1dda317
Update core/src/main/java/com/linecorp/armeria/server/healthcheck/Cpu…
Bue-von-hon Mar 22, 2024
828e51a
Add a factory method to an HealthChecker interface
Bue-von-hon Mar 22, 2024
d619208
supplement javadocs
jrhee17 Mar 26, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
package com.linecorp.armeria.server.healthcheck;

import static java.util.Objects.requireNonNull;

import java.lang.management.ManagementFactory;
import java.lang.management.OperatingSystemMXBean;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
import java.util.Arrays;
import java.util.List;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i added!

public class DefaultCpuHealthChecker implements HealthChecker{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we also add integration tests for this class?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.😭

Copy link
Contributor

@jrhee17 jrhee17 Mar 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


private static final List<String> OPERATING_SYSTEM_BEAN_CLASS_NAMES = Arrays.asList(
Bue-von-hon marked this conversation as resolved.
Show resolved Hide resolved
"com.ibm.lang.management.OperatingSystemMXBean", // J9
"com.sun.management.OperatingSystemMXBean" // HotSpot
);

private final OperatingSystemMXBean operatingSystemBean;

private final Class<?> operatingSystemBeanClass;

private final Method systemCpuUsage;

private final double targetCpuUsage;

public DefaultCpuHealthChecker(final double targetCpuUsage) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i added!

this.targetCpuUsage = targetCpuUsage;
this.operatingSystemBean = ManagementFactory.getOperatingSystemMXBean();
this.operatingSystemBeanClass = getFirstClassFound(OPERATING_SYSTEM_BEAN_CLASS_NAMES);
this.systemCpuUsage = detectMethod("getSystemCpuLoad");
}

@Override
public boolean isHealthy() {
double cpuUsage = invoke(systemCpuUsage);
return cpuUsage <= targetCpuUsage;
}

private double invoke(final Method method) {
try {
return (double) method.invoke(operatingSystemBean);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
catch (IllegalAccessException | IllegalArgumentException | InvocationTargetException e) {
return Double.NaN;
}
}

private Method detectMethod(final String name) {
jrhee17 marked this conversation as resolved.
Show resolved Hide resolved
requireNonNull(name);
Bue-von-hon marked this conversation as resolved.
Show resolved Hide resolved
if (operatingSystemBeanClass == null) {
return null;
}
try {
// ensure the Bean we have is actually an instance of the interface
requireNonNull(operatingSystemBeanClass.cast(operatingSystemBean));
return operatingSystemBeanClass.getMethod(name);
}
catch (ClassCastException | NoSuchMethodException | SecurityException e) {
return null;
}
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i added!

Bue-von-hon marked this conversation as resolved.
Show resolved Hide resolved
for (String className : classNames) {
try {
return Class.forName(className);
Bue-von-hon marked this conversation as resolved.
Show resolved Hide resolved
}
catch (ClassNotFoundException ignore) {
}
}
return null;
}
}