NETOBSERV-2234: Validation warning about missing metrics#2552
Conversation
|
@Amoghrd: This pull request references NETOBSERV-2234 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@Amoghrd: This pull request references NETOBSERV-2234 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2552 +/- ##
==========================================
- Coverage 72.07% 72.00% -0.08%
==========================================
Files 105 105
Lines 10924 10953 +29
==========================================
+ Hits 7874 7887 +13
- Misses 2565 2578 +13
- Partials 485 488 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
| } | ||
| } | ||
|
|
||
| func (v *validator) validateFLPMetricsForConsolePlugin() { |
There was a problem hiding this comment.
I don't think this warning is going to be always relevant. Some metrics from the defaults can be disabled or modified without necessarily being a problem for the console display. I can think for example about "node_to_node_ingress_flows_total" which is used in default alerts but not in the console queries (and it generates already a warning if it's disabled AND the related alert is in use)
Which means sometimes the warning would be triggered as a false positive.
There was a problem hiding this comment.
TBH, we have improved the situation lately about missing metrics in the console, I wonder if it's still worth it to try doing something in the webhook
There was a problem hiding this comment.
So is it better to have a separate list of metrics only required for console which does not include alerts?
Or since we implemented incremental metrics too, is it better to not have this warning at all?
There was a problem hiding this comment.
I think the difficult thing here is to make the operator aware of what the console plugin is going to query. IMO that's something potentially error prone in the long run / could be a maintenance pain.
Also, we need to put ourselves in the user's shoes when they want to disable some metrics - what's their reason: it's likely because they want to reduce the load/storage on prometheus. A warning could be annoying for them, if they did that on purpose. I think we've done some effort on the console side to better clarify why some queries don't work, ie. because some metrics are disabled, and so this could be sufficient and a warning is not needed anymore? wdyt?
PS: sorry for the late reply!
There was a problem hiding this comment.
yeah sounds good, will close out this PR and mark the JIRA as WontDo. Can reopen it in the future if there is a need
|
/hold |
|
New changes are detected. LGTM label has been removed. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughRefactors metric-include computation into shared helpers, adds validation that warns when the web console plugin is enabled while Loki is disabled and a custom metrics include list omits required defaults, and extends unit tests to cover these validation scenarios. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
|
/unhold |
|
@Amoghrd: This pull request references NETOBSERV-2234 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
api/flowcollector/v1beta2/flowcollector_validation_webhook.go (1)
443-467:⚠️ Potential issue | 🟠 MajorFalse-positive risk: warning compares against full defaults, not console-query-required metrics.
On Line 443,
GetDefaultMetrics()is used as the required set, but the warning on Line 464 says those are required for console plugin queries. That over-approximates and can warn on metrics that console queries do not actually need.Suggested direction
- defaults := v.fc.GetDefaultMetrics() + required := v.fc.GetConsolePluginRequiredMetrics() ... - for _, metric := range defaults { + for _, metric := range required {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/flowcollector/v1beta2/flowcollector_validation_webhook.go` around lines 443 - 467, The warning is using v.fc.GetDefaultMetrics() (GetDefaultMetrics) as the required set but should only check metrics actually needed by console queries; change the check in the validation that builds userMetrics from v.fc.Processor.Metrics.IncludeList and compares against defaults to instead compute the concrete set of console-required metrics (e.g., a new function or constant like ConsoleQueryRequiredMetrics) and compare userMetrics to that set; update the message and keep guidance about using spec.processor.metrics.additionalIncludeList, appending the warning to v.warnings only when console-required metrics (not the full defaults) are missing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/flowcollector/v1beta2/flowcollector_validation_webhook_test.go`:
- Around line 911-1010: Add two edge-case table-driven tests in
flowcollector_validation_webhook_test.go covering
spec.processor.metrics.includeList == nil and spec.processor.metrics.includeList
== &[]FLPMetric{}: create FlowCollector test entries (use FlowCollector,
FlowCollectorSpec, FlowCollectorLoki, FlowCollectorConsolePlugin,
FlowCollectorFLP, FLPMetrics) mirroring the existing "Console plugin with Loki
disabled..." case but once omitting IncludeList (nil) and once setting
IncludeList: &[]FLPMetric{} (empty slice), and assert the same expectedWarnings
about missing default metrics when ConsolePlugin.Enable is true and Loki.Enable
is false; also include a case showing no warning when ConsolePlugin is disabled
to cover the empty/nil path.
---
Duplicate comments:
In `@api/flowcollector/v1beta2/flowcollector_validation_webhook.go`:
- Around line 443-467: The warning is using v.fc.GetDefaultMetrics()
(GetDefaultMetrics) as the required set but should only check metrics actually
needed by console queries; change the check in the validation that builds
userMetrics from v.fc.Processor.Metrics.IncludeList and compares against
defaults to instead compute the concrete set of console-required metrics (e.g.,
a new function or constant like ConsoleQueryRequiredMetrics) and compare
userMetrics to that set; update the message and keep guidance about using
spec.processor.metrics.additionalIncludeList, appending the warning to
v.warnings only when console-required metrics (not the full defaults) are
missing.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7190651c-76f7-44b3-a7e0-f58be242d74c
📒 Files selected for processing (3)
api/flowcollector/v1beta2/flowcollector_alert_types.goapi/flowcollector/v1beta2/flowcollector_validation_webhook.goapi/flowcollector/v1beta2/flowcollector_validation_webhook_test.go
Add test cases for nil and empty slice includeList scenarios: - nil includeList: No warning (uses defaults automatically) - Empty slice includeList: Warning expected (explicitly empty, missing all defaults) Addresses CodeRabbit review feedback to ensure edge cases are covered. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
@Amoghrd: This pull request references NETOBSERV-2234 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Rebased with main and updated wrt coderabbit review comment |
Amoghrd
added
needs-review
|
@Amoghrd: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
4 participants
Description
Warning saying that some of the queries run from the console plugin will fail when user does not add some of the default metrics in includeList
Dependencies
#2546
Checklist
Summary by CodeRabbit
Bug Fixes
Tests