Skip to content

Recording Rule uwm stage #7553

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

Merged
merged 4 commits into from
Aug 11, 2025
Merged

Conversation

gcpsoares
Copy link
Contributor

  • Include uwm recording rules in stage.

@openshift-ci openshift-ci bot requested review from eisraeli and pacho-rh August 8, 2025 14:29
Copy link
Contributor

github-actions bot commented Aug 8, 2025

Code Review by Gemini

### `components/monitoring/prometheus/staging/base/prometheusrule-uwm/prometheusrule-uwm-namespace.yaml`

**Issue 1: PrometheusRule name is generic**
The `PrometheusRule` name `prometheus-recording-rules-namespace` is quite generic. Given that this rule is specifically for User Workload Monitoring (UWM), it would be clearer to include `uwm` in the name to indicate its purpose and avoid potential naming conflicts with other generic rules.

```diff
--- a/components/monitoring/prometheus/staging/base/prometheusrule-uwm/prometheusrule-uwm-namespace.yaml
+++ b/components/monitoring/prometheus/staging/base/prometheusrule-uwm/prometheusrule-uwm-namespace.yaml
@@ -2,7 +2,7 @@
 kind: PrometheusRule
 metadata:
-  name: prometheus-recording-rules-namespace
+  name: uwm-recording-rules-namespace
   namespace: openshift-user-workload-monitoring
 spec:
   groups:

Issue 2: Group name is very verbose
The group name rules_container_memory_working_set_bytes_namespace_container is excessively long and descriptive. A shorter, more general name that still conveys the context of the rules within the group would improve readability.

--- a/components/monitoring/prometheus/staging/base/prometheusrule-uwm/prometheusrule-uwm-namespace.yaml
+++ b/components/monitoring/prometheus/staging/base/prometheusrule-uwm/prometheusrule-uwm-namespace.yaml
@@ -5,7 +5,7 @@
   namespace: openshift-user-workload-monitoring
 spec:
   groups:
-  - name: rules_container_memory_working_set_bytes_namespace_container
+  - name: uwm-container-memory-rules
     rules:
     - expr: |
         avg by (namespace,container) (rate(container_memory_working_set_bytes{job="kubelet", metrics_path="/metrics/cadvisor", image!=""}[5m]))

Issue 3: Missing newline at end of file
The file is missing a newline character at the very end. This is a common best practice for text files and can prevent issues with some tools or version control systems.

--- a/components/monitoring/prometheus/staging/base/prometheusrule-uwm/prometheusrule-uwm-namespace.yaml
+++ b/components/monitoring/prometheus/staging/base/prometheusrule-uwm/prometheusrule-uwm-namespace.yaml
@@ -9,4 +9,5 @@
     - expr: |
         avg by (namespace,container) (rate(container_memory_working_set_bytes{job="kubelet", metrics_path="/metrics/cadvisor", image!=""}[5m]))
       record: namespace_container:container_memory_working_set_bytes
-\ No newline at end of file
+

Copy link
Contributor

github-actions bot commented Aug 8, 2025

Code Review by Gemini

```diff
--- a/components/monitoring/prometheus/staging/stone-stg-rh01/prometheusrule-uwm.yaml
+++ b/components/monitoring/prometheus/staging/stone-stg-rh01/prometheusrule-uwm.yaml
@@ -4,7 +4,7 @@
   namespace: openshift-user-workload-monitoring
 spec:
   groups:
-    - name: rules_container_memory_working_set_bytes_namespace_container
+    - name: uwm_container_memory_rules
       rules:
       - expr: |
           avg by (namespace,container) (rate(container_memory_working_set_bytes{job="kubelet", metrics_path="/metrics/cadvisor", image!=""}[5m]))

Copy link
Contributor

github-actions bot commented Aug 8, 2025

Code Review by Gemini

### components/monitoring/prometheus/staging/base/kustomization.yaml

