Skip to content

fix(prometheus): return firing_alerts from critical_alerts()#1218

Open
NETIZEN-11 wants to merge 2 commits intokrkn-chaos:mainfrom
NETIZEN-11:fix/prometheus-critical-alerts-return
Open

fix(prometheus): return firing_alerts from critical_alerts()#1218
NETIZEN-11 wants to merge 2 commits intokrkn-chaos:mainfrom
NETIZEN-11:fix/prometheus-critical-alerts-return

Conversation

@NETIZEN-11
Copy link
Copy Markdown
Contributor

@NETIZEN-11 NETIZEN-11 commented Mar 31, 2026

Description

This PR fixes an issue in krkn/prometheus/client.py:

  • Added missing return firing_alerts in critical_alerts() so that collected alerts are properly returned instead of always returning None
  • This ensures callers can correctly access and act on firing alerts

Additionally:

  • Added Apache 2.0 license header to the file

This PR is focused on a single fix to improve correctness and maintain consistency.


Type of change

  • Refactor
  • New feature
  • Bug fix
  • Optimization

Related Tickets & Documents

Checklist before requesting a review

  • Ensure the changes and proposed solution have been discussed in the relevant issue
  • I have performed a self-review of my code by running krkn and specific scenario
  • Added unit tests with above 80% coverage

Tests Performed

python run_kraken.py

# Output:
No runtime errors observed.
critical_alerts() now correctly returns firing alerts instead of None.

@qodo-code-review
Copy link
Copy Markdown

Review Summary by Qodo

Fix critical_alerts() to return firing alerts properly

🐞 Bug fix

Grey Divider

Walkthroughs

Description
• Added missing return statement in critical_alerts() method
• Method now correctly returns firing_alerts instead of None
• Added Apache 2.0 license header to file
• Cleaned up trailing whitespace in method
Diagram
flowchart LR
  A["critical_alerts() method"] -->|"collects alerts"| B["firing_alerts variable"]
  B -->|"missing return (bug)"| C["returns None"]
  B -->|"with fix"| D["returns firing_alerts"]
  D -->|"enables"| E["callers access alerts"]
Loading

Grey Divider

File Changes

1. krkn/prometheus/client.py 🐞 Bug fix +18/-2

Add license header and fix critical_alerts return

• Added Apache 2.0 license header (16 lines)
• Added missing return firing_alerts statement in critical_alerts() method
• Removed trailing whitespace after the logging statement
• Method now properly returns boolean value instead of implicitly returning None

krkn/prometheus/client.py


Grey Divider

Qodo Logo

@qodo-code-review
Copy link
Copy Markdown

qodo-code-review bot commented Mar 31, 2026

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0)

Grey Divider


Remediation recommended

1. Flake8 E305 violation 🐞 Bug ⚙ Maintainability
Description
def metrics() is defined immediately after critical_alerts() without the required blank lines,
which violates flake8 E305 and can fail lint checks. This is introduced by the new `return
firing_alerts placement at the end of critical_alerts()`.
Code

krkn/prometheus/client.py[R185-186]

+    return firing_alerts
def metrics(
Evidence
The module has return firing_alerts followed immediately by the next top-level def metrics( with
no separating blank lines, which triggers flake8 E305 (expected 2 blank lines after function
definition). The repository’s flake8 configuration only ignores W503 and E203, so E305 would still
be enforced if flake8 runs.

krkn/prometheus/client.py[182-187]
setup.cfg[40-42]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`def metrics()` follows `critical_alerts()` with no blank line(s), violating flake8 E305 / PEP8 (top-level function definitions must be separated by two blank lines).

### Issue Context
Repo `setup.cfg` enables flake8 and does not ignore E305, so this can fail linting.

### Fix Focus Areas
- krkn/prometheus/client.py[185-187]

### Suggested change
Insert two blank lines between `return firing_alerts` and `def metrics(`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

ⓘ The new review experience is currently in Beta. Learn more

Grey Divider

Qodo Logo

paigerube14

This comment was marked as outdated.

@NETIZEN-11 NETIZEN-11 force-pushed the fix/prometheus-critical-alerts-return branch from 4c5304f to 8afc056 Compare March 31, 2026 13:26
critical_alerts() collected alerts into a list but always returned None,
making the result inaccessible to every caller. Add the missing
return statement so callers can act on the firing alerts.

Also adds Apache 2.0 license header.

Signed-off-by: NETIZEN-11 <[email protected]>
@NETIZEN-11 NETIZEN-11 force-pushed the fix/prometheus-critical-alerts-return branch from 35cca76 to 5d2dc55 Compare March 31, 2026 23:12
@NETIZEN-11
Copy link
Copy Markdown
Contributor Author

@paigerube14 I've squashed the commits into a single commit and force-pushed the changes. Please review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants