fix(observability): bundle patched Perses Loki/Table/StatChart plugins [RHOAIENG-78528]#8726
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited) Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (2)
📜 Recent review details⏰ Context from checks skipped due to timeout. (3)
🧰 Additional context used📓 Path-based instructions (3)**⚙️ CodeRabbit configuration file
Files:
packages/{model-registry,model-serving,model-training,kserve,llmd-serving,feature-store,observability,plugin-core,app-config}/src/**/*.{ts,tsx}⚙️ CodeRabbit configuration file
Files:
**/*.{ts,tsx,js,jsx}⚙️ CodeRabbit configuration file
Files:
🔇 Additional comments (4)
📝 WalkthroughWalkthroughThe observability package adds Loki, StatChart, and Table Perses plugin dependencies. The plugin loader dynamically imports locally bundled Prometheus, Loki, Table, and StatChart implementations, caches them by server and npm module names, and uses the asynchronous override loader for installed-plugin and module-import resolution. Tests cover resolution, caching, retries, failures, and remote-loader delegation. 🚥 Pre-merge checks | ✅ 10✅ Passed checks (10 passed)
Comment |
![odh-dashboard-agent[bot]](https://avatars.githubusercontent.com/in/3714079?s=60&v=4){kind=small}
There was a problem hiding this comment.
Preflight Agent Report
Verdict:
Commit: 11127d1
Checks
| Check | Status | Details |
|---|---|---|
| Conflicts | Mergeable, but branch is BEHIND main — consider rebasing before merge | |
| CI (Quality Gates) | ⏭️ | Quality Gates Summary ✅ · Application Quality Gate (observability) ✅ |
| CI (Konflux / Prow) | Still running/queued — Konflux builds and Prow image mirrors pending | |
| Jira | ✅ | RHOAIENG-78528 — Critical Bug, In Progress |
| Test Coverage | ✅ | No test files added; Test Impact section explains why (third-party plugin override, validated on cluster) |
| PR Body | ✅ | All template sections complete · Jira link present · Before/After screenshots included |
| Claude (local) | ✅ | No correctness bugs — two-key Map design is intentional and correct |
| Style (local) | ✅ | No CSS/styling changes |
| RBAC (local) | ✅ | No RBAC/auth concerns — pure plugin bundling change |
| Jira Eval (local) | ➖ | No formal acceptance criteria in RHOAIENG-78528 |
| Human (PR) | REVIEW_REQUIRED — no human approvals yet |
Automated by ODH Dashboard Agent
11127d1 to
e7229e2
Compare
![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
🤖 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 `@packages/observability/package.json`:
- Around line 51-55: Update the `@perses-dev/plugin-system` dependency in the
package manifest from ^0.54.0-beta.10 to ^0.54.0-rc.1, aligning it with the
listed Perses plugin RC dependencies while leaving the other dependency versions
unchanged.
🪄 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: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: b879de0c-9ac6-4654-ada6-840fa0ec06fc
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json,!package-lock.json
📒 Files selected for processing (2)
packages/observability/package.jsonpackages/observability/src/perses/persesPluginsLoader.tsx
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: ODH Dashboard Agent
- GitHub Check: Phase 0: Hermetic Build Preflight
- GitHub Check: preflight (8726, 11127d1, perses-plugin-updates, christianvogt/o...
- GitHub Check: Setup
🧰 Additional context used
📓 Path-based instructions (4)
**
⚙️ CodeRabbit configuration file
**: REVIEW PRIORITIES:
- Security vulnerabilities — provide severity, exploit scenario,
and remediation code. Cite CWE/CVE IDs.- Bugs that could reach production — logic errors, null/undefined,
race conditions, incorrect async handling, resource leaks.- API contract correctness — shape mismatches, missing error handling,
silent failures, wrong HTTP status codes.- Performance only when measurable — O(n^2) in hot paths, unbounded
memory growth, missing pagination.Do not comment on:
- Naming preferences (unless genuinely misleading)
- Import ordering or formatting (handled by ESLint and Prettier)
- Alternative patterns that are equally valid
- Missing docs unless a public API is genuinely unclear
- Code deduplication / DRY suggestions where both copies are short
and self-contained (< 20 lines)- Adding explicit type annotations when TypeScript can infer the type
- Suggesting exhaustive switch/if-else when a default branch exists
Files:
packages/observability/package.jsonpackages/observability/src/perses/persesPluginsLoader.tsx
**/package.json
⚙️ CodeRabbit configuration file
**/package.json: SUPPLY CHAIN: NPM PACKAGE MANIFEST TAMPERING (CWE-829)package.json controls dependency resolution and lifecycle script execution.
Attackers use it to introduce malicious packages or execute code during
npm install.FLAG AS CRITICAL:
- Lifecycle scripts (preinstall, postinstall, prepare, prepublish) that
run shell commands, curl/wget, or reference external URLs. NOTE:
"prepare" running husky and "postinstall" running turbo/lerna are
standard toolchain patterns and should not be flagged- New dependencies with names that differ by 1-2 characters from
well-known packages already in the project's dependencies (e.g.,
"lodassh" when "lodash" is already a dependency). This is a
best-effort heuristic, not a comprehensive typosquatting check- publishConfig with a non-default registry URL
- Scoped packages (
@scope/pkg) where the scope doesn't match a known,
trusted organization- New dependencies not imported or referenced anywhere in the code
changes within the same PR (dependency added but never used)
Files:
packages/observability/package.json
packages/{model-registry,model-serving,model-training,kserve,llmd-serving,feature-store,observability,plugin-core,app-config}/src/**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
packages/{model-registry,model-serving,model-training,kserve,llmd-serving,feature-store,observability,plugin-core,app-config}/src/**/*.{ts,tsx}: FEATURE PACKAGE SOURCE:
These packages contribute to the dashboard via extensions or direct imports.
- Follow the same PatternFly v6 and TypeScript strict-mode conventions as the main frontend.
- Exports must be stable — other packages depend on them.
- Co-locate tests with source files.
- Check for AGENTS.md in the package root for package-specific guidance.
Files:
packages/observability/src/perses/persesPluginsLoader.tsx
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
**/*.{ts,tsx,js,jsx}: WEB SECURITY (XSS, CSRF Prevention):
- No dangerouslySetInnerHTML without sanitization (XSS - CWE-79)
- Validate all API responses before rendering
- CSRF token validation for state-changing operations
- No sensitive data in localStorage
Files:
packages/observability/src/perses/persesPluginsLoader.tsx
🔇 Additional comments (1)
packages/observability/src/perses/persesPluginsLoader.tsx (1)
4-7: Also applies to: 28-42
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 `@packages/observability/src/perses/persesPluginsLoader.tsx`:
- Around line 23-59: Add co-located tests for the override-loading logic in
loadBundledOverride and both loader entry points. Cover manifest-name lookup,
cached npm-name alias lookup, delegation for unknown plugins, and retry behavior
after a rejected dynamic import, using mocked bundled modules and loaders
without changing unrelated behavior.
- Around line 52-57: Update the override-loading promise in the loader flow
around loadedOverrides and getPluginModule so rejected dynamic imports remove
their cache entries before propagating the error. Evict both the requested name
and any npm package name alias that may have been registered, while preserving
successful caching and rejection behavior.
🪄 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: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 4a737bac-bcc2-4002-807f-b0ffc5c28322
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json,!package-lock.json
📒 Files selected for processing (2)
packages/observability/package.jsonpackages/observability/src/perses/persesPluginsLoader.tsx
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Phase 0: Hermetic Build Preflight
- GitHub Check: Setup
🧰 Additional context used
📓 Path-based instructions (4)
**
⚙️ CodeRabbit configuration file
**: REVIEW PRIORITIES:
- Security vulnerabilities — provide severity, exploit scenario,
and remediation code. Cite CWE/CVE IDs.- Bugs that could reach production — logic errors, null/undefined,
race conditions, incorrect async handling, resource leaks.- API contract correctness — shape mismatches, missing error handling,
silent failures, wrong HTTP status codes.- Performance only when measurable — O(n^2) in hot paths, unbounded
memory growth, missing pagination.Do not comment on:
- Naming preferences (unless genuinely misleading)
- Import ordering or formatting (handled by ESLint and Prettier)
- Alternative patterns that are equally valid
- Missing docs unless a public API is genuinely unclear
- Code deduplication / DRY suggestions where both copies are short
and self-contained (< 20 lines)- Adding explicit type annotations when TypeScript can infer the type
- Suggesting exhaustive switch/if-else when a default branch exists
Files:
packages/observability/package.jsonpackages/observability/src/perses/persesPluginsLoader.tsx
**/package.json
⚙️ CodeRabbit configuration file
**/package.json: SUPPLY CHAIN: NPM PACKAGE MANIFEST TAMPERING (CWE-829)package.json controls dependency resolution and lifecycle script execution.
Attackers use it to introduce malicious packages or execute code during
npm install.FLAG AS CRITICAL:
- Lifecycle scripts (preinstall, postinstall, prepare, prepublish) that
run shell commands, curl/wget, or reference external URLs. NOTE:
"prepare" running husky and "postinstall" running turbo/lerna are
standard toolchain patterns and should not be flagged- New dependencies with names that differ by 1-2 characters from
well-known packages already in the project's dependencies (e.g.,
"lodassh" when "lodash" is already a dependency). This is a
best-effort heuristic, not a comprehensive typosquatting check- publishConfig with a non-default registry URL
- Scoped packages (
@scope/pkg) where the scope doesn't match a known,
trusted organization- New dependencies not imported or referenced anywhere in the code
changes within the same PR (dependency added but never used)
Files:
packages/observability/package.json
packages/{model-registry,model-serving,model-training,kserve,llmd-serving,feature-store,observability,plugin-core,app-config}/src/**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
packages/{model-registry,model-serving,model-training,kserve,llmd-serving,feature-store,observability,plugin-core,app-config}/src/**/*.{ts,tsx}: FEATURE PACKAGE SOURCE:
These packages contribute to the dashboard via extensions or direct imports.
- Follow the same PatternFly v6 and TypeScript strict-mode conventions as the main frontend.
- Exports must be stable — other packages depend on them.
- Co-locate tests with source files.
- Check for AGENTS.md in the package root for package-specific guidance.
Files:
packages/observability/src/perses/persesPluginsLoader.tsx
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
**/*.{ts,tsx,js,jsx}: WEB SECURITY (XSS, CSRF Prevention):
- No dangerouslySetInnerHTML without sanitization (XSS - CWE-79)
- Validate all API responses before rendering
- CSRF token validation for state-changing operations
- No sensitive data in localStorage
Files:
packages/observability/src/perses/persesPluginsLoader.tsx
🔇 Additional comments (1)
packages/observability/package.json (1)
52-52: Duplicate: alignplugin-systemwith the added Perses RC plugins.Line 52 still pins
@perses-dev/plugin-systemto^0.54.0-beta.10, while the new Loki, Table, and StatChart RCs require the0.54.0-rc.1peer range noted in the previous review. Strict peer resolution can therefore fail installation instead of loading the bundled plugins. (docs.npmjs.com)Proposed fix
- "`@perses-dev/plugin-system`": "^0.54.0-beta.10", + "`@perses-dev/plugin-system`": "^0.54.0-rc.1",#!/usr/bin/env bash set -euo pipefail for spec in \ '`@perses-dev/loki-plugin`@0.6.0-rc.2' \ '`@perses-dev/stat-chart-plugin`@0.13.0-rc.1' \ '`@perses-dev/table-plugin`@0.13.0-rc.1'; do npm view "$spec" peerDependencies --json done npm install --ignore-scripts --strict-peer-deps --dry-runSource: MCP tools
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8726 +/- ##
==========================================
+ Coverage 60.67% 60.70% +0.03%
==========================================
Files 2979 2979
Lines 100608 100640 +32
Branches 23963 23967 +4
==========================================
+ Hits 61045 61095 +50
+ Misses 39563 39545 -18
... and 2 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
|
Note for future reference: This is safe for our dashboards because none of our 8 StatChart panels (4 ODH + 4 RHOAI in
When upgrading these overrides in the future, verify that the new version either includes #746 or that our stat panels still don't use sparklines. |
DaoDaoNoCode
left a comment
DaoDaoNoCode
left a comment
There was a problem hiding this comment.
/lgtm
The comment above is just a FYI for future developers, the fix in this PR is correct.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: DaoDaoNoCode 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 |
ae15d2e
into
opendatahub-io:main
2 participants
https://redhat.atlassian.net/browse/RHOAIENG-78528
Description
Bundles newer Perses plugin builds as local overrides so Loki-backed Table and StatChart panels use instant queries instead of always hitting
query_range.The COO-shipped Perses Loki plugin incorrectly sends range queries for panels that need instant queries, producing silently wrong values. Upstream fixes landed in:
This PR overrides:
@perses-dev/loki-pluginv0.6.0-rc.2@perses-dev/table-pluginv0.13.0-rc.1@perses-dev/stat-chart-pluginv0.13.0-rc.1Also registers the existing Prometheus override under the remote module name (
Prometheus) so bundled overrides match both the Perses API name and the npm package name.Compare the usage breakdown table:
Before:
After:
How Has This Been Tested?
Manually tested on a shared cluster: compared Loki-backed StatChart / Table panel values against direct Loki instant queries and confirmed results align with the patched plugin behavior.
Confirmed Model dashboard continues to load for a regular user.
Test Impact
No new automated tests. Coverage is a third-party Perses plugin version override; validation is against live Loki query behavior on a cluster.
Request review criteria:
Self checklist (all need to be checked):
If you have UI changes:
After the PR is posted & before it merges:
mainSummary by CodeRabbit