-
Notifications
You must be signed in to change notification settings - Fork 259
bug fix - krr period is 0, for period under 1 day #453
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
WalkthroughThe change updates the logic for constructing the time period string in the Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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: 0
🧹 Nitpick comments (2)
robusta_krr/core/integrations/prometheus/metrics_service/prometheus_metrics_service.py (2)
280-280: Consider adding explicit handling for zero-hour edge case.While the current logic works, periods very close to zero could result in
0hwhich might still cause issues with Prometheus queries. Consider adding a minimum threshold:- hours_literal = min(int(period.total_seconds()) // 3600, 32) + hours_literal = min(max(int(period.total_seconds()) // 3600, 1), 32)This ensures the minimum period is
1heven for very short durations.
283-283: Verify mathematical consistency in days calculation.The days calculation uses
// 3600 // 24which is mathematically equivalent to// 86400, but less clear. For consistency and readability, consider:- days_literal = min(int(period.total_seconds()) // 3600 // 24, 32) + days_literal = min(int(period.total_seconds()) // 86400, 32)This matches the constant used in the condition above and makes the calculation more explicit.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
robusta_krr/core/integrations/prometheus/metrics_service/prometheus_metrics_service.py(1 hunks)
🔇 Additional comments (1)
robusta_krr/core/integrations/prometheus/metrics_service/prometheus_metrics_service.py (1)
278-284: Fix approved - logic correctly handles sub-day periods in hours.The bug fix properly addresses the issue where periods under 1 day were being converted to 0 days. The new logic:
- Uses hours format ("Xh") for periods ≤ 1 day (86400 seconds)
- Uses days format ("Xd") for longer periods
- Properly caps both at 32 to prevent excessive query ranges
The mathematical calculations are correct and the implementation will resolve the original issue where sub-day periods resulted in invalid Prometheus queries.
No description provided.