**Improvement:**
The commit message "Include uwm recording rules in stage" suggests that the PrometheusRule should be applied to all staging environments, not just a specific one. To achieve this, the `prometheusrule-uwm.yaml` should be included in the `staging/base` kustomization, making it a common resource for all clusters inheriting from this base.

```diff
--- a/components/monitoring/prometheus/staging/base/kustomization.yaml
+++ b/components/monitoring/prometheus/staging/base/kustomization.yaml
@@ -1,15 +1,16 @@
 apiVersion: kustomize.config.k8s.io/v1beta1
 kind: Kustomization
 resources:
 - ../../base/uwm-config
 - ../../base/external-secrets
 - ../../base/observability-operator
 - ../../base/rbac
 - monitoringstack/
+- prometheusrule-uwm.yaml # Add this line
 
 patches:
   - path: rhobs-secret-path.yaml
     target:
       name: rhobs
       kind: ExternalSecret
       group: external-secrets.io

components/monitoring/prometheus/staging/stone-stg-rh01/kustomization.yaml

Improvement:
Since the prometheusrule-uwm.yaml will be moved to staging/base (as suggested above), it should be removed from this cluster-specific kustomization file to avoid redundancy and ensure consistency across staging environments.

--- a/components/monitoring/prometheus/staging/stone-stg-rh01/kustomization.yaml
+++ b/components/monitoring/prometheus/staging/stone-stg-rh01/kustomization.yaml
@@ -1,12 +1,11 @@
 apiVersion: kustomize.config.k8s.io/v1beta1
 kind: Kustomization
 resources:
 - ../base
-- prometheusrule-uwm.yaml # Remove this line
 
 patches:
   - path: cluster-id-label.yaml
     target:
       name: appstudio-federate-ms
       kind: MonitoringStack
       group: monitoring.rhobs

components/monitoring/prometheus/staging/base/prometheusrule-uwm.yaml (New File)

Improvement:
Create this new file in the staging/base directory. This ensures that the UWM recording rules are applied to all staging environments that inherit from this base, aligning with the commit message's intent. The content should be moved from the currently proposed components/monitoring/prometheus/staging/stone-stg-rh01/prometheusrule-uwm.yaml.

# Create this new file: components/monitoring/prometheus/staging/base/prometheusrule-uwm.yaml
apiVersion: monitoring.coreos.com/v1
kind: PrometheusRule
metadata:
  name: prometheus-recording-rules-uwm-namespace
  namespace: openshift-user-workload-monitoring
spec:
  groups:
    - name: rules_container_memory_working_set_bytes_namespace_container
      rules:
      - expr: |
          avg by (namespace,container) (rate(container_memory_working_set_bytes{job="kubelet", metrics_path="/metrics/cadvisor", image!=""}[5m]))
        record: namespace_container:container_memory_working_set_bytes

components/monitoring/prometheus/staging/stone-stg-rh01/prometheusrule-uwm.yaml

Improvement:
This file should be deleted as its content will be moved to components/monitoring/prometheus/staging/base/prometheusrule-uwm.yaml.

--- a/components/monitoring/prometheus/staging/stone-stg-rh01/prometheusrule-uwm.yaml
+++ /dev/null
@@ -1,12 +0,0 @@
-apiVersion: monitoring.coreos.com/v1
-kind: PrometheusRule
-metadata:
-  name: prometheus-recording-rules-uwm-namespace
-  namespace: openshift-user-workload-monitoring
-spec:
-  groups:
-    - name: rules_container_memory_working_set_bytes_namespace_container
-      rules:
-      - expr: |
-          avg by (namespace,container) (rate(container_memory_working_set_bytes{job="kubelet", metrics_path="/metrics/cadvisor", image!=""}[5m]))
-        record: namespace_container:container_memory_working_set_bytes

Copy link
Contributor

@mftb mftb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/approve

Copy link

openshift-ci bot commented Aug 11, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gcpsoares, mftb

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@mftb mftb merged commit 6cea98e into redhat-appstudio:main Aug 11, 2025
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants