Conversation
This commit adds a variety of real disk metrics for the block devices that back Elasticsearch data paths. A collection of statistics are read from /proc/diskstats and are used to report the raw metrics for operations and read/write bytes. Relates elastic#15915
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 5 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| builder.field(WRITE_OPERATIONS, totalWriteOperations); | ||
| builder.field(READ_KILOBYTES, totalReadKilobytes); | ||
| builder.field(WRITE_KILOBYTES, totalWriteKilobytes); | ||
| } |
There was a problem hiding this comment.
Missing endObject() call for "total" in IoStats serialization
High Severity
The IoStats.toXContent() method calls builder.startObject("total") on line 399 but never calls the corresponding builder.endObject(). This results in malformed JSON/XContent output where the "total" object is never closed, causing serialization errors when generating filesystem statistics.
| } catch (Exception e) { | ||
| majorDeviceNumber = -1; | ||
| minorDeviceNumber = -1; | ||
| } |
There was a problem hiding this comment.
Device numbers uninitialized when mount point not found
Medium Severity
The majorDeviceNumber and minorDeviceNumber fields default to 0 when no matching mount point is found in /proc/self/mountinfo. The code uses -1 to indicate "unknown" (as seen in the exception handler), but if the loop completes without finding a match, the values remain at 0. This inconsistency causes FsProbe to treat (0, 0) as a valid device number since it checks for != -1.
| } catch (IOException e) { | ||
| logger.debug("unexpected exception reading filesystem info", e); | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Cache refresh returns null violating SingleObjectCache contract
High Severity
The stats() helper method returns null on IOException, and FsInfoCache.refresh() propagates this null. However, SingleObjectCache requires refresh() to never return null—this is enforced by assertions and documented in its contract. With assertions enabled, this causes AssertionError. With assertions disabled, the null value is cached and returned to callers, causing NullPointerException. The previous code returned an empty FsInfo object instead of null.
Additional Locations (1)
| permission java.io.FilePermission "/proc/sys/vm/max_map_count", "read"; | ||
|
|
||
| // io stats on Linux | ||
| permission java.io.FilePermission "/proc/diskstats", "read"; |
There was a problem hiding this comment.
Missing security permission for /proc/self/mountinfo file access
High Severity
The new code in ESFileStore reads /proc/self/mountinfo to obtain device numbers, but security.policy only adds permission for /proc/diskstats, not /proc/self/mountinfo. When running with a SecurityManager, this causes a SecurityException that's silently caught, setting device numbers to -1. This prevents any devices from being matched, completely disabling the I/O statistics feature in production.
Additional Locations (1)
| private final FsProbe probe; | ||
|
|
||
| private final SingleObjectCache<FsInfo> fsStatsCache; | ||
| private final TimeValue refreshInterval; |
There was a problem hiding this comment.
Unused field stored after constructor
Low Severity
The refreshInterval field is assigned in the constructor (line 49), logged (line 50), and passed to FsInfoCache (line 51), but never accessed after construction. It should be a local variable in the constructor instead of a class field.
2 participants
PR_015
Note
Medium Risk
Touches monitoring stats generation and
FsInfoserialization/output, which can affect node stats API compatibility and stability. Logic is Linux-gated and mostly best-effort (returns null on errors), reducing blast radius but still worth careful review.Overview
Adds Linux-only disk I/O reporting to node
fsstats by mapping each data path to its backing block device (major/minor) and sampling/proc/diskstatsto compute per-interval read/write ops and KB, plus aggregated totals.This extends
FsInfowire/XContent formats with an optionalio_statssection (newDeviceStats/IoStats), updatesFsProbe/FsServiceto pass prior samples for delta calculations, and caches device identifiers from/proc/self/mountinfovia newlucene:*device_numberfilestore attributes.Includes security policy changes to allow reading
/proc/diskstats, updates affected tests/fixtures for the newFsInfoconstructor/serialization, and adds new unit tests forDeviceStats/FsProbeI/O parsing/deltas.Written by Cursor Bugbot for commit ecce53f. This will update automatically on new commits. Configure here.