Exporting threshold values as metrics from env vars #9349
Conversation
…eshold and Noobaa_bucket_no_capacity_threshold Signed-off-by: Aayush Chouhan <[email protected]>
aayushchouhan09
requested review from
dannyzaken,
liranmauda and
naveenpaul1
WalkthroughThe changes add new bucket capacity threshold configuration options (BUCKET_LOW_CAPACITY_THRESHOLD and BUCKET_NO_CAPACITY_THRESHOLD) and a QUOTA_MAX_OBJECTS configuration to config.js, introduce corresponding Prometheus Gauge metrics in the core report, and update the stats aggregator to populate these metrics from environment-configured values. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10–15 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
config.js (1)
624-627: Consider validating threshold values.The threshold percentages accept any integer value without validation. Consider adding checks to ensure:
- Both values are between 0 and 100
BUCKET_LOW_CAPACITY_THRESHOLD < BUCKET_NO_CAPACITY_THRESHOLDInvalid thresholds could lead to incorrect alerting behavior in the operator's PrometheusRule.
Example validation:
// Bucket capacity thresholds (percentage) -config.BUCKET_LOW_CAPACITY_THRESHOLD = parseInt(process.env.BUCKET_LOW_CAPACITY_THRESHOLD, 10) || 80; -config.BUCKET_NO_CAPACITY_THRESHOLD = parseInt(process.env.BUCKET_NO_CAPACITY_THRESHOLD, 10) || 95; +const low_threshold = parseInt(process.env.BUCKET_LOW_CAPACITY_THRESHOLD, 10) || 80; +const no_threshold = parseInt(process.env.BUCKET_NO_CAPACITY_THRESHOLD, 10) || 95; + +if (low_threshold < 0 || low_threshold > 100 || no_threshold < 0 || no_threshold > 100) { + throw new Error(`Bucket capacity thresholds must be between 0 and 100. Got LOW=${low_threshold}, NO=${no_threshold}`); +} +if (low_threshold >= no_threshold) { + throw new Error(`BUCKET_LOW_CAPACITY_THRESHOLD (${low_threshold}) must be less than BUCKET_NO_CAPACITY_THRESHOLD (${no_threshold})`); +} + +config.BUCKET_LOW_CAPACITY_THRESHOLD = low_threshold; +config.BUCKET_NO_CAPACITY_THRESHOLD = no_threshold;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
config.js(2 hunks)src/server/analytic_services/prometheus_reports/noobaa_core_report.js(1 hunks)src/server/system_services/stats_aggregator.js(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: liranmauda
Repo: noobaa/noobaa-core PR: 9174
File: config.js:1044-1046
Timestamp: 2025-07-30T10:20:09.402Z
Learning: The NooBaa config.js file has a built-in environment variable override system via `load_config_env_overrides()` function. Any config property can be overridden using environment variables prefixed with `CONFIG_JS_`. For example, `CONFIG_JS_NC_HEALTH_BUCKETS_COUNT_LIMIT=10000` would override `config.NC_HEALTH_BUCKETS_COUNT_LIMIT`. The system handles type conversion automatically and provides consistent override patterns across all config values.
📚 Learning: 2025-07-30T10:20:09.402Z
Learnt from: liranmauda
Repo: noobaa/noobaa-core PR: 9174
File: config.js:1044-1046
Timestamp: 2025-07-30T10:20:09.402Z
Learning: The NooBaa config.js file has a built-in environment variable override system via `load_config_env_overrides()` function. Any config property can be overridden using environment variables prefixed with `CONFIG_JS_`. For example, `CONFIG_JS_NC_HEALTH_BUCKETS_COUNT_LIMIT=10000` would override `config.NC_HEALTH_BUCKETS_COUNT_LIMIT`. The system handles type conversion automatically and provides consistent override patterns across all config values.
Applied to files:
config.js
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: run-package-lock-validation
- GitHub Check: Build Noobaa Image
- GitHub Check: run-jest-unit-tests
🔇 Additional comments (3)
src/server/system_services/stats_aggregator.js (1)
1068-1070: LGTM! Threshold metrics correctly wired.The bucket capacity threshold values are properly set from config and exported as Prometheus metrics using the auto-generated setters.
config.js (1)
1126-1126: Clarify relevance of QUOTA_MAX_OBJECTS.The addition of
QUOTA_MAX_OBJECTSis not mentioned in the PR objectives, which focus on bucket capacity thresholds. Is this configuration intentionally included in this PR, or should it be part of a separate change?src/server/analytic_services/prometheus_reports/noobaa_core_report.js (1)
434-448: LGTM! Threshold metrics properly defined.The new Gauge metrics for bucket capacity thresholds are correctly defined with
generate_default_set: true, which will auto-generate the setter methods used by stats_aggregator. The help text clearly identifies these as percentage values.
|
@aayushchouhan09 I don't really understand this PR. Why do we need to pass an env from the operator to the core just to export it as a constant metric that is only used by another resource created by the operator? I believe we can come up with a simpler solution. |
Thanks @dannyzaken I will update the operator PR with the changes (and will keep this PR open until we merge that) |
2 participants
Describe the Problem
Currently, bucket capacity alert thresholds (80% and 95%) are hardcoded in PrometheusRule (in noobaa-operator). So, we made them configurable from noobaa CR and set the env vars which can be exported as metrics from noobaa core.
Explain the Changes
bucket_low_capacity_thresholdandbucket_no_capacity_thresholdIssues: Fixed #xxx / Gap #xxx
Testing Instructions:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.