[NC | NSFS | GLACIER] Add disk usage info to nsfs metrics #9094
Conversation
WalkthroughTwo new configuration parameters were introduced to enable periodic reporting of filesystem statistics from a specified path. The Prometheus metrics reporting service was updated to periodically collect, cache, and include disk usage statistics from this path in its metrics output. These changes are conditional on configuration settings and do not alter existing logic. Changes
Sequence Diagram(s)sequenceDiagram
participant Config as config.js
participant Prometheus as prometheus_reporting.js
participant FS as Filesystem
Config->>Prometheus: Provide statfs path & interval config
loop Every interval (if path is set)
Prometheus->>FS: statfs(path)
FS-->>Prometheus: statfs data
Prometheus->>Prometheus: Cache statfs data
end
User->>Prometheus: Request /metrics/nsfs_stats
Prometheus->>Prometheus: Retrieve cached statfs data
Prometheus-->>User: Respond with NSFS metrics (includes disk usage)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error Exit handler never called! 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (3)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
tangledbytes
force-pushed
the
utkarsh/feat/add-disk-cache-metrics
branch
from
e37ada3 to
e305c57
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/server/analytic_services/prometheus_reporting.js (1)
237-239: Mirror the fix in the non-fork stats handler
Same remark as above—onceget_disk_usageis made robust & numeric, this path stays consistent.
🧹 Nitpick comments (4)
config.js (1)
957-961: Comment/doc typo & naming inconsistency
The comment saysSTAT_PATHwhile the actual key isNSFS_GLACIER_METRICS_STATFS_PATH. Minor, but it trips up grepping and future maintenance.-// NSFS_GLACIER_METRICS_STAT_PATH if set NooBaa will start reporting the statfs info of that +// NSFS_GLACIER_METRICS_STATFS_PATH – if set, NooBaa will start reporting statfs info for thatsrc/server/analytic_services/prometheus_reporting.js (3)
16-17: Lazily require heavy native modules
nb_native()spins up native bindings; there’s no need to load it when the feature is disabled (default path is empty). Re-require only inside theif (config.NSFS_GLACIER_METRICS_STATFS_PATH)block to save startup time and avoid an unnecessary dependency cycle.-const nb_native = require('../../util/nb_native'); -const { get_process_fs_context } = require('../../util/native_fs_utils'); +let nb_native; +let get_process_fs_context;and inside the
if:if (!nb_native) { nb_native = require('../../util/nb_native'); ({ get_process_fs_context } = require('../../util/native_fs_utils')); }
81-93: Prime the cache before the first scrape
On process start the first/metrics/nsfs_statsrequest will likely returndisk_usage: undefined/NaNuntil the interval fires. Call the samestatfsfetch once immediately after theifblock (or reuse the helper suggested above) to avoid the “blank metric” on the very first scrape.
105-106: Return numericdisk_usageto match Prometheus expectations
Prometheus treats JSON values as numbers/strings strictly. After fixingget_disk_usage, ensure the value here is numeric; otherwise clients will have to special-case the type.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
config.js(1 hunks)src/server/analytic_services/prometheus_reporting.js(5 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/server/analytic_services/prometheus_reporting.js (4)
src/sdk/bucketspace_fs.js (6)
nb_native(12-12)require(15-23)require(24-24)require(25-25)config(9-9)dbg(37-37)config.js (2)
require(17-17)config(7-7)src/sdk/namespace_fs.js (1)
statfs(465-465)src/util/http_utils.js (2)
config(19-19)dbg(18-18)
tangledbytes
force-pushed
the
utkarsh/feat/add-disk-cache-metrics
branch
2 times, most recently
from
b58fed7 to
a51abc8
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com> missed committing config.js Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com> fix stupid coderrabit AI suggestion Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com> change ratio to percentage Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com>
tangledbytes
force-pushed
the
utkarsh/feat/add-disk-cache-metrics
branch
from
a51abc8 to
207cf6d
Compare
| @@ -68,6 +79,21 @@ async function start_server( | |||
| if (!config.PROMETHEUS_ENABLED) { | |||
| return; | |||
| } | |||
|
|
|||
| if (config.NSFS_GLACIER_METRICS_STATFS_PATH) { | |||
There was a problem hiding this comment.
could you move the internal logic to a function?
|
@tangledbytes A general question, since we can have buckets on different mounts/file systems, shouldn't we receive it as a parameter instead of setting a single path as a config? |
Explain the Changes
This PR adds support for NooBaa reporting
disk_usageas part of NSFS stats if enabled. This feature was requested by a customer.Example:
Issues: Fixed #xxx / Gap #xxx
Testing Instructions:
Summary by CodeRabbit