OU-1389: swap monitoring-plugin to be feature driven#1034
Conversation
|
@PeterYurkovich: This pull request references OU-1137 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 task to target the "5.0.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. |
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
Bot
added
the
approved
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThe PR splits console extensions into patch files, adds new monitoring feature flags and default feature enabling, and updates startup, Docker, webpack, and documentation to match the new patch-based console configuration. ChangesMonitoring console patch rollout
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/hold for CMO PR |
openshift-ci
Bot
added
the
do-not-merge/hold
PeterYurkovich
changed the title
|
@PeterYurkovich: This pull request references OU-1389 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 task to target the "5.0.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. |
|
/unhold |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
/test e2e-monitoring |
|
e2e-perses was run locally against a running cluster with COO already installed and it passed except scenarios involving bug https://redhat.atlassian.net/browse/OU-1466. Other than that, it was working fine. |
|
/label qe-approved |
openshift-ci
Bot
added
the
qe-approved
|
I got an error while trying to start the backend locally: this might be caused by trying to add an element to an empty array in the json. |
PeterYurkovich
force-pushed
the
swap-monitoring-to-features
branch
from
53d0f9f to
663830b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/plugin_handler.go (1)
35-58: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMake feature patching deterministic
pkg/plugin_handler.go:50-55iterates overcfg.Features, so the per-feature patch order is arbitrary. Because every patch inserts at/extensions/0, the final manifest order can change between process restarts/replicas and affect overlappingconsole.page/routeentries. Sort the enabled feature keys before applying these patches.🤖 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 `@pkg/plugin_handler.go` around lines 35 - 58, Make the per-feature patch application in patchManifest deterministic by avoiding direct iteration over cfg.Features; collect the enabled feature keys, sort them first, and then apply each feature patch in that stable order. Keep the existing special-case handling for ClusterHealthAnalyzer and Incidents unchanged, but ensure the loop that calls performPatch with fmt.Sprintf("%s.patch.json", feature) uses the sorted list so overlapping /extensions/0 inserts produce consistent manifest output.
🧹 Nitpick comments (3)
config/acm-alerting.patch.json (1)
4-95: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPrefer the RFC 6902 append token over a hardcoded index.
Switching from
/extensions/1to/extensions/0fixes the reported out-of-bounds crash on a short/emptyextensionsarray, but since all sixaddops in this file target the same fixed index, each subsequent op inserts before the previous one — reversing this file's own declaration order in the final manifest. Using the RFC 6902"-"token (/extensions/-) appends after the last element regardless of current array length, which sidesteps both the out-of-bounds risk and the ordering inversion, and is supported byevanphx/json-patch(the library used viajsonpatch.DecodePatchinpkg/plugin_handler.go).🔧 Suggested fix
- "path": "/extensions/0", + "path": "/extensions/-",(apply to each
addop in this file)🤖 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 `@config/acm-alerting.patch.json` around lines 4 - 95, The patch ops in the alerting manifest currently use a fixed "/extensions/0" target for every add, which reverses the intended order and still depends on array state. Update each console.navigation/section and console.page/route add operation in this patch file to use the RFC 6902 append token "/extensions/-" so the entries are appended in declaration order regardless of existing length.config/perses-dashboards.patch.json (1)
4-134: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSame fixed-index-append pattern as
acm-alerting.patch.json, with added route-overlap risk.This file has 10 sequential
addops all targeting/extensions/0, so within this document, insertion order is reversed. This file also declares multipleexact: falselist routes (/monitoring/v2/dashboards,/multicloud/monitoring/v2/dashboards,/virt-monitoring/v2/dashboards) alongside more specific/viewroutes under the same prefixes. If console's route matching for overlappingexact: falsepaths is order-sensitive, the relative position of these entries in the finalextensionsarray matters. Combined with the non-deterministic per-feature loop ordering noted inpkg/plugin_handler.go, using/extensions/-(RFC 6902 append token) here would remove this ordering dependency entirely rather than relying on the current reversal happening to work out.🤖 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 `@config/perses-dashboards.patch.json` around lines 4 - 134, The patch uses repeated inserts at /extensions/0, which reverses the intended order and creates a fragile dependency for overlapping exact: false routes in DashboardListPage and DashboardPage. Update this patch to append each new extension with /extensions/- so the final array order matches the declaration order without relying on reversal or loop behavior, especially for the /monitoring/v2/dashboards, /multicloud/monitoring/v2/dashboards, and /virt-monitoring/v2/dashboards entries.AGENTS.md (1)
8-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInclude the backend feature registry in the onboarding checklist.
The new-feature guidance now points at patch files/components, but feature work still needs
pkg/server.goandweb/src/components/hooks/useFeatures.tsto wire new flags through the backend and UI. Without those files in the guide, contributors can update a patch without making it load.Suggested fix
- **Key Files**: `config/*.patch.json`, `web/src/components/` + **Key Files**: `pkg/server.go`, `web/src/components/hooks/useFeatures.ts`, `config/*.patch.json`, `web/src/components/`🤖 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 `@AGENTS.md` around lines 8 - 18, The new-feature checklist is missing the backend feature wiring steps, so update the onboarding guidance to explicitly include `pkg/server.go` and `web/src/components/hooks/useFeatures.ts` alongside the existing patch/component steps. Make sure the “Adding a New Feature” section tells contributors to register new flags in `pkg/server.go` and expose them through `useFeatures.ts` so feature work is actually loaded end-to-end.
🤖 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.
Outside diff comments:
In `@pkg/plugin_handler.go`:
- Around line 35-58: Make the per-feature patch application in patchManifest
deterministic by avoiding direct iteration over cfg.Features; collect the
enabled feature keys, sort them first, and then apply each feature patch in that
stable order. Keep the existing special-case handling for ClusterHealthAnalyzer
and Incidents unchanged, but ensure the loop that calls performPatch with
fmt.Sprintf("%s.patch.json", feature) uses the sorted list so overlapping
/extensions/0 inserts produce consistent manifest output.
---
Nitpick comments:
In `@AGENTS.md`:
- Around line 8-18: The new-feature checklist is missing the backend feature
wiring steps, so update the onboarding guidance to explicitly include
`pkg/server.go` and `web/src/components/hooks/useFeatures.ts` alongside the
existing patch/component steps. Make sure the “Adding a New Feature” section
tells contributors to register new flags in `pkg/server.go` and expose them
through `useFeatures.ts` so feature work is actually loaded end-to-end.
In `@config/acm-alerting.patch.json`:
- Around line 4-95: The patch ops in the alerting manifest currently use a fixed
"/extensions/0" target for every add, which reverses the intended order and
still depends on array state. Update each console.navigation/section and
console.page/route add operation in this patch file to use the RFC 6902 append
token "/extensions/-" so the entries are appended in declaration order
regardless of existing length.
In `@config/perses-dashboards.patch.json`:
- Around line 4-134: The patch uses repeated inserts at /extensions/0, which
reverses the intended order and creates a fragile dependency for overlapping
exact: false routes in DashboardListPage and DashboardPage. Update this patch to
append each new extension with /extensions/- so the final array order matches
the declaration order without relying on reversal or loop behavior, especially
for the /monitoring/v2/dashboards, /multicloud/monitoring/v2/dashboards, and
/virt-monitoring/v2/dashboards entries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 1277e1fd-d3d2-4948-bc69-8c831476c024
📒 Files selected for processing (22)
.claude/commands/run-perses-dev.mdAGENTS.mdCONTRIBUTING.mdDockerfile.mcpMakefileREADME.mdcmd/plugin-backend.goconfig/acm-alerting.patch.jsonconfig/alerting.patch.jsonconfig/clear-extensions.patch.jsonconfig/cluster-health-analyzer.patch.jsonconfig/legacy-dashboards.patch.jsonconfig/metrics.patch.jsonconfig/monitoring-console-plugin.patch.jsonconfig/monitoring-plugin.patch.jsonconfig/perses-dashboards.patch.jsonconfig/targets.patch.jsonpkg/plugin_handler.gopkg/server.goscripts/start-console.shweb/console-extensions.jsonweb/webpack.config.ts
💤 Files with no reviewable changes (2)
- config/clear-extensions.patch.json
- web/console-extensions.json
| patchedManifest = performPatch(patchedManifest, filepath.Join(cfg.ConfigPath, "monitoring-console-plugin.patch.json")) | ||
| } | ||
|
|
||
| for feature := range cfg.Features { |
There was a problem hiding this comment.
maybe this is a pre existing issue, but it seems the feature iteration to perform the patches is not deterministic. Should we assign some order to the features and sort before iterating?. Or is fine and we trust the console to resolve the extensions even if they are not in order. Not sure if there will be issues with some routes that might match before others.
There was a problem hiding this comment.
Routes are handled via react router, so determinism is influenced by that not the extension load order. It's not really an issue to add determinism though, is there a specific order we should do? Maybe:
- monitoring-plugin
- alerting
- metrics
- dashboards-legacy
- targets
- monitoring-console-plugin
- acm-alerting
- incidents
- dashboards-perses
There was a problem hiding this comment.
fixed in latest commit
| IMAGE ?= quay.io/${ORG}/${PLUGIN_NAME}:${VERSION} | ||
| FEATURES ?=incidents,perses-dashboards,dev-config | ||
| MONITORING_FEATURES ?=alerting,targets,legacy-dashboards,metrics | ||
| ALL_FEATURES ?=alerting,targets,legacy-dashboards,metrics,incidents,perses-dashboards |
There was a problem hiding this comment.
| ALL_FEATURES ?=alerting,targets,legacy-dashboards,metrics,incidents,perses-dashboards | |
| ALL_FEATURES ?=$(MONITORING_FEATURES),incidents,perses-dashboards |
There was a problem hiding this comment.
fixed in latest commit
|
/retest |
|
/override ci/prow/okd-scos-images Image pull is failing in CI, other image job is succeeding, so going to override this one |
|
@PeterYurkovich: Overrode contexts on behalf of PeterYurkovich: ci/prow/okd-scos-images 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 kubernetes-sigs/prow repository. |
|
@PeterYurkovich: all tests passed! 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. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jgbernalp, PeterYurkovich The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
4 participants
This PR is part of a set that looks to update the monitoring-plugin to be feature driven, allowing for each feature to be toggled on or off individually. This conversion will happen over the course of the following PRs:
*.patch.jsonfeature files are able to be loaded correctly (OU-1389: config path to point to correct directory cluster-monitoring-operator#2981)alerting,targets,metricsandlegacy-dashboardsfeature flags to the monitoring-plugin. Ensure theTestClusterMonitorConsolePlugintest still passesSummary by CodeRabbit
New Features
Bug Fixes
Documentation