fix(jans-cedarling): update hardcoded status field in telemetry entry#14555
fix(jans-cedarling): update hardcoded status field in telemetry entry#14555dagregi wants to merge 4 commits into
Conversation
Signed-off-by: dagregi <dagmawi.m@proton.me>
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
📝 WalkthroughWalkthroughChangesThe change computes aggregate engine health as Health status plumbing
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
mo-auto
added
comp-jans-cedarling
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@jans-cedarling/cedarling/src/lock/health_registry.rs`:
- Around line 101-109: Introduce a public strongly typed SystemHealth enum with
Unknown, Running, and Degraded variants in health_registry.rs, update
compute_status to return it, and use matches!(s, HealthStatus::Success) for
success detection; convert the enum to strings only at serialization boundaries
and update downstream mappings and dispatch_audit assignments to use as_str() or
to_string(). In jans-cedarling/cedarling/src/lock/transport/mod.rs:45, change
AuditItem::status from Option<String> to the appropriate Option<SystemHealth>
type.
In `@jans-cedarling/cedarling/src/lock/mod.rs`:
- Around line 413-422: Update HealthRegistry with a current_status method that
reads the registered checks and computes the overall status directly, returning
the existing unknown, running, or degraded values without collecting a HashMap.
In dispatch_audit, replace HealthRegistry::compute_status(®istry.collect())
with the new current_status call while preserving the existing status assignment
and telemetry dispatch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d61093c5-863d-4f62-a700-2738e6e84677
📒 Files selected for processing (9)
jans-cedarling/cedarling/src/lock/health_registry.rsjans-cedarling/cedarling/src/lock/health_ticker.rsjans-cedarling/cedarling/src/lock/mod.rsjans-cedarling/cedarling/src/lock/transport/mapping.rsjans-cedarling/cedarling/src/lock/transport/mod.rsjans-cedarling/cedarling/src/lock/transport/test_utils.rsjans-cedarling/cedarling/src/log/interface.rsjans-cedarling/cedarling/src/log/log_entry.rsjans-cedarling/cedarling/src/log/log_strategy.rs
| pub payload: AuditPayload, | ||
| pub pdp_id: PdpID, | ||
| pub app_name: Option<ApplicationName>, | ||
| pub status: Option<String>, |
There was a problem hiding this comment.
Maybe better to have a status as an enum?
Signed-off-by: dagregi <dagmawi.m@proton.me>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
jans-cedarling/cedarling/src/lock/transport/mapping.rs (1)
400-400: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd a descriptive message to the assertion.
As per coding guidelines, all assertions must include a descriptive message explaining what is being tested.
🐛 Proposed fix
- assert_eq!(lock_entry.status, "unknown"); + assert_eq!(lock_entry.status, "unknown", "metrics without explicit status should default to unknown");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@jans-cedarling/cedarling/src/lock/transport/mapping.rs` at line 400, Update the assertion checking lock_entry.status in the relevant test to include a descriptive failure message explaining that the lock entry status is expected to be "unknown".Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@jans-cedarling/cedarling/src/lock/health_registry.rs`:
- Around line 26-33: Update the HealthStatus::fmt implementation to use
f.write_str for the literal "success" and "failure" outputs instead of the
write! macro, preserving the existing Display behavior.
In `@jans-cedarling/cedarling/src/lock/transport/mapping.rs`:
- Around line 174-176: Update the status mapping in the surrounding transport
conversion to use SystemHealth::as_str with map_or, returning the existing
"unknown" fallback and allocating only the final String. Replace the current
map_or_else closure and s.to_string formatting while preserving the same output
for present and absent statuses.
---
Outside diff comments:
In `@jans-cedarling/cedarling/src/lock/transport/mapping.rs`:
- Line 400: Update the assertion checking lock_entry.status in the relevant test
to include a descriptive failure message explaining that the lock entry status
is expected to be "unknown".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0af9e1b6-f15e-4a82-834d-8995f216636c
📒 Files selected for processing (5)
jans-cedarling/cedarling/src/lock/health_registry.rsjans-cedarling/cedarling/src/lock/health_ticker.rsjans-cedarling/cedarling/src/lock/mod.rsjans-cedarling/cedarling/src/lock/transport/mapping.rsjans-cedarling/cedarling/src/lock/transport/mod.rs
tareknaser
left a comment
tareknaser
left a comment
There was a problem hiding this comment.
Looks good to me. thank you
I just have a minor comment
| "degraded" | ||
| } | ||
| .to_string(); | ||
| let overall_status = self.registry.compute_status(); |
There was a problem hiding this comment.
Here collect() already runs every registered check to build engine_status and then compute_status() takes the read lock again and runs every check a second time just to reduce them to the overall value. So on each tick the checks run twice under two separate lock snapshots.
I think compute_status() is the right call in dispatch_audit (there's no collect() there so re-deriving is the cheap option) but here you already have the map in hand. Could you derive the overall from engine_status instead? Something like:
let overall_status = if engine_status.is_empty() {
SystemHealth::Unknown
} else if engine_status.values().all(|s| *s == HealthStatus::Success) {
SystemHealth::Running
} else {
SystemHealth::Degraded
};That way the checks only run once and the overall status is guaranteed to match the engine_status map you send in the same entry.
Signed-off-by: dagregi <dagmawi.m@proton.me>
| .as_ref() | ||
| .map(HealthRegistry::compute_status); | ||
| item.status = status; | ||
| (self.telemetry_worker.as_ref(), "telemetry") |
There was a problem hiding this comment.
The worker name can also be an enum
There was a problem hiding this comment.
This might be an overkill since it's only used for a single time logging
| assert_eq!(lock_entry.service.as_deref(), Some("jans-auth")); | ||
| assert_eq!(lock_entry.node_name, pdp_id.to_string()); | ||
| assert_eq!(lock_entry.status, "running"); | ||
| assert_eq!(lock_entry.status, "unknown"); |
There was a problem hiding this comment.
I think better to compare enums
b0fbdf1
Signed-off-by: dagregi <dagmawi.m@proton.me>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@jans-cedarling/cedarling/src/lock/transport/mapping.rs`:
- Line 401: Update the changed assert_eq! calls in the lock transport mapping
tests, including the assertions around lock_entry.status at the referenced
locations, to include descriptive failure messages. Ensure each message explains
the expected system health status while preserving the existing comparisons.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 050ffe6e-21cf-4b40-8d87-ac6b3519b336
📒 Files selected for processing (1)
jans-cedarling/cedarling/src/lock/transport/mapping.rs
tareknaser
left a comment
tareknaser
left a comment
There was a problem hiding this comment.
No further comments from my side
5 participants
Prepare
Description
Target issue
closes #14549
Implementation Details
Test and Document the changes
Please check the below before submitting your PR. The PR will not be merged if there are no commits that start with
docs:to indicate documentation changes or if the below checklist is not selected.Summary by CodeRabbit
running,degraded,unknown) to metrics/telemetry records based on registered health checks when available.runningwhen health information isn’t available; they now reportunknown.degraded) is now preserved end-to-end and correctly mapped into telemetry metrics.