-
Notifications
You must be signed in to change notification settings - Fork 288
[Rob 1901] custom rule label #1957
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Warning Rate limit exceeded@Avi-Robusta has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 41 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughAdds Helm settings for PrometheusRule group name and custom labels, threads them from registry → RRM → PrometheusAlertResourceHandler, updates handler to merge/apply labels and group name on created/updated PrometheusRule CRs, and inserts documentation describing these settings (duplicated in two places). Changes
Sequence Diagram(s)sequenceDiagram
participant Sink as RobustaSink
participant RRM as RRM
participant Registry as Registry\n(global_config)
participant Handler as PrometheusAlertResourceHandler
Sink->>RRM: __init__(dal, cluster, account_id, registry)
RRM->>Registry: read global_config
Registry-->>RRM: {prometheus_rule_group_name, prometheus_rule_custom_labels}
RRM->>Handler: __init__(..., custom_labels, group_name)
Handler->>Handler: store __custom_labels & __group_name
Note over Handler: On PrometheusRule create/update
Handler->>Handler: __get_snapshot_body()
Handler->>Handler: build base labels (release, role, id)
Handler->>Handler: merge __custom_labels into labels
Handler->>Handler: set group name on PrometheusRule metadata
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20-30 minutes
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/robusta/core/sinks/robusta/robusta_sink.py (1)
95-99: RRM now correctly receives registry; optionally useself.registryfor clarityPassing
registryintoRRMmatches the updated constructor and lets it readglobal_configfor PrometheusRule settings. For consistency and readability you might consider passingself.registryinstead of the raw parameter, sinceSinkBasealready stores it.src/robusta/core/sinks/robusta/rrm/rrm.py (1)
16-31: RRM initialization correctly pulls PrometheusRule config; consider cleaning up unusedaccount_idUsing
registry.get_global_config()here to derivecustom_labelsandgroup_nameand passing them intoPrometheusAlertResourceHandleris a clean way to thread the new settings without tying RRM toRobustaSink.Two minor points:
- These values are read once at RRM construction. If you ever want live updates on
config_reload, you’d need to re-readglobal_configor recreate the handler; currently a Helm upgrade/restart is required, which matches the docs.- Ruff correctly reports
account_idas unused in the constructor. When it’s safe to touch the API, consider removing it or storing/using it (e.g., for logging/metrics) to avoid drift between signature and implementation.src/robusta/core/sinks/robusta/rrm/prometheus_alert_resource_manager.py (1)
18-37: Constructor wiring forcustom_labelsandgroup_namelooks good; consider updating group name on existing CRsThe new constructor parameters and defaults (
group_name or "kubernetes-apps",custom_labels or {}) are consistent with the Helm defaults and RRM usage.One behavioral nuance: in
__create_cr_rules, whenexisting_cr_objis present, you only updategroups[0]["rules"]and leave the existing group name as-is. That means changingprometheus_rule_group_nameinglobalConfigwill not update thespec.groups[0].nameof already-created PrometheusRule CRDs; only CRDs created after the change will use the new group name.If you want the configuration to apply uniformly after a Helm upgrade, you could also set the group name when updating existing objects, e.g.:
if existing_cr_obj: if not existing_cr_obj.get("spec"): existing_cr_obj["spec"] = {} if not existing_cr_obj["spec"].get("groups"): existing_cr_obj["spec"]["groups"] = [{}] - existing_cr_obj["spec"]["groups"][0]["rules"] = rules + existing_cr_obj["spec"]["groups"][0]["rules"] = rules + existing_cr_obj["spec"]["groups"][0]["name"] = self.__group_nameThis keeps behavior aligned with the new configuration knobs after upgrades.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
docs/setup-robusta/alertsui.rst(1 hunks)helm/robusta/values.yaml(1 hunks)src/robusta/core/sinks/robusta/robusta_sink.py(1 hunks)src/robusta/core/sinks/robusta/rrm/prometheus_alert_resource_manager.py(3 hunks)src/robusta/core/sinks/robusta/rrm/rrm.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/robusta/core/sinks/robusta/rrm/rrm.py (3)
src/robusta/core/sinks/robusta/rrm/account_resource_fetcher.py (1)
AccountResourceFetcher(9-22)src/robusta/core/sinks/robusta/robusta_sink.py (1)
get_global_config(720-721)src/robusta/core/sinks/robusta/rrm/prometheus_alert_resource_manager.py (1)
PrometheusAlertResourceHandler(17-207)
src/robusta/core/sinks/robusta/robusta_sink.py (1)
src/robusta/core/sinks/robusta/rrm/rrm.py (1)
RRM(15-90)
🪛 Ruff (0.14.5)
src/robusta/core/sinks/robusta/rrm/rrm.py
16-16: Unused method argument: account_id
(ARG002)
⏰ 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_tests
- GitHub Check: run_tests
- GitHub Check: Deploy docs
🔇 Additional comments (2)
helm/robusta/values.yaml (1)
37-41: New PrometheusRule config options align with handler behaviorDefining
prometheus_rule_group_name(defaulting to"kubernetes-apps") andprometheus_rule_custom_labelsunderglobalConfigmatches howRRMandPrometheusAlertResourceHandlerconsume these values. The defaults are sensible and preserve existing behavior when unset.docs/setup-robusta/alertsui.rst (1)
69-91: Docs accurately describe new PrometheusRule customization knobsThe new section clearly explains how to set
prometheus_rule_group_nameandprometheus_rule_custom_labelsunderglobalConfig, and correctly scopes them to PrometheusRules created from the Alerts UI. This matches the Helm values and handler implementation.
src/robusta/core/sinks/robusta/rrm/prometheus_alert_resource_manager.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/robusta/core/sinks/robusta/rrm/prometheus_alert_resource_manager.py (1)
78-84: Avoid letting custom labels break the internalrelease.appselector.The
labels.update(self.__custom_labels)on line 84 allows users to override the internal identification label (release.app). If overridden, the__list_rules_objectsmethod won't discover CRDs created with the custom value, leading to sync conflicts.This issue was already identified in a previous review. Please refer to the earlier comment on lines 78-92 for the recommended fix.
🧹 Nitpick comments (1)
src/robusta/core/sinks/robusta/rrm/prometheus_alert_resource_manager.py (1)
22-22: Makecustom_labelsoptional with a default value for defensive coding.The
custom_labelsparameter is currently required. If a caller doesn't provide it, aTypeErrorwill be raised. Consider making it optional to improve robustness.Apply this diff to make the parameter optional:
def __init__( self, cluster: str, resource_kind: ResourceKind, - custom_labels: Dict[str, str], + custom_labels: Optional[Dict[str, str]] = None, group_name: Optional[str] = None, ):Then update line 36 to handle the None case:
self.__group_name = group_name or "kubernetes-apps" - self.__custom_labels = custom_labels + self.__custom_labels = custom_labels or {}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/robusta/core/sinks/robusta/rrm/prometheus_alert_resource_manager.py(4 hunks)
⏰ 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). (2)
- GitHub Check: run_tests
- GitHub Check: run_tests
src/robusta/core/sinks/robusta/rrm/prometheus_alert_resource_manager.py
Outdated
Show resolved
Hide resolved
src/robusta/core/sinks/robusta/rrm/prometheus_alert_resource_manager.py
Outdated
Show resolved
Hide resolved
arikalon1
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice work!
For example adding this config
created this prometheus rule from the UI