NETOBSERV-2234: Validation warning about missing metrics#2552
NETOBSERV-2234: Validation warning about missing metrics#2552Amoghrd wants to merge 3 commits intonetobserv:mainfrom
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?
|
/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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 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)
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
|
PR needs rebase. 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. |
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