Display all file cache metrics in NodeStats and use the correct human-readable field name #13232

lukas-vlcek · 2024-04-16T15:14:43Z

Description

I think this is a copy&paste issue from previous fileCacheReserved test?
Also the order of parameters in builder.humanReadableField() should be different.

~~Question 1): Do we need CHANGELOG record for this PR? (I think we do, because this is a bugfix, right?)~~

Question 2): Why we display cache_utilized field only if its value != 0? The point is that if you want to design a client handling nodes stats response then you may not be even aware that such field exists (how can you learn it does exist if documentation is incomplete?). Can't we just print this field even if the value is 0? (See example below)

Similarly we can ask why we do not want to print cache_reserved_in_bytes if !=-1?
Sounds like the idea is that if these two fields have the default value we do not want to print them. Can I ask why? (Yes, -1 is not ideal default value).

      "fs" : {
        "timestamp" : 1713278935920,
        "total" : {
          "total_in_bytes" : 1995218165760,
          "free_in_bytes" : 476316594176,
          "available_in_bytes" : 476316594176,
          "cache_reserved_in_bytes" : 0,
          "cache_utilized_in_bytes" : 0
        },
        "data" : [
          {
            "path" : "/Users/lukas.vlcek/projects/OpenSearch-worktrees/OpenSearch-main/build/testclusters/runTask-0/data/nodes/0",
            "mount" : "/System/Volumes/Data (/dev/disk3s5)",
            "type" : "apfs",
            "total_in_bytes" : 1995218165760,
            "free_in_bytes" : 476316594176,
            "available_in_bytes" : 476316594176,
            "cache_reserved_in_bytes" : 0,
            "cache_utilized_in_bytes" : 0
          }
        ]
      }

Related Issues

Introduced in #6350
Back-ported in #6485

Check List

~~[ ] New functionality includes testing.~~
- All tests pass
~~[ ] New functionality has been documented.~~
- ~~[ ] New functionality has javadoc added~~
Failing checks are inspected and point to the corresponding known issue(s) (See: Troubleshooting Failing Builds)
Commits are signed per the DCO using --signoff
Commit changes are listed out in CHANGELOG.md file (See: Changelog)
~~[ ] Public documentation issue/PR created~~

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

lukas-vlcek · 2024-04-16T15:19:16Z

This PR will need backport 2.x as well.

github-actions · 2024-04-16T16:01:03Z

✅ Gradle check result for feb966c: SUCCESS

codecov · 2024-04-16T16:02:49Z

Codecov Report

Attention: Patch coverage is 57.69231% with 11 lines in your changes missing coverage. Please review.

Project coverage is 71.73%. Comparing base (6d0484a) to head (4466d34).
Report is 167 commits behind head on main.

Files	Patch %	Lines
...rc/main/java/org/opensearch/monitor/fs/FsInfo.java	30.76%	4 Missing and 5 partials ⚠️
...c/main/java/org/opensearch/monitor/fs/FsProbe.java	71.42%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #13232      +/-   ##
============================================
+ Coverage     71.69%   71.73%   +0.03%     
+ Complexity    62385    62346      -39     
============================================
  Files          5148     5148              
  Lines        293301   293321      +20     
  Branches      42383    42389       +6     
============================================
+ Hits         210285   210405     +120     
+ Misses        65607    65561      -46     
+ Partials      17409    17355      -54

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

github-actions · 2024-04-17T15:12:46Z

❌ Gradle check result for 48b11ef: