feat: create disk memory health checker #5781

AnneMayor · 2024-06-22T11:04:08Z

Motivation:

@Bue-von-hon suggested to resolve More HealthChecker implementations #1854 issue. I felt interested to solve the problem and opened this PR.

Modifications:

I created DiskMemoryHealthChecker class based on DiskSpaceMetrics.java in the micrometer core.

Result:

Resolved More HealthChecker implementations #1854 issue.
Users who use this library can use new DiskMemoryHealthChecker feature on their own repository.

CLAassistant · 2024-06-22T11:04:13Z

All committers have signed the CLA.

github-actions · 2024-06-22T12:12:55Z

🔍 Build Scan® (commit: `7e1db1b`)

Job name	Status	Build Scan®
build-ubicloud-standard-8-jdk-8	✅	https://ge.armeria.dev/s/2lwuymoxwc5dm
build-ubicloud-standard-8-jdk-21-snapshot-blockhound	✅	https://ge.armeria.dev/s/w4zj4icrw2wwi
build-ubicloud-standard-8-jdk-17-min-java-17-coverage	✅	https://ge.armeria.dev/s/wj7vxbamd5xw4
build-ubicloud-standard-8-jdk-17-min-java-11	✅	https://ge.armeria.dev/s/72fypb4oxxqxw
build-ubicloud-standard-8-jdk-17-leak	✅	https://ge.armeria.dev/s/pgfbp5wpo766e
build-ubicloud-standard-8-jdk-11	✅	https://ge.armeria.dev/s/a6lcdu36q74fk
build-macos-12-jdk-21	✅	https://ge.armeria.dev/s/jygcqjiwm5jb4

jrhee17

Looks good overall! Left a minor comment 👍

jrhee17 · 2024-06-24T01:12:51Z

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

+     * @param targetTotalDiskSpace Target total disk space in bytes.
+     * @return an instance of {@link HealthChecker} configured with the provided disk memory space targets.
+     */
+    static HealthChecker ofDisk(long targetFreeDiskSpace, long targetTotalDiskSpace, String path) {


I'm not sure it would be very useful to allow users to specify the path.
What do you think of just setting this value as a new File(".") like done in spring-boot by default?

ref: https://github.com/spring-projects/spring-boot/blob/ca6ba2e7cf1355fe4764e6bec93dc78f5e76c07e/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/metrics/MetricsProperties.java#L205

I also don't think accepting targetTotalDiskSpace would be very useful since it would be mostly static in most cases.

Ideally, we should discover all disks in the system automatically (with options to specify or filter filesystem), detect the size of the filesystem. We should also allow a user to specify percentage values.

I see, I prefer a single health checker is responsible for a single disk as the condition for health checking may become complex. I guess it's fine to leave the File parameter as is then.

Yeah, we could indeed make use of composition of multiple health checkers to check multiple filesystems. Shall we focus on handling a single filesystem first and see how we could evolve this? That is, yeah, let's keep the File parameter.

Thanks for all comments :) Unfortunately, I am not sure what I should do exactly.
Should I remove percentage values if then?

Suggested change

static HealthChecker ofDisk(long targetFreeDiskSpace, long targetTotalDiskSpace, String path) {

static HealthChecker ofDisk(long targetFreeDiskSpace, File file) {

Then we only need targetFreeDiskSpace and file as parameter at first?

I think we can enhance it in the future :)

@injae-kim Thank you for your clarification. I get it. Will fix it within this weekend🙂

injae-kim · 2024-07-05T14:55:18Z

FYI) @AnneMayor is my opensource mentoring memeber, contributed on spring-projects/spring-data-redis#2905!

Welcome to armeria! 😃

AnneMayor · 2024-07-07T08:05:19Z

I fixed it. Please, review it :)

jrhee17

Left some more comments 👍

jrhee17 · 2024-07-10T08:30:26Z

core/src/test/java12/com/linecorp/armeria/server/JavaHttpClientUpgradeTest.java

