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.

Sign up for GitHub

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

Jump to bottom

Added DefaultMemoryHealthChecker #4711

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

Added DefaultMemoryHealthChecker #4711

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e233572
add memory health check
lazmond3 Feb 22, 2023
1a7bfbb
every time call ManagementFactory.getMemoryPoolMXBeans
lazmond3 Mar 6, 2023
f4b4ca9
null check BufferPoolMXBean
lazmond3 Mar 6, 2023
a5eccc9
rename fields
lazmond3 Mar 6, 2023
13f0361
implement toString
lazmond3 Mar 6, 2023
7403228
changed to ratio
lazmond3 Mar 6, 2023
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
51 changes: 51 additions & 0 deletions core/src/main/java/com/linecorp/armeria/server/healthcheck/DefaultMemoryHealthChecker.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
package com.linecorp.armeria.server.healthcheck;
Copy link
Member

Choose a reason for hiding this comment

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

Missing copyright header


import java.lang.management.BufferPoolMXBean;
import java.lang.management.ManagementFactory;
import java.lang.management.MemoryPoolMXBean;
import java.lang.management.MemoryType;
import java.lang.management.MemoryUsage;
import java.util.List;
import java.util.stream.Collectors;

import com.google.common.base.MoreObjects;

public class DefaultMemoryHealthChecker implements HealthChecker {
Copy link
Member

Choose a reason for hiding this comment

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

Could be package private final:

Suggested change
public class DefaultMemoryHealthChecker implements HealthChecker {
final class DefaultMemoryHealthChecker implements HealthChecker {


private final double targetHeapMemoryUtilizationRate;
Copy link
Member

Choose a reason for hiding this comment

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

Could we change Rate to Ratio in this PR? Rate means speed of change which is not what we meant here.

private final double targetNonHeapMemoryUtilizationRate;
private final double targetTotalMemoryUtilizationRate;

public DefaultMemoryHealthChecker(final double targetHeapMemoryUtilizationRate, final double targetNonHeapMemoryUtilizationRate, final double targetMemoryTotalUsage) {
this.targetHeapMemoryUtilizationRate = targetHeapMemoryUtilizationRate;
this.targetNonHeapMemoryUtilizationRate = targetNonHeapMemoryUtilizationRate;
this.targetTotalMemoryUtilizationRate = targetMemoryTotalUsage;
Comment on lines +20 to +22
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, we could add some checks here, e.g.

this.heapUsageRatioThreshold = validateRatio(heapUsageRatioThreshold, "heapUsageRatioThreshold");
this.nonHeapUsageRatioThreshold = validateRatio(nonHeapUsageRatioThreshold, "nonHeapUsageRatioThreshold");
this.totalUsageRatioThreshold = validateRatio(totalUsageRatioThreshold, "totalUsageRatioThreshold");
...

private static void validateRatio(double ratio, String paramName) {
    checkArgument(ratio >= 0 && ratio <= 1, "%s (expected: >= 0 and <= 1)", paramName);
}

Also, I suggested renaming the parameters and member fields as shown in the above example.

  • Utilization -> Usage (because it's shorter)
  • Memory -> '' (because it's already a part of the enclosing class name)
  • Target -> Threshold (because target means something we aim for, but we don't really want that happen here)

}

@Override
public boolean isHealthy() {
final List<MemoryPoolMXBean> heapMemories = getHeapMemories();
final double heapMemoryUsage = heapMemories.stream().map(MemoryPoolMXBean::getUsage).mapToDouble(MemoryUsage::getUsed).sum();
final double maximumHeapMemory = heapMemories.stream().map(MemoryPoolMXBean::getUsage).mapToDouble(MemoryUsage::getMax).sum();
final long runtimeMaxMemory = Runtime.getRuntime().maxMemory();

final BufferPoolMXBean nonHeapMemoryUsage = ManagementFactory.getPlatformMXBean(BufferPoolMXBean.class);
final double totalMemoryUsage = Runtime.getRuntime().totalMemory();
return (heapMemoryUsage / maximumHeapMemory) <= targetHeapMemoryUtilizationRate &&
(nonHeapMemoryUsage == null || (nonHeapMemoryUsage.getMemoryUsed() / nonHeapMemoryUsage.getTotalCapacity()) <= targetNonHeapMemoryUtilizationRate) &&
Copy link
Member

Choose a reason for hiding this comment

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

Convert from long to double before dividing?

(runtimeMaxMemory == Long.MAX_VALUE || totalMemoryUsage / runtimeMaxMemory <= targetTotalMemoryUtilizationRate);
Copy link
Member

Choose a reason for hiding this comment

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

Can we log a warning message if nonHeapMemoryUsage is null or runtimeMaxMemory is Long.MAX_VALUE, so that we tell our users that those properties are not available and thus it will never become unhealthy even if you specified the threshold?

We could keep a boolean flag so make sure that the log message is logged only once, so we don't pollute the log file.

}

@Override
public String toString() {
return MoreObjects.toStringHelper(this)
.add("targetTotalMemoryUtilizationRate", this.targetTotalMemoryUtilizationRate)
.add("targetHeapMemoryUtilizationRate", this.targetHeapMemoryUtilizationRate)
.add("targetNonHeapMemoryUtilizationRate", this.targetNonHeapMemoryUtilizationRate)
.toString();
}

private List<MemoryPoolMXBean> getHeapMemories() {
return ManagementFactory.getMemoryPoolMXBeans().stream().filter(e -> MemoryType.HEAP.equals(e.getType())).collect(Collectors.toList());
Copy link
Member

Choose a reason for hiding this comment

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

We could call .map(MemoryPoolMXBean::getUsage) before collecting so we don't have to do it twice in isHealthy().

}
}