(feat): Add e2e health data to org pulse - #1385
Conversation
|
Claude finished @AjayJagan's task in 2m 24s —— View job PR Review: Add e2e health data to org pulse
This was a read-only review (fork PR). No autofixes were applied. This PR adds a substantial E2E health monitoring system for Blocking Issues
Non-blocking Issues
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| try { | ||
| const fs = require('fs'); | ||
| const path_module = require('path'); | ||
| const fullPath = path_module.join(process.cwd(), 'data', 'system-health/odh-e2e-health.json'); | ||
| if (fs.existsSync(fullPath)) { |
There was a problem hiding this comment.
Hard constraint #2 violation (blocking): This directly reads the filesystem using fs/path and process.cwd() instead of using the readFromStorage abstraction. The storage abstractions handle demo mode, path-traversal safety, and PVC mounts.
From AGENTS.md:
Always use
readFromStorage/writeToStoragefor data files. Never construct raw filesystem paths.
This function receives writeToStorage but needs readFromStorage as well to load existing trends. Consider passing readFromStorage alongside writeToStorage, or refactoring so the caller passes the existing data in.
| try { | |
| const fs = require('fs'); | |
| const path_module = require('path'); | |
| const fullPath = path_module.join(process.cwd(), 'data', 'system-health/odh-e2e-health.json'); | |
| if (fs.existsSync(fullPath)) { | |
| // Load existing data using storage abstraction instead of raw filesystem | |
| // TODO: Pass readFromStorage to this function and use it here | |
| const fs = require('fs'); | |
| const path_module = require('path'); | |
| const fullPath = path_module.join(process.cwd(), 'data', 'system-health/odh-e2e-health.json'); | |
| if (fs.existsSync(fullPath)) { | |
| existingData = JSON.parse(fs.readFileSync(fullPath, 'utf8')); | |
| } |
| ], | ||
| "hiddenRoutes": { | ||
| "disconnected-repo-detail": "component-maturity" | ||
| "disconnected-repo-detail": "component-maturity", |
There was a problem hiding this comment.
Bug: The hidden route key "test-run-detail" doesn't match the actual route ID "e2e-run-detail" registered in client/index.js. This means the detail view route will appear in the sidebar navigation instead of being hidden.
Should be:
| "disconnected-repo-detail": "component-maturity", | |
| "e2e-run-detail": "odh-e2e-health" |
| Zap, | ||
| Code | ||
| } from 'lucide-vue-next' | ||
| import { apiRequest } from '@shared/client/services/api' | ||
|
|
||
| const nav = inject('moduleNav', null) | ||
|
|
There was a problem hiding this comment.
Bug: Math.random() is used to generate mock test details (lines ~1199-1217 in the diff). This causes the UI to show different numbers on every re-render (since this is inside a computed property), which is a poor user experience — values flicker when the component re-evaluates.
Consider computing deterministic mock values based on the component name or build ID (e.g., a simple hash function), or generating them once in onMounted instead of inside a computed.
| (statusFilter.value === 'all' || run.status === statusFilter.value) | ||
| ) | ||
| }) | ||
|
|
There was a problem hiding this comment.
Bug: loadMoreRuns is referenced in the template but never defined in the <script setup> block. This will throw a runtime error if a user clicks "Load More Runs" when the runHistory?.pagination?.hasNextPage condition is true.
| // Skip infrastructure components entirely - they are not real user-facing component failures | ||
| return component !== 'infrastructure' && stats.failureRate > 0.10; | ||
| }) | ||
| .map(([component, stats]) => ({ | ||
| component, |
There was a problem hiding this comment.
Hard constraint #7 (API documentation): The @openapi annotation path is /api/modules/system-health/opendatahub-operator-e2e-health but the route is actually mounted at /api/modules/system-health/odh-e2e-health (see server/index.js line router.use('/odh-e2e-health', ...)). The OpenAPI path doesn't match the actual URL, which will cause confusion and potentially break API documentation validation.
| // Skip pending/running tests - only process completed tests | ||
| if (mappedStatus === null) { | ||
| return null; | ||
| } | ||
|
|
||
| return { | ||
| buildId: status.build_id || prowJob.metadata?.name || 'unknown', | ||
| jobName: job.job, | ||
| suite: suite, |
There was a problem hiding this comment.
Performance concern: The prowjobs.js endpoint returns ALL Prow jobs (the diff mentions ~15,000+ items) as a single JSON response. This is downloaded and parsed in-memory on every hourly refresh. At ~23MB per fetch, this will put significant memory pressure on the Node.js process, especially in a container with limited resources.
Consider:
- Setting a reasonable memory ceiling and monitoring for OOM
- Using streaming JSON parsing for very large payloads
- Caching the raw data and only re-fetching if newer data is available (e.g., using
If-Modified-Sinceheaders)
|
|
||
| const cutoffTime = new Date(Date.now() - daysBack * 24 * 60 * 60 * 1000); | ||
|
|
||
| for (const job of prowData.items) { |
There was a problem hiding this comment.
Performance: For every failed job, extractFailedComponents makes HTTP requests to fetch JUnit XML from GCS. With potentially dozens of failed jobs, this creates N sequential/parallel HTTP requests during each refresh cycle. Combined with the 23MB prowjobs download, this is very heavy for a "lightweight fetching" backend (hard constraint #3: "The app is a display layer, not a compute engine").
Consider whether this JUnit parsing should happen in an external pipeline that pushes pre-computed results, consistent with the project's architecture pattern for compute-heavy operations.
| status = 'error'; | ||
| logger.error('opendatahub-operator E2E health refresh failed:', err); | ||
|
|
||
| // For development, use demo storage if the real fetch fails and we have demo data | ||
| if (process.env.NODE_ENV !== 'production') { |
There was a problem hiding this comment.
Potential data loss: In the development error path, readFromStorage is used but it was destructured from storage at the top of the try block (line 128). If the error occurs before that destructuring succeeds (e.g., storage is undefined), this readFromStorage call will also throw, masking the original error.
Additionally, the require('./storage') call here creates a different getE2EHealthData that requires readFromStorage as a parameter — but the existing call at line 155 also passes readFromStorage from the same potentially-undefined scope.
| context: "degraded", | ||
| message: "Some components showing elevated failure rates", | ||
| components: actionableComponents.slice(0, 2), // Top 2 concerning | ||
| actionRequired: false |
There was a problem hiding this comment.
Security — No auth middleware on POST endpoint: The /refresh endpoint triggers a potentially expensive external API call (downloading 23MB from Prow, then fetching JUnit XMLs from GCS). None of the routes in this module apply requireAuth or requireAdmin. The POST /refresh endpoint should at minimum require admin authentication to prevent unauthenticated users from triggering expensive refresh operations.
Other modules in this codebase (e.g., quality, disconnected) wrap their admin-level routes with requireAdmin.
|
|
||
| const healthData = ref(null) | ||
| const runHistory = ref(null) | ||
| const loading = ref(false) | ||
| const error = ref(null) | ||
| let hasFetched = false |
There was a problem hiding this comment.
Bug — module-level mutable state: The healthData, runHistory, loading, error refs and the hasFetched flag are defined at module scope. This means they're shared as singletons across all components that import this composable — which is the intended behavior for a "shared store" pattern. However, the hasFetched flag is never reset, so if the initial fetch fails, calling useOdhOperatorE2eHealth() again from another component won't retry.
Consider resetting hasFetched on error, or providing a way to retry.
9aabac0 to
98ce03d
Compare
|
Claude finished @AjayJagan's task in 3m 6s —— View job PR Review: Add e2e health data to org pulse
This was a read-only review (fork PR). No autofixes were applied. This PR adds a substantial E2E health monitoring system for Blocking Issues
Non-blocking Issues
|
| const fs = require('fs'); | ||
| const path_module = require('path'); | ||
| const fullPath = path_module.join(process.cwd(), 'data', 'system-health/odh-e2e-health.json'); | ||
| if (fs.existsSync(fullPath)) { |
There was a problem hiding this comment.
Hard constraint #2 violation (blocking): maintainHistoricalTrends uses fs.readFileSync and process.cwd() to read data files directly, bypassing the readFromStorage abstraction. This function receives writeToStorage — add readFromStorage as a parameter and use it to load the existing data.
| if (fs.existsSync(fullPath)) { | |
| const existingData = await readFromStorage('system-health/odh-e2e-health.json'); |
The caller in saveE2EHealthData would also need to pass readFromStorage.
| ], | ||
| "hiddenRoutes": { | ||
| "disconnected-repo-detail": "component-maturity" | ||
| "disconnected-repo-detail": "component-maturity", |
There was a problem hiding this comment.
Bug (blocking): The hidden route key "test-run-detail" doesn't match the actual route ID "e2e-run-detail" registered in client/index.js. This will cause the detail view to appear in the sidebar instead of being hidden.
| "disconnected-repo-detail": "component-maturity", | |
| "e2e-run-detail": "odh-e2e-health" |
| (statusFilter.value === 'all' || run.status === statusFilter.value) | ||
| ) | ||
| }) | ||
|
|
There was a problem hiding this comment.
Bug (blocking): loadMoreRuns is referenced in the template but never defined in the <script setup> block. This will throw a runtime error when the "Load More Runs" button is clicked and runHistory?.pagination?.hasNextPage is true.
Either define loadMoreRuns using the existing loadRunHistory composable (with append: true), or remove this button.
| return component !== 'infrastructure' && stats.failureRate > 0.10; | ||
| }) | ||
| .map(([component, stats]) => ({ | ||
| component, |
There was a problem hiding this comment.
Hard constraint #7 violation (blocking): The @openapi annotation path is /api/modules/system-health/opendatahub-operator-e2e-health but the route is mounted at /api/modules/system-health/odh-e2e-health (see server/index.js line router.use('/odh-e2e-health', ...)). This mismatch will break OpenAPI validation and API documentation.
| component, | |
| * /api/modules/system-health/odh-e2e-health: |
| context: "degraded", | ||
| message: "Some components showing elevated failure rates", | ||
| components: actionableComponents.slice(0, 2), // Top 2 concerning | ||
| actionRequired: false |
There was a problem hiding this comment.
Security issue (blocking): The POST /refresh endpoint triggers expensive external API calls (downloading ~23MB from Prow + fetching JUnit XMLs) but has no authentication middleware. The context object provides requireAuth and requireAdmin — this endpoint should use at minimum requireAdmin to prevent unauthenticated abuse.
Other modules in this codebase protect admin-level endpoints with requireAdmin.
| const passRate = deduplicatedJobs.length > 0 ? passedJobs.length / deduplicatedJobs.length : 0; | ||
|
|
||
| const dailyStatus = calculateDailyStatus(passRate); | ||
|
|
There was a problem hiding this comment.
Bug: calculateConsecutiveFailures pre-filters runs to only those containing the component in failedComponents, then iterates the filtered set counting consecutive failures. Since every run in the filtered set already has the component in failedComponents, the loop always returns the total count of the filtered set — it never hits the else break. The function should operate on all runs sorted by time, not just failed ones:
function calculateConsecutiveFailures(runs, component) {
const sortedRuns = [...runs]
.sort((a, b) => new Date(b.timestamp) - new Date(a.timestamp));
let consecutive = 0;
for (const run of sortedRuns) {
if (run.failedComponents && run.failedComponents.includes(component)) {
consecutive++;
} else {
break;
}
}
return consecutive;
}
|
|
||
| /** | ||
| * Health check for the opendatahub-operator E2E scheduler | ||
| * @returns {Promise<Object>} Health status |
There was a problem hiding this comment.
Bug: getE2EHealthData() is called without passing readFromStorage as an argument. The function signature requires it: async function getE2EHealthData(readFromStorage). This will throw or return null, making the health check always report "no recent data".
The healthCheck function needs to accept a storage context parameter.
98ce03d to
ec75bfc
Compare
|
Claude finished @AjayJagan's task in 2m 34s —— View job PR Review: Add e2e health data to org pulse
This was a read-only review (fork PR). No autofixes were applied. This PR adds a substantial E2E health monitoring system for Previously Reported Issues — Now FixedThe following issues from the prior two reviews are resolved in this push:
Remaining Blocking Issues
Non-blocking Issues
|
| .filter(run => run.failedComponents && run.failedComponents.includes(component)) | ||
| .sort((a, b) => new Date(b.timestamp) - new Date(a.timestamp)); | ||
|
|
||
| let consecutive = 0; | ||
| for (const run of sortedRuns) { | ||
| if (run.failedComponents && run.failedComponents.includes(component)) { | ||
| consecutive++; | ||
| } else { | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| return consecutive; | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
Bug (blocking): calculateConsecutiveFailures pre-filters runs to only those where the component failed (.filter(run => run.failedComponents.includes(component))), then iterates the filtered set checking the same condition. Since every run in the filtered set already has the failure, the else break is never reached — the function always returns the total count of filtered runs, not actual consecutive failures.
The version in component-mapper.js is correct — it operates on all runs. This one should do the same:
| .filter(run => run.failedComponents && run.failedComponents.includes(component)) | |
| .sort((a, b) => new Date(b.timestamp) - new Date(a.timestamp)); | |
| let consecutive = 0; | |
| for (const run of sortedRuns) { | |
| if (run.failedComponents && run.failedComponents.includes(component)) { | |
| consecutive++; | |
| } else { | |
| break; | |
| } | |
| } | |
| return consecutive; | |
| } | |
| /** | |
| function calculateConsecutiveFailures(runs, component) { | |
| const sortedRuns = [...runs] | |
| .sort((a, b) => new Date(b.timestamp) - new Date(a.timestamp)); | |
| let consecutive = 0; | |
| for (const run of sortedRuns) { | |
| if (run.failedComponents && run.failedComponents.includes(component)) { | |
| consecutive++; | |
| } else { | |
| break; | |
| } | |
| } | |
| return consecutive; | |
| } |
|
|
||
| /** | ||
| * Health check for the opendatahub-operator E2E scheduler | ||
| * @returns {Promise<Object>} Health status |
There was a problem hiding this comment.
Bug (blocking): getE2EHealthData() requires readFromStorage as its first argument (see storage.js), but it's called here with no arguments. This will always return null, making the health check always report "no recent data."
The healthCheck function needs to accept a storage context parameter and pass readFromStorage through:
| * @returns {Promise<Object>} Health status | |
| const healthData = await getE2EHealthData(storage.readFromStorage); |
And the function signature should accept a storage parameter.
ec75bfc to
9cc4e9c
Compare
|
Claude finished @AjayJagan's task in 6m 34s —— View job PR Review: Add e2e health data to org pulse
This was a read-only review (fork PR). No autofixes were applied. This PR adds a substantial E2E health monitoring system for Previously Reported Issues — Now FixedAll blocking issues from the prior two reviews are resolved in this push:
Non-blocking Issues
|
| return new Promise((resolve, reject) => { | ||
| const timeout = setTimeout(() => { | ||
| req.abort(); | ||
| reject(new Error('JUnit XML fetch timeout')); | ||
| }, 10000); // 10 second timeout | ||
|
|
||
| const req = require('https').get(url, { timeout: 10000 }, (res) => { |
There was a problem hiding this comment.
Bug: The setTimeout callback on line 360 references req.abort(), but req is declared with const on line 364 — after the setTimeout. While this works in practice because the 10-second timeout fires long after req is assigned, it's fragile: if https.get() ever threw synchronously, req would remain in the temporal dead zone and the timeout callback would throw a ReferenceError.
Safer pattern — declare req before the timeout:
| return new Promise((resolve, reject) => { | |
| const timeout = setTimeout(() => { | |
| req.abort(); | |
| reject(new Error('JUnit XML fetch timeout')); | |
| }, 10000); // 10 second timeout | |
| const req = require('https').get(url, { timeout: 10000 }, (res) => { | |
| async function fetchJunitXml(url) { | |
| return new Promise((resolve, reject) => { | |
| let req; | |
| const timeout = setTimeout(() => { | |
| if (req) req.abort(); | |
| reject(new Error('JUnit XML fetch timeout')); | |
| }, 10000); // 10 second timeout | |
| req = require('https').get(url, { timeout: 10000 }, (res) => { |
| failures: 0, | ||
| lastFailure: null, | ||
| affectedSuites: new Set() | ||
| }; |
There was a problem hiding this comment.
Bug: runs.sort(...) mutates the input array in place. This function is called multiple times from calculateComponentStats while iterating over recentRuns — each call re-sorts the shared array, which can cause unpredictable ordering mid-iteration.
Use a copy to avoid mutating the caller's data:
| }; | |
| const sortedRuns = [...runs].sort((a, b) => new Date(b.timestamp) - new Date(a.timestamp)); |
The same in-place mutation issue exists in deduplicateRunsByPRAndSuite (line 103) and deduplicateLatestPerPR (line 191).
| /** | ||
| * Parse detailed test suite information from real JUnit XML artifacts | ||
| * @param {Object} runDetails - Basic run information | ||
| * @param {Function} readFromStorage - Storage read function | ||
| * @returns {Promise<Array>} Array of detailed test suite data | ||
| */ | ||
| async function parseTestSuiteDetails(runDetails, readFromStorage) { | ||
| try { | ||
| // For failed tests, try to fetch real JUnit XML from Prow artifacts | ||
| if (runDetails.status === 'failed' && runDetails.artifactsUrl) { | ||
| console.log(`Fetching real JUnit XML for build ${runDetails.buildId}`); | ||
|
|
||
| try { | ||
| const junitXml = await fetchJunitXmlFromArtifacts(runDetails.artifactsUrl); |
There was a problem hiding this comment.
Same bug as fetchJunitXml: The setTimeout callback references req before it's declared on line 437. While the 15-second delay makes this safe in practice, it's fragile. Use let req before the timeout for safety.
| /** | |
| * Parse detailed test suite information from real JUnit XML artifacts | |
| * @param {Object} runDetails - Basic run information | |
| * @param {Function} readFromStorage - Storage read function | |
| * @returns {Promise<Array>} Array of detailed test suite data | |
| */ | |
| async function parseTestSuiteDetails(runDetails, readFromStorage) { | |
| try { | |
| // For failed tests, try to fetch real JUnit XML from Prow artifacts | |
| if (runDetails.status === 'failed' && runDetails.artifactsUrl) { | |
| console.log(`Fetching real JUnit XML for build ${runDetails.buildId}`); | |
| try { | |
| const junitXml = await fetchJunitXmlFromArtifacts(runDetails.artifactsUrl); | |
| async function fetchJunitXmlFromArtifacts(artifactsUrl) { | |
| return new Promise((resolve, reject) => { | |
| let req; | |
| const timeout = setTimeout(() => { | |
| if (req) req.abort(); | |
| reject(new Error('JUnit XML fetch timeout')); | |
| }, 15000); // 15 second timeout for artifact fetching | |
| // Convert artifacts URL to direct JUnit XML URL | |
| const junitUrl = artifactsUrl.endsWith('/') | |
| ? artifactsUrl + 'artifacts/junit.xml' | |
| : artifactsUrl + '/artifacts/junit.xml'; | |
| console.log(`Fetching JUnit XML from: ${junitUrl}`); | |
| req = require('https').get(junitUrl, { timeout: 15000 }, (res) => { |
8e662af to
8981d89
Compare
|
Claude finished @AjayJagan's task in 3m 16s —— View job PR Review: Add e2e health data to org pulse
This was a read-only review (fork PR). No autofixes were applied. This PR adds a substantial E2E health monitoring system for Previously Reported Issues — Now FixedAll blocking issues from the prior five reviews are resolved in this push:
Non-blocking Issues
|
| return calculateCurrent14DayStatus('odh') | ||
| } | ||
|
|
||
| function getRhoaiTrend() { | ||
| return calculateCurrent14DayStatus('rhoai') | ||
| } | ||
|
|
||
| // Weekly aggregation using real Prow API data from recentRuns | ||
| const weeklyChartData = computed(() => { | ||
| // Use historical trends data instead of individual recent runs | ||
| const historicalTrends = healthData.value?.historical_trends | ||
| if (!historicalTrends?.daily_status || historicalTrends.daily_status.length === 0) { | ||
| return null | ||
| } | ||
|
|
||
| const dailyStatus = historicalTrends.daily_status | ||
| if (dailyStatus.length < 2) { | ||
| return null // Need at least 2 days for trend visualization | ||
| } | ||
|
|
There was a problem hiding this comment.
Performance (non-blocking): getOdhTrend() and getRhoaiTrend() are called 4+ times each in the template (passRate, passedRuns, totalRuns, status). These are regular functions that recalculate 14-day status with filtering, deduplication, and sorting on each call. Consider caching as computed properties:
| return calculateCurrent14DayStatus('odh') | |
| } | |
| function getRhoaiTrend() { | |
| return calculateCurrent14DayStatus('rhoai') | |
| } | |
| // Weekly aggregation using real Prow API data from recentRuns | |
| const weeklyChartData = computed(() => { | |
| // Use historical trends data instead of individual recent runs | |
| const historicalTrends = healthData.value?.historical_trends | |
| if (!historicalTrends?.daily_status || historicalTrends.daily_status.length === 0) { | |
| return null | |
| } | |
| const dailyStatus = historicalTrends.daily_status | |
| if (dailyStatus.length < 2) { | |
| return null // Need at least 2 days for trend visualization | |
| } | |
| const odhTrend = computed(() => calculateCurrent14DayStatus('odh')) | |
| const rhoaiTrend = computed(() => calculateCurrent14DayStatus('rhoai')) |
Then reference odhTrend / rhoaiTrend in the template instead of calling the functions repeatedly.
| logger.info('Updating historical trends from accumulated data...'); | ||
| await updateHistoricalTrends(healthData, accumulatedRuns, logger); | ||
|
|
There was a problem hiding this comment.
Redundancy (non-blocking): updateHistoricalTrends() at step 10 (line 137) computes up to 30 days of daily trends from all accumulated runs. Then saveE2EHealthData() at step 11 calls maintainHistoricalTrends() which overwrites data.historical_trends with a 14-day version. The step 10 computation is wasted.
Consider removing updateHistoricalTrends() from the refresh flow and letting maintainHistoricalTrends() in saveE2EHealthData be the single source of truth for trends, or vice versa — pick one and remove the other.
| } | ||
|
|
||
| try { | ||
| const params = new URLSearchParams() | ||
| params.append('page', page) |
There was a problem hiding this comment.
Cleanup (non-blocking): Several console.log statements are left in the client composable (loadHealthData, refreshData). These will log to the user's browser console in production. Consider removing them or guarding behind a dev-only check.
8981d89 to
a711201
Compare
|
Claude finished @AjayJagan's task in 4m 25s —— View job PR Review: Add e2e health data to org pulse
This was a read-only review (fork PR). No autofixes were applied. This PR adds a substantial E2E health monitoring system for Previously Reported Issues — Now FixedAll blocking issues from the prior six reviews are resolved in this push:
Non-blocking Issues
|
| const detailsResponse = await apiRequest(`/modules/system-health/odh-e2e-health/runs/${buildId}/details`) | ||
| testSuites.value = detailsResponse.testSuites || [] | ||
| } catch (detailError) { | ||
| console.log('Detailed test suites not available:', detailError.message) |
There was a problem hiding this comment.
Cleanup (non-blocking): This console.log will appear in users' browser console in production. Consider removing or guarding behind a dev check.
| console.log('Detailed test suites not available:', detailError.message) | |
| // Detailed test suites may not be available in demo mode - this is expected |
| ], | ||
| "last_updated": "2026-08-11T10:00:00.000Z" | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
Minor: Missing newline at end of file.
a711201 to
baf69b5
Compare
|
Claude finished @AjayJagan's task in 2m 58s —— View job PR Review: Add e2e health data to org pulse
This was a read-only review (fork PR). No autofixes were applied. This PR adds a substantial E2E health monitoring system for Previously Reported Issues — Now FixedAll blocking issues from the prior seven reviews are resolved in this push:
Remaining Blocking Issue
Non-blocking Issues
|
| @@ -0,0 +1,413 @@ | |||
| /** | |||
There was a problem hiding this comment.
Hard constraint #8 (blocking): This PR introduces a new data format (system-health/odh-e2e-health.json) with fixture data, but docs/DATA-FORMATS.md is not updated to document it. Per AGENTS.md:
Documentation changes must land in the same PR as the code they describe:
- Data format changes → update
docs/DATA-FORMATS.mdandfixtures/
The fixture file is correctly added — please also add the schema documentation to docs/DATA-FORMATS.md covering the top-level keys (lastSyncedAt, suites, summary, recentRuns, accumulatedRuns, componentStats, historical_trends, etc.).
baf69b5 to
6b32c5a
Compare
|
Claude finished @AjayJagan's task in 3m 12s —— View job PR Review: Add e2e health data to org pulse
This was a read-only review (fork PR). No autofixes were applied. This PR adds a substantial E2E health monitoring system for Previously Reported Issues — Now FixedAll blocking issues from the prior eight reviews are resolved in this push:
No New Blocking IssuesNo blocking issues remain. All hard constraints are satisfied, security is properly handled, and the bugs from prior reviews are fixed. Non-blocking Issues
|
| const currentlyBlockingComponents = computed(() => { | ||
| if (!healthData.value?.topFailingComponents) return [] | ||
| return healthData.value.topFailingComponents.filter(component => | ||
| component.consecutiveFailures > 0 | ||
| ) |
There was a problem hiding this comment.
Bug (non-blocking — latent, not currently triggered): topFailingComponents is an object { context, message, components, actionRequired } returned by the API (see generateContextualFailingComponents in routes.js), not an array. Calling .filter() on it would throw TypeError. Same issue affects componentTrends (line 163) which calls .reduce().
These computed properties aren't currently accessed by any template, so this won't crash the UI today — but it will the moment someone uses currentlyBlockingComponents, overallHealth, or componentTrends.
| const currentlyBlockingComponents = computed(() => { | |
| if (!healthData.value?.topFailingComponents) return [] | |
| return healthData.value.topFailingComponents.filter(component => | |
| component.consecutiveFailures > 0 | |
| ) | |
| const currentlyBlockingComponents = computed(() => { | |
| if (!healthData.value?.topFailingComponents?.components) return [] | |
| return healthData.value.topFailingComponents.components.filter(component => | |
| component.consecutiveFailures > 0 | |
| ) | |
| }) |
| const componentTrends = computed(() => { | ||
| if (!healthData.value?.topFailingComponents) return {} | ||
|
|
||
| return healthData.value.topFailingComponents.reduce((trends, component) => { | ||
| trends[component.component] = { | ||
| name: component.component, | ||
| failureRate: component.failureRate, | ||
| consecutiveFailures: component.consecutiveFailures, | ||
| trend: component.failureRate > 0.3 ? 'worsening' : | ||
| component.failureRate < 0.1 ? 'improving' : 'stable', | ||
| severity: component.failureRate > 0.5 ? 'high' : | ||
| component.failureRate > 0.2 ? 'medium' : 'low' | ||
| } | ||
| return trends | ||
| }, {}) | ||
| }) |
There was a problem hiding this comment.
Same latent bug as currentlyBlockingComponents: .reduce() on the topFailingComponents object will throw if accessed. Should use .components array:
| const componentTrends = computed(() => { | |
| if (!healthData.value?.topFailingComponents) return {} | |
| return healthData.value.topFailingComponents.reduce((trends, component) => { | |
| trends[component.component] = { | |
| name: component.component, | |
| failureRate: component.failureRate, | |
| consecutiveFailures: component.consecutiveFailures, | |
| trend: component.failureRate > 0.3 ? 'worsening' : | |
| component.failureRate < 0.1 ? 'improving' : 'stable', | |
| severity: component.failureRate > 0.5 ? 'high' : | |
| component.failureRate > 0.2 ? 'medium' : 'low' | |
| } | |
| return trends | |
| }, {}) | |
| }) | |
| const componentTrends = computed(() => { | |
| if (!healthData.value?.topFailingComponents?.components) return {} | |
| return healthData.value.topFailingComponents.components.reduce((trends, component) => { | |
| trends[component.component] = { | |
| name: component.component, | |
| failureRate: component.failureRate, | |
| consecutiveFailures: component.consecutiveFailures, | |
| trend: component.failureRate > 0.3 ? 'worsening' : | |
| component.failureRate < 0.1 ? 'improving' : 'stable', | |
| severity: component.failureRate > 0.5 ? 'high' : | |
| component.failureRate > 0.2 ? 'medium' : 'low' | |
| } | |
| return trends | |
| }, {}) | |
| }) |
6b32c5a to
943f5f5
Compare
|
Claude finished @AjayJagan's task in 3m 15s —— View job PR Review: Add e2e health data to org pulse
This was a read-only review (fork PR). No autofixes were applied. This PR adds a substantial E2E health monitoring system for Previously Reported Issues — Now FixedAll blocking issues from the prior nine reviews are resolved in this push:
No New Blocking IssuesNo blocking issues remain. All hard constraints are satisfied, security is properly handled, and all previously identified bugs are fixed. Non-blocking Issues
|
| const currentlyBlockingComponents = computed(() => { | ||
| if (!healthData.value?.topFailingComponents) return [] | ||
| return healthData.value.topFailingComponents.filter(component => | ||
| component.consecutiveFailures > 0 | ||
| ) | ||
| }) |
There was a problem hiding this comment.
Bug (non-blocking — latent, not currently triggered): topFailingComponents is an object { context, message, components, actionRequired } from generateContextualFailingComponents() in routes.js, not an array. Calling .filter() on it throws TypeError. Same issue affects componentTrends (line 165) which calls .reduce(), and overallHealth (line 128) which reads currentlyBlockingComponents.value.length.
None of these computed properties are currently accessed by any template, so this won't crash today — but they're part of the composable's public API and will break the moment someone uses them.
| const currentlyBlockingComponents = computed(() => { | |
| if (!healthData.value?.topFailingComponents) return [] | |
| return healthData.value.topFailingComponents.filter(component => | |
| component.consecutiveFailures > 0 | |
| ) | |
| }) | |
| const currentlyBlockingComponents = computed(() => { | |
| if (!healthData.value?.topFailingComponents?.components) return [] | |
| return healthData.value.topFailingComponents.components.filter(component => | |
| component.consecutiveFailures > 0 | |
| ) | |
| }) |
| const componentTrends = computed(() => { | ||
| if (!healthData.value?.topFailingComponents) return {} | ||
|
|
||
| return healthData.value.topFailingComponents.reduce((trends, component) => { | ||
| trends[component.component] = { | ||
| name: component.component, | ||
| failureRate: component.failureRate, | ||
| consecutiveFailures: component.consecutiveFailures, | ||
| trend: component.failureRate > 0.3 ? 'worsening' : | ||
| component.failureRate < 0.1 ? 'improving' : 'stable', | ||
| severity: component.failureRate > 0.5 ? 'high' : | ||
| component.failureRate > 0.2 ? 'medium' : 'low' | ||
| } | ||
| return trends | ||
| }, {}) | ||
| }) |
There was a problem hiding this comment.
Same latent bug as currentlyBlockingComponents: .reduce() on the topFailingComponents object will throw TypeError if accessed. Should use .components array:
| const componentTrends = computed(() => { | |
| if (!healthData.value?.topFailingComponents) return {} | |
| return healthData.value.topFailingComponents.reduce((trends, component) => { | |
| trends[component.component] = { | |
| name: component.component, | |
| failureRate: component.failureRate, | |
| consecutiveFailures: component.consecutiveFailures, | |
| trend: component.failureRate > 0.3 ? 'worsening' : | |
| component.failureRate < 0.1 ? 'improving' : 'stable', | |
| severity: component.failureRate > 0.5 ? 'high' : | |
| component.failureRate > 0.2 ? 'medium' : 'low' | |
| } | |
| return trends | |
| }, {}) | |
| }) | |
| const componentTrends = computed(() => { | |
| if (!healthData.value?.topFailingComponents?.components) return {} | |
| return healthData.value.topFailingComponents.components.reduce((trends, component) => { | |
| trends[component.component] = { | |
| name: component.component, | |
| failureRate: component.failureRate, | |
| consecutiveFailures: component.consecutiveFailures, | |
| trend: component.failureRate > 0.3 ? 'worsening' : | |
| component.failureRate < 0.1 ? 'improving' : 'stable', | |
| severity: component.failureRate > 0.5 ? 'high' : | |
| component.failureRate > 0.2 ? 'medium' : 'low' | |
| } | |
| return trends | |
| }, {}) | |
| }) |
943f5f5 to
c918b80
Compare
|
Claude finished @AjayJagan's task in 2m 23s —— View job PR Review: Add e2e health data to org pulse
This was a read-only review (fork PR). No autofixes were applied. This PR adds a substantial E2E health monitoring system for Previously Reported Issues — Now FixedAll blocking issues from the prior nine reviews are resolved in this push:
No New Blocking IssuesNo blocking issues remain. All hard constraints are satisfied, security is properly handled, and all previously identified bugs are fixed. Non-blocking Issues
|
| const currentlyBlockingComponents = computed(() => { | ||
| if (!healthData.value?.topFailingComponents) return [] | ||
| return healthData.value.topFailingComponents.filter(component => | ||
| component.consecutiveFailures > 0 | ||
| ) | ||
| }) |
There was a problem hiding this comment.
Bug (non-blocking — latent, not currently triggered): topFailingComponents is an object { context, message, components, actionRequired } returned by generateContextualFailingComponents() in routes.js, not an array. Calling .filter() on it will throw TypeError. Same issue affects componentTrends (line 165) which calls .reduce(), and overallHealth (line 126) which reads currentlyBlockingComponents.value.length.
None of these computed properties are currently accessed by any template, so this won't crash today — but they're part of the composable's public API and will break the moment someone uses them.
| const currentlyBlockingComponents = computed(() => { | |
| if (!healthData.value?.topFailingComponents) return [] | |
| return healthData.value.topFailingComponents.filter(component => | |
| component.consecutiveFailures > 0 | |
| ) | |
| }) | |
| const currentlyBlockingComponents = computed(() => { | |
| if (!healthData.value?.topFailingComponents?.components) return [] | |
| return healthData.value.topFailingComponents.components.filter(component => | |
| component.consecutiveFailures > 0 | |
| ) | |
| }) |
| const componentTrends = computed(() => { | ||
| if (!healthData.value?.topFailingComponents) return {} | ||
|
|
||
| return healthData.value.topFailingComponents.reduce((trends, component) => { | ||
| trends[component.component] = { | ||
| name: component.component, | ||
| failureRate: component.failureRate, | ||
| consecutiveFailures: component.consecutiveFailures, | ||
| trend: component.failureRate > 0.3 ? 'worsening' : | ||
| component.failureRate < 0.1 ? 'improving' : 'stable', | ||
| severity: component.failureRate > 0.5 ? 'high' : | ||
| component.failureRate > 0.2 ? 'medium' : 'low' | ||
| } | ||
| return trends | ||
| }, {}) | ||
| }) |
There was a problem hiding this comment.
Same latent bug as currentlyBlockingComponents: .reduce() on the topFailingComponents object will throw TypeError if accessed. Should use .components array:
| const componentTrends = computed(() => { | |
| if (!healthData.value?.topFailingComponents) return {} | |
| return healthData.value.topFailingComponents.reduce((trends, component) => { | |
| trends[component.component] = { | |
| name: component.component, | |
| failureRate: component.failureRate, | |
| consecutiveFailures: component.consecutiveFailures, | |
| trend: component.failureRate > 0.3 ? 'worsening' : | |
| component.failureRate < 0.1 ? 'improving' : 'stable', | |
| severity: component.failureRate > 0.5 ? 'high' : | |
| component.failureRate > 0.2 ? 'medium' : 'low' | |
| } | |
| return trends | |
| }, {}) | |
| }) | |
| const componentTrends = computed(() => { | |
| if (!healthData.value?.topFailingComponents?.components) return {} | |
| return healthData.value.topFailingComponents.components.reduce((trends, component) => { | |
| trends[component.component] = { | |
| name: component.component, | |
| failureRate: component.failureRate, | |
| consecutiveFailures: component.consecutiveFailures, | |
| trend: component.failureRate > 0.3 ? 'worsening' : | |
| component.failureRate < 0.1 ? 'improving' : 'stable', | |
| severity: component.failureRate > 0.5 ? 'high' : | |
| component.failureRate > 0.2 ? 'medium' : 'low' | |
| } | |
| return trends | |
| }, {}) | |
| }) |
Signed-off-by: Ajay Jaganathan <36824134+AjayJagan@users.noreply.github.com>
c918b80 to
ef475ec
Compare
|
Claude finished @AjayJagan's task in 2m 31s —— View job PR Review: Add e2e health data to org pulse
This was a read-only review (fork PR). No autofixes were applied. This PR adds a substantial E2E health monitoring system for Previously Reported Issues — Now FixedAll blocking issues from the prior nine reviews are resolved in this push:
No New Blocking IssuesNo blocking issues remain. All hard constraints are satisfied, security is properly handled, and all previously identified bugs are fixed. Non-blocking Issues
|
|
|
||
| const componentData = computed(() => { | ||
| if (!healthData.value?.topFailingComponents) return null | ||
| return healthData.value.topFailingComponents.find(c => c.component === componentName) |
There was a problem hiding this comment.
Bug (non-blocking — latent, not currently triggered): topFailingComponents is an object { context, message, components, actionRequired } returned by generateContextualFailingComponents() in routes.js, not an array. Calling .find() on it will throw TypeError: topFailingComponents.find is not a function.
This function is exported but not currently called by any template, so it won't crash today — but it will the moment someone uses useComponentAnalysis.
| return healthData.value.topFailingComponents.find(c => c.component === componentName) | |
| if (!healthData.value?.topFailingComponents?.components) return null | |
| return healthData.value.topFailingComponents.components.find(c => c.component === componentName) |
|
|
||
| const passRate = healthData.value.overallPassRate || 0 | ||
| const totalRuns = healthData.value.totalRuns || 0 | ||
| const failingComponents = healthData.value.topFailingComponents?.length || 0 |
There was a problem hiding this comment.
Bug (non-blocking — latent, not currently triggered): topFailingComponents is an object, not an array, so .length is undefined — failingComponents will always be 0. This means isHealthy and needsAttention (lines 295-296) never account for failing components.
| const failingComponents = healthData.value.topFailingComponents?.length || 0 | |
| const failingComponents = healthData.value.topFailingComponents?.components?.length || 0 |
2 participants
Description
This implementation establishes a complete end-to-end health monitoring system for the opendatahub-operator that intelligently tracks test results across ODH and RHOAI suites. The system features smart incremental data accumulation that fetches only recent data from the Prow CI API, merges it with existing runs while preventing duplicates, and maintains a rolling 30-day dataset through automatic eviction of old entries. Key capabilities include real-time component failure analysis with contextual insights, dynamic historical trend calculation updated hourly from accumulated data, detailed test run tracking with JUnit XML parsing for failure diagnosis, and comprehensive suite health monitoring with traffic-light status indicators.