@@ -73,6 +75,31 @@ protected void configure(ServerBuilder sb) {
    @BeforeEach
    void setup() {
        serverChannelPipeline.set(null);
+
+        bodyPublisher = new BodyPublisher() {


Can you revert unrelated changes in this PR? I prefer that flaky tests be handled separately - I think it's fine if some flaky tests don't pass in the CI run

Oh I see. Thanks.

jrhee17 · 2024-07-10T08:45:00Z

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

+     * Both free disk space must be (inclusive) lower than the specified threshold in order
+     * for the {@link HealthChecker} to report as healthy.
+     *
+     * @param targetFreeDiskSpace Target free disk space in bytes.


Suggested change

* @param targetFreeDiskSpace Target free disk space in bytes.

* @param diskSpacePercentage the target free disk space as a percentage (0 - 1).

I ammended targetDiskSpacePercentage as you suggested below. Please, consider it.

jrhee17 · 2024-07-10T08:47:46Z

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

+     * Creates a new instance of {@link HealthChecker}
+     * which reports health based on disk space of specific file path.


Suggested change

* Creates a new instance of {@link HealthChecker}

* which reports health based on disk space of specific file path.

* Creates a new instance of {@link HealthChecker} which reports health based on free disk space

* of the file system the specified path belongs to.

jrhee17 · 2024-07-10T08:48:16Z

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

+     * @param targetFreeDiskSpace Target free disk space in bytes.
+     * @return an instance of {@link HealthChecker} configured with the provided disk memory space targets.
+     */
+    static HealthChecker ofDisk(double targetFreeDiskSpace, File path) {


Suggested change

static HealthChecker ofDisk(double targetFreeDiskSpace, File path) {

static HealthChecker ofDisk(double targetDiskSpacePercentage, File path) {

jrhee17 · 2024-07-10T08:49:02Z

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

+    private final File path;
+
+    DiskMemoryHealthChecker(double targetFreeDiskSpace, File path) {
+        checkArgument(targetFreeDiskSpace >= 0, "freeDiskSpace: %s (expected: >= 0)", targetFreeDiskSpace);


Since we are dealing with percentages, can you also validate that targetFreeDiskSpace <= 1?

For sure 👍

jrhee17 · 2024-07-10T08:50:14Z

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

+     */
+    @Override
+    public boolean isHealthy() {
+        return path.getUsableSpace() >= targetFreeDiskSpace;


Can you check the percentage of free space instead?

Suggested change

return path.getUsableSpace() >= targetFreeDiskSpace;

return targetFreeSpacePercentage * path.getTotalSpace() <= path.getUsableSpace();

AnneMayor · 2024-07-20T06:21:15Z

comments resolved.

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

injae-kim · 2024-07-20T14:10:53Z

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

+ * final DiskMemoryHealthChecker diskMemoryHealthChecker = HealthChecker.ofDisk(100, new File("/tmp"));
+ *
+ * // Returns false if a file disk usable memory space is less than 100 bytes,
+ * // or true if a file disk usable memory space is greater than or equal to 100 bytes.


Suggested change

* final DiskMemoryHealthChecker diskMemoryHealthChecker = HealthChecker.ofDisk(100, new File("/tmp"));

*

* // Returns false if a file disk usable memory space is less than 100 bytes,

* // or true if a file disk usable memory space is greater than or equal to 100 bytes.

* final DiskMemoryHealthChecker diskMemoryHealthChecker = HealthChecker.ofDisk(0.8, new File("/tmp"));

*

* // Returns false if a file disk usable memory space is less than 80%.

* // or true if a file disk usable memory space is greater than or equal to 80%.

Can you update doc properly too here?

Oh, I missed it. Thanks.

AnneMayor · 2024-07-21T06:04:59Z

@injae-kim comments resolved 🙇‍♀️

jrhee17

Left some final comments 👍

jrhee17 · 2024-07-25T07:48:21Z

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

+
+    DiskMemoryHealthChecker(double targetFreeSpacePercentage, File path) {
+        checkArgument(targetFreeSpacePercentage >= 0 && targetFreeSpacePercentage <= 1,
+                      "freeDiskSpace: %s (expected < 0 or expected > 1)", targetFreeSpacePercentage);


Since that's what we expect

Suggested change

"freeDiskSpace: %s (expected < 0 or expected > 1)", targetFreeSpacePercentage);

"freeDiskSpacePercentage: %s (expected >= 0 and <= 1)", targetFreeSpacePercentage);

Oh, I just meant to express the error status. I didn't think of the normal value comments. Thanks.

jrhee17 · 2024-07-25T07:57:17Z

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

+    /**
+     * Creates a new instance of {@link HealthChecker} which reports health based on free disk space
+     * of the file system the specified path belongs to.
+     * Both free disk space must be (inclusive) lower than the specified threshold in order


Suggested change

* Both free disk space must be (inclusive) lower than the specified threshold in order

* There must be more free space (inclusive) than the specified threshold in order

jrhee17 · 2024-07-25T07:58:55Z

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

+     * @param targetDiskSpacePercentage the target free disk space as a percentage (0 - 1).
+     * @return an instance of {@link HealthChecker} configured with the provided disk memory space targets.
+     */
+    static HealthChecker ofDisk(double targetDiskSpacePercentage, File path) {
+        return new DiskMemoryHealthChecker(targetDiskSpacePercentage, path);
+    }


Sorry about changing my mind, but freeDiskSpacePercentage seems like a better term since it is analogous to the df linux command, and percentage denotes the type of value.

Can you also replace the name targetFreeSpacePercentage used in DiskMemoryHealthChecker to freeDiskSpacePercentage as well?

Suggested change

* @param targetDiskSpacePercentage the target free disk space as a percentage (0 - 1).

* @return an instance of {@link HealthChecker} configured with the provided disk memory space targets.

*/

static HealthChecker ofDisk(double targetDiskSpacePercentage, File path) {

return new DiskMemoryHealthChecker(targetDiskSpacePercentage, path);

}

* @param freeDiskSpacePercentage the target free disk space as a percentage (0 - 1).

* @return an instance of {@link HealthChecker} configured with the provided disk memory space targets.

*/

static HealthChecker ofDisk(double freeDiskSpacePercentage, File path) {

return new DiskMemoryHealthChecker(freeDiskSpacePercentage, path);

}

For sure. Will apply it.

jrhee17 · 2024-07-25T08:15:22Z

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

+ * when the target free disk space exceeds a file disk usable space.
+ * For example:
+ * <pre>{@code
+ * final DiskMemoryHealthChecker diskMemoryHealthChecker = HealthChecker.ofDisk(80, new File("/tmp"));


Suggested change

* final DiskMemoryHealthChecker diskMemoryHealthChecker = HealthChecker.ofDisk(80, new File("/tmp"));

* final DiskMemoryHealthChecker diskMemoryHealthChecker = HealthChecker.ofDisk(0.8, new File("/tmp"));

AnneMayor · 2024-07-25T13:58:17Z

Resolved.

@ikhoon , since rebasing merge your merge commit was updated to my current commit. I hope that would no change logs. Please, review it 🙇‍♀️

jrhee17

Looks good to me 👍 👍 👍 Thanks @AnneMayor 👍

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

AnneMayor · 2024-09-21T09:15:59Z

Sorry for being late.
@ikhoon resolved.

Please, review it.
Thank you all for your kind supports.

minwoox

This looks great. Thanks!
Left small suggestions. 😉

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

minwoox · 2024-09-23T03:03:52Z

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

+     */
+    @Override
+    public CompletionStage<HealthCheckStatus> get() {
+        return CompletableFuture.supplyAsync(() -> new HealthCheckStatus(isHealthy(), 500));


What do you think of adding the method variant of HealthChecker.ofDisk() that takes an internal?

That's good. I personally want to make this method relating to HealthChecker class methods. Will add it.

AnneMayor requested review from ikhoon, jrhee17, minwoox and trustin as code owners June 22, 2024 11:04

AnneMayor force-pushed the feature/create-disk-memory-health-checker branch 2 times, most recently from 0e516c8 to 6655182 Compare June 22, 2024 16:46

jrhee17 reviewed Jun 24, 2024

View reviewed changes

jrhee17 added the new feature label Jun 24, 2024

jrhee17 added this to the 1.30.0 milestone Jun 24, 2024

AnneMayor mentioned this pull request Jun 24, 2024

2024.06.23 ready-techie/weekly-retrospective#218

Closed

AnneMayor force-pushed the feature/create-disk-memory-health-checker branch 2 times, most recently from 442e518 to 8070a07 Compare July 7, 2024 07:32

AnneMayor force-pushed the feature/create-disk-memory-health-checker branch 2 times, most recently from d9e7f3f to f1e1716 Compare July 7, 2024 11:32

jrhee17 reviewed Jul 10, 2024

View reviewed changes

AnneMayor force-pushed the feature/create-disk-memory-health-checker branch from f1e1716 to 539c119 Compare July 20, 2024 06:20

AnneMayor force-pushed the feature/create-disk-memory-health-checker branch from 539c119 to 5588b14 Compare July 20, 2024 07:12

injae-kim reviewed Jul 20, 2024

View reviewed changes

core/src/test/java/com/linecorp/armeria/server/healthcheck/DiskMemoryCheckerTest.java Show resolved Hide resolved

injae-kim reviewed Jul 20, 2024

View reviewed changes

AnneMayor force-pushed the feature/create-disk-memory-health-checker branch from 5588b14 to 8ce5606 Compare July 21, 2024 06:04

AnneMayor requested review from jrhee17 and injae-kim July 23, 2024 15:27

jrhee17 reviewed Jul 25, 2024

View reviewed changes

AnneMayor force-pushed the feature/create-disk-memory-health-checker branch from f4c53c1 to bf1bc4e Compare July 25, 2024 13:52

AnneMayor force-pushed the feature/create-disk-memory-health-checker branch from bf1bc4e to 1791558 Compare July 25, 2024 17:08

jrhee17 approved these changes Jul 30, 2024

View reviewed changes

ikhoon modified the milestones: 1.30.0, 1.31.0 Aug 2, 2024

ikhoon reviewed Aug 7, 2024

View reviewed changes

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

github-actions bot added the Stale label Sep 17, 2024

AnneMayor force-pushed the feature/create-disk-memory-health-checker branch from 1791558 to 04f40a6 Compare September 21, 2024 09:15

Create disk memory health checker

7e1db1b

AnneMayor force-pushed the feature/create-disk-memory-health-checker branch from 04f40a6 to 7e1db1b Compare September 21, 2024 10:48

github-actions bot removed the Stale label Sep 22, 2024

minwoox reviewed Sep 23, 2024

View reviewed changes

github-actions bot added the Stale label Oct 27, 2024

jrhee17 modified the milestones: 1.31.0, 1.32.0 Nov 5, 2024

github-actions bot removed the Stale label Nov 6, 2024

github-actions bot added the Stale label Dec 15, 2024

	static HealthChecker ofDisk(long targetFreeDiskSpace, long targetTotalDiskSpace, String path) {
	static HealthChecker ofDisk(long targetFreeDiskSpace, File file) {

	* @param targetFreeDiskSpace Target free disk space in bytes.
	* @param diskSpacePercentage the target free disk space as a percentage (0 - 1).

		* Creates a new instance of {@link HealthChecker}
		* which reports health based on disk space of specific file path.

	static HealthChecker ofDisk(double targetFreeDiskSpace, File path) {
	static HealthChecker ofDisk(double targetDiskSpacePercentage, File path) {

	return path.getUsableSpace() >= targetFreeDiskSpace;
	return targetFreeSpacePercentage * path.getTotalSpace() <= path.getUsableSpace();

	"freeDiskSpace: %s (expected < 0 or expected > 1)", targetFreeSpacePercentage);
	"freeDiskSpacePercentage: %s (expected >= 0 and <= 1)", targetFreeSpacePercentage);

	* Both free disk space must be (inclusive) lower than the specified threshold in order
	* There must be more free space (inclusive) than the specified threshold in order

	* final DiskMemoryHealthChecker diskMemoryHealthChecker = HealthChecker.ofDisk(80, new File("/tmp"));
	* final DiskMemoryHealthChecker diskMemoryHealthChecker = HealthChecker.ofDisk(0.8, new File("/tmp"));

feat: create disk memory health checker #5781

Are you sure you want to change the base?

feat: create disk memory health checker #5781

Conversation

AnneMayor commented Jun 22, 2024 • edited Loading

CLAassistant commented Jun 22, 2024 • edited Loading

github-actions bot commented Jun 22, 2024 • edited Loading

🔍 Build Scan® (commit: 7e1db1b)

jrhee17 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trustin Jun 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnneMayor Jul 2, 2024 • edited Loading

Choose a reason for hiding this comment

injae-kim Jul 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

injae-kim commented Jul 5, 2024 • edited Loading

AnneMayor commented Jul 7, 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

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

AnneMayor commented Jul 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnneMayor commented Jul 21, 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

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

AnneMayor commented Jul 25, 2024

jrhee17 left a comment

Choose a reason for hiding this comment

AnneMayor commented Sep 21, 2024

minwoox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnneMayor commented Jun 22, 2024 •

edited

Loading

CLAassistant commented Jun 22, 2024 •

edited

Loading

github-actions bot commented Jun 22, 2024 •

edited

Loading

🔍 Build Scan® (commit: `7e1db1b`)

trustin Jun 25, 2024 •

edited

Loading

AnneMayor Jul 2, 2024 •

edited

Loading

injae-kim Jul 5, 2024 •

edited

Loading

injae-kim commented Jul 5, 2024 •

edited

Loading