feat: add Delivery Insights (DORA metrics) page to the sidebar - #729
Draft
LakshanSS wants to merge 12 commits into
Draft
feat: add Delivery Insights (DORA metrics) page to the sidebar#729LakshanSS wants to merge 12 commits into
LakshanSS wants to merge 12 commits into
Conversation
…and component levels Adds an Insights tab to the domain (Namespace), system (Project), and component entity pages, backed by the observer's new DORA read API (openchoreo#3668). The tab hosts two inner views per the Insights design: - Delivery Insights: four DORA KPI tiles (value, DORA classification, delta vs previous window, sparkline), trend charts with range (7d/30d/90d/12mo) and granularity (daily/weekly/monthly) controls, an environment filter, a one-level-down breakdown table (namespace: by project, project: by component, component: by environment) with row drill-down into the child's Insights tab (environment rows apply the env filter instead), per-environment metric cards, and a how-it-is-calculated footnote. - Cost Insights: embeds the existing FinOps cost analysis at project level (drill-down preserved under /insights/cost); other levels point to the project pages until cost lands there. Supporting changes: getDoraMetrics/getDoraDeployments on the observability API client, and namespace-level observer URL resolution (resolve-urls without environmentName resolves through the namespace's environments) for the cross-environment scopes. Signed-off-by: LakshanSS <lakshan230897@gmail.com>
The sparkline is absolutely positioned in the tile's bottom-right corner, so long footer text (e.g. the lead-time coverage line) flowed underneath it. Reserve the sparkline's width as footer padding and let the text wrap. Signed-off-by: LakshanSS <lakshan230897@gmail.com>
Signed-off-by: LakshanSS <lakshan230897@gmail.com>
The test asserted the pre-namespace-resolution error message
('namespaceName and environmentName are required'), but environmentName
has been optional since resolve-urls gained namespace-level resolution.
Also adds coverage for the omitted-environmentName success path.
Signed-off-by: LakshanSS <lakshan230897@gmail.com>
resolveForNamespace cached results keyed only by namespaceName, so a namespace result resolved for one user could be served to a different user who cannot access that namespace or its environments. Scope the cache key to the caller's token, matching per-request authorization. Signed-off-by: LakshanSS <lakshan230897@gmail.com>
InsightsContent's environment filter narrowed the KPI tiles/charts but not the project/component breakdown table: useDoraBreakdown rebuilt each child scope without scope.environment, so the table always showed all environments. Also include environment in scopeKey so the breakdown effect refires when only the environment filter changes. Signed-off-by: LakshanSS <lakshan230897@gmail.com>
An unrounded rate (e.g. 1.1428571428571428) printed the raw float in the KPI tile instead of a formatted value like 1.14/day. Signed-off-by: LakshanSS <lakshan230897@gmail.com>
Resolves conflicts with the cost insights view (openchoreo#723): both PRs independently added new types/methods to ObservabilityApi.ts and types.ts, kept both sides. Signed-off-by: LakshanSS <lakshan230897@gmail.com>
Surface the four DORA metrics as a standalone Delivery Insights page reached from the sidebar, scoped by a namespace/project/component breadcrumb, instead of as an Insights tab on entity pages. The audience for delivery performance is engineering leadership looking across an organisation, whereas entity pages are a developer's view of one component — so this follows the placement Cost Insights established. - KPI tiles for Deployment Frequency, Lead Time for Changes, Change Failure Rate and MTTR, each with its DORA classification, delta vs the previous equal window, and a sparkline; one trend chart per metric at daily/weekly/monthly granularity (lead time as p50/p75/p95). - A one-level-down breakdown table (namespace to projects, project to components, component to environments) sorted by deployment frequency, each row carrying its own metrics and an overall rating (the scope's weakest tier). Project/component rows drill the page scope down; environment rows apply the environment filter. - Per-environment metric cards, an environment filter, and a "how these metrics are calculated" footnote. - Scope, range, granularity and environment all live in the URL, so a view can be bookmarked or shared. - The namespace/project/component picker is extracted as a shared ScopeBreadcrumb, now used by both Delivery and Cost Insights. Data comes from the observer's insights endpoints via new getDoraMetrics / getDoraDeployments client methods. Observer URL resolution gains namespace-level support so the org-wide scope can resolve without an environment. Signed-off-by: LakshanSS <lakshan230897@gmail.com>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR adds Delivery Insights with DORA metrics, scoped navigation, drill-down views, environment filtering, URL-persisted state, observability APIs, namespace-level URL resolution, and portal routing. Cost Insights now uses the shared ChangesDelivery Insights
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Portal
participant DeliveryInsightsPage
participant ScopeBreadcrumb
participant DeliveryInsightsContent
participant ObservabilityApi
Portal->>DeliveryInsightsPage: render /delivery-insights
DeliveryInsightsPage->>ScopeBreadcrumb: provide URL-derived scope
ScopeBreadcrumb-->>DeliveryInsightsPage: return scope selection or drill-down
DeliveryInsightsPage->>DeliveryInsightsContent: provide scope and filters
DeliveryInsightsContent->>ObservabilityApi: query DORA metrics and deployments
ObservabilityApi-->>DeliveryInsightsContent: return typed DORA data
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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 |
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (7)
plugins/openchoreo-observability/src/components/DeliveryInsights/DeliveryInsightsPage.tsx (2)
152-161: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueGuard
onDrillat component level.At component level the breakdown rows are environments, and the content applies them through
onEnvFilterChange.onDrillhas no guard for that level. If it is ever called there, it replacescomponentwith an environment name.♻️ Proposed change
const onDrill = useCallback( (childName: string) => { + // Component-level rows are environments; the content handles those. + if (component) return; if (project) { onScopeChange({ namespace, project, component: childName }); } else { onScopeChange({ namespace, project: childName }); } }, - [namespace, project, onScopeChange], + [namespace, project, component, onScopeChange], );🤖 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 `@plugins/openchoreo-observability/src/components/DeliveryInsights/DeliveryInsightsPage.tsx` around lines 152 - 161, Update the onDrill callback to guard against component-level scope before applying namespace/project drill-down changes; at that level, route the selected environment through onEnvFilterChange instead of assigning it to component, while preserving the existing project and namespace behavior.
95-102: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse the functional updater form of
setSearchParams.
updatepassesnextback to React instead of returning it, so the closure still reads the render-timesearchParams. Use the functional form to build from the latest params before calling the mutator.🤖 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 `@plugins/openchoreo-observability/src/components/DeliveryInsights/DeliveryInsightsPage.tsx` around lines 95 - 102, Update the useCallback named update to use setSearchParams’s functional updater: receive the latest URLSearchParams, clone them, apply mutator, and return the updated params. Remove the render-time searchParams dependency if it is no longer referenced, while preserving replace: true.plugins/openchoreo-observability/src/components/DeliveryInsights/DoraBreakdownTable.test.tsx (2)
95-101: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis test cannot fail, and the worst-tier rule has no coverage.
The assertion re-checks the text that the earlier test already checks, so the test passes whether or not the row is interactive. Assert the observable property instead: a non-clickable row carries no
cursor: pointer.
overallRatingis the only non-trivial logic in the component, and no test covers its worst-tier selection.💚 Proposed tests
it('does not act on a row click when no handler is supplied', async () => { // The component level passes neither handler for entity-backed rows; the // row must simply not be interactive rather than navigating anywhere. await renderTable({ onDrill: undefined }); - fireEvent.click(screen.getByText('checkout')); - expect(screen.getByText('checkout')).toBeInTheDocument(); + const row = screen.getByText('checkout').closest('tr')!; + expect(row).not.toHaveStyle('cursor: pointer'); }); + + it('rates a row by its weakest known tier', async () => { + const mixed = summary(30, 'Elite'); + mixed!.changeFailureRate!.classification = 'Low'; + mixed!.mttr!.classification = 'Unknown'; + await renderTable({ rows: [{ ...projectRow, summary: mixed }] }); + expect(screen.getByText('Low')).toBeInTheDocument(); + expect(screen.queryByText('Elite')).not.toBeInTheDocument(); + });🤖 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 `@plugins/openchoreo-observability/src/components/DeliveryInsights/DoraBreakdownTable.test.tsx` around lines 95 - 101, Strengthen the no-handler test around renderTable by asserting the clicked row does not expose cursor: pointer, rather than rechecking its text. Also add coverage for overallRating’s worst-tier selection, verifying the component chooses the lowest applicable rating when multiple tiers are present.
6-49: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winType the fixture so the
as anycasts disappear.
classificationis typed asstring, so lines 42 and 48 needas any. Those casts remove the type check that would catch a drift between the fixture andDoraMetricsResponse['summary'].♻️ Proposed change
-import type { DoraBreakdownRow } from './useDoraBreakdown'; +import type { DoraBreakdownRow } from './useDoraBreakdown'; +import type { DoraClassification } from '../../types'; -const summary = (total: number, classification: string) => ({ +const summary = ( + total: number, + classification: DoraClassification, +): DoraBreakdownRow['summary'] => ({Then drop
as anyon both rows:- summary: summary(30, 'Elite') as any, + summary: summary(30, 'Elite'),- summary: summary(10, 'High') as any, + summary: summary(10, 'High'),🤖 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 `@plugins/openchoreo-observability/src/components/DeliveryInsights/DoraBreakdownTable.test.tsx` around lines 6 - 49, Update the summary fixture helper to type its classification parameter and returned object against DoraMetricsResponse['summary'], using the existing classification type rather than string. Ensure summary(30, 'Elite') and summary(10, 'High') satisfy that type, then remove the as any casts from projectRow.summary and envRow.summary.plugins/openchoreo-observability/src/components/DeliveryInsights/DoraBreakdownTable.tsx (1)
221-230: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe delta in the rating column is frequency-only, and its colors bypass the theme.
Two points in this block:
deltacomes fromdf?.deltaPctat Line 162, so the percentage sits in the "DORA rating" column while it describes deployment frequency. Add atitleso the meaning is explicit.- The colors are hardcoded hex values. They do not follow the theme, so contrast can drop in dark mode. Use
theme.palette.success.mainandtheme.palette.error.main.♻️ Proposed change
{delta !== null && delta !== 0 && ( <Typography component="span" className={classes.miniDelta} - style={{ color: delta > 0 ? '`#1e7e34`' : '`#c62828`' }} + title="Change in deployment frequency vs the previous window" + color={delta > 0 ? 'primary' : 'error'} >Define the positive color through the theme in
useStylesifprimaryis not the wanted tone.🤖 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 `@plugins/openchoreo-observability/src/components/DeliveryInsights/DoraBreakdownTable.tsx` around lines 221 - 230, Update the delta Typography rendering in DoraBreakdownTable to add a title clarifying that the percentage represents deployment frequency, not the DORA rating. Replace the hardcoded positive and negative hex colors with theme-derived success and error palette values, defining any needed color in useStyles.packages/openchoreo-client-node/src/observability-url-resolver.test.ts (1)
46-84: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winThe isolation test passes for a reason unrelated to token partitioning.
User B returns an empty environment list, so the code throws before it reaches any
env:cache entry. The test therefore proves only that thens:key is token-specific. Add a case where user B lists the same environment namedevbut its plane lookup fails. That case exercises theenv:${namespace}/${envName}cache entry that user A populated, which is the path described in the comment onresolveForNamespace.🤖 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 `@packages/openchoreo-client-node/src/observability-url-resolver.test.ts` around lines 46 - 84, Update the isolation test around resolveForNamespace so user B returns the same “dev” environment while its plane lookup fails, rather than returning an empty list. Configure getB to provide the environment listing and then return a failed response for the subsequent plane lookup, and assert the failure reflects user B’s lookup instead of user A’s cached observer URL. Keep the existing user A setup and token-routed client creation, ensuring the env:${namespace}/${envName} cache path is exercised.plugins/openchoreo-observability/src/components/DeliveryInsights/DeliveryInsightsContent.tsx (1)
100-104: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSkip environment-card queries when the cards are hidden.
When
envFilteris set at the domain or system level, this call passeseffectiveScope, but Lines 358-369 hidebreakdown.envRows. The supplieduseDoraBreakdown.tscontext at Lines 37-196 still creates one environment summary request for every namespace environment. Add aloadEnvironmentRowsinput and disable those requests while an environment filter is active.Also applies to: 358-369
🤖 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 `@plugins/openchoreo-observability/src/components/DeliveryInsights/DeliveryInsightsContent.tsx` around lines 100 - 104, Update useDoraBreakdown to accept a loadEnvironmentRows option and guard the environment summary requests with it. In DeliveryInsightsContent, pass false whenever envFilter is active so domain/system filtered views do not fetch hidden breakdown.envRows cards, while preserving environment-row loading when no filter is applied.
🤖 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/openchoreo-client-node/src/observability-url-resolver.ts`:
- Around line 152-203: The environment-level cache is not partitioned by token,
allowing users to reuse each other’s cached results. In
packages/openchoreo-client-node/src/observability-url-resolver.ts lines 152-203,
update resolveForEnvironment’s env: cache key to include the token; in
packages/openchoreo-client-node/src/observability-url-resolver.test.ts lines
46-84, add a case where user B lists the same dev environment but its plane
lookup fails, ensuring the test exercises user A’s populated env: entry.
In
`@plugins/openchoreo-observability-backend/src/services/ObservabilityService.ts`:
- Around line 55-57: Update the namespace-scoped branch in ObservabilityService
to avoid returning only the first observer URL when environments in the
namespace may use different observers: either validate and enforce a single
observer URL per namespace, or aggregate requests across every namespace
observer. Add coverage for multiple environments resolving to different
observers and preserve the existing environment-specific behavior.
In
`@plugins/openchoreo-observability/src/components/DeliveryInsights/DeliveryInsightsContent.tsx`:
- Around line 169-176: Update the refresh flow in DeliveryInsightsContent so the
Refresh button’s refetch handler reloads both headline metrics and the
useDoraBreakdown data. Expose and invoke the breakdown refetch function or pass
an equivalent reload key, ensuring tables and environment cards refresh with the
KPI tiles and charts.
- Around line 223-232: Update the Change Failure Rate value expression in the
DoraMetricTile to display an em dash unless cfr exists and cfr.total is greater
than zero; only call formatPercent(cfr.rate) for nonzero deployment totals,
matching the policy used by DoraEnvironmentCards.
In
`@plugins/openchoreo-observability/src/components/DeliveryInsights/DoraTrendChart.tsx`:
- Around line 47-75: Update DoraTrendChart’s chartData construction and axis
configuration so sparse leadTime/mttr buckets are retained in chronological
order with null metric values, causing Recharts to render gaps instead of
connecting across unmeasured periods. Preserve existing bucketLabel formatting
for populated points and add a regression test covering missing daily or monthly
buckets.
In
`@plugins/openchoreo-observability/src/components/DeliveryInsights/useDoraBreakdown.ts`:
- Around line 154-176: Limit concurrent DORA metric requests in the fetchSummary
flow instead of starting every children and envChildren request at once. Add or
reuse a bounded-concurrency mechanism around the two Promise.all mappings,
preserving each row’s summary on success and returning the original child on
failure; ensure the combined project and environment fan-out cannot exceed the
configured limit.
- Around line 68-78: Update the environment query in useDoraBreakdown to filter
by metadata.annotations[CHOREO_ANNOTATIONS.NAMESPACE] using scope.namespace,
matching the System and Component queries. Remove the metadata.namespace filter
while preserving the Environment kind and environment name mapping.
In
`@plugins/openchoreo-observability/src/components/DeliveryInsights/useDoraInsights.ts`:
- Around line 27-30: Update useDoraInsights so each DORA response is associated
with the current scope, range, and granularity query key, and only expose data
when those keys match the active query. Clear or suppress prior data for a new
query so failed requests cannot return an older response. Preserve the existing
behavior of setting loading to false for stale scopes while ensuring new scopes
only return responses belonging to the current query.
---
Nitpick comments:
In `@packages/openchoreo-client-node/src/observability-url-resolver.test.ts`:
- Around line 46-84: Update the isolation test around resolveForNamespace so
user B returns the same “dev” environment while its plane lookup fails, rather
than returning an empty list. Configure getB to provide the environment listing
and then return a failed response for the subsequent plane lookup, and assert
the failure reflects user B’s lookup instead of user A’s cached observer URL.
Keep the existing user A setup and token-routed client creation, ensuring the
env:${namespace}/${envName} cache path is exercised.
In
`@plugins/openchoreo-observability/src/components/DeliveryInsights/DeliveryInsightsContent.tsx`:
- Around line 100-104: Update useDoraBreakdown to accept a loadEnvironmentRows
option and guard the environment summary requests with it. In
DeliveryInsightsContent, pass false whenever envFilter is active so
domain/system filtered views do not fetch hidden breakdown.envRows cards, while
preserving environment-row loading when no filter is applied.
In
`@plugins/openchoreo-observability/src/components/DeliveryInsights/DeliveryInsightsPage.tsx`:
- Around line 152-161: Update the onDrill callback to guard against
component-level scope before applying namespace/project drill-down changes; at
that level, route the selected environment through onEnvFilterChange instead of
assigning it to component, while preserving the existing project and namespace
behavior.
- Around line 95-102: Update the useCallback named update to use
setSearchParams’s functional updater: receive the latest URLSearchParams, clone
them, apply mutator, and return the updated params. Remove the render-time
searchParams dependency if it is no longer referenced, while preserving replace:
true.
In
`@plugins/openchoreo-observability/src/components/DeliveryInsights/DoraBreakdownTable.test.tsx`:
- Around line 95-101: Strengthen the no-handler test around renderTable by
asserting the clicked row does not expose cursor: pointer, rather than
rechecking its text. Also add coverage for overallRating’s worst-tier selection,
verifying the component chooses the lowest applicable rating when multiple tiers
are present.
- Around line 6-49: Update the summary fixture helper to type its classification
parameter and returned object against DoraMetricsResponse['summary'], using the
existing classification type rather than string. Ensure summary(30, 'Elite') and
summary(10, 'High') satisfy that type, then remove the as any casts from
projectRow.summary and envRow.summary.
In
`@plugins/openchoreo-observability/src/components/DeliveryInsights/DoraBreakdownTable.tsx`:
- Around line 221-230: Update the delta Typography rendering in
DoraBreakdownTable to add a title clarifying that the percentage represents
deployment frequency, not the DORA rating. Replace the hardcoded positive and
negative hex colors with theme-derived success and error palette values,
defining any needed color in useStyles.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f7556576-8d60-4388-b571-f836a6b56da7
📒 Files selected for processing (28)
.changeset/delivery-insights-dora-ui.mdpackages/openchoreo-client-node/src/observability-url-resolver.test.tspackages/openchoreo-client-node/src/observability-url-resolver.tspackages/portal-app/src/components/Root/Root.tsxpackages/portal-app/src/createPortalApp.tsxplugins/openchoreo-observability-backend/src/router.test.tsplugins/openchoreo-observability-backend/src/router.tsplugins/openchoreo-observability-backend/src/services/ObservabilityService.tsplugins/openchoreo-observability/src/api/ObservabilityApi.tsplugins/openchoreo-observability/src/api/ObserverUrlCache.tsplugins/openchoreo-observability/src/components/CostInsights/CostInsightsBreadcrumb.tsxplugins/openchoreo-observability/src/components/DeliveryInsights/DeliveryInsightsContent.tsxplugins/openchoreo-observability/src/components/DeliveryInsights/DeliveryInsightsPage.test.tsxplugins/openchoreo-observability/src/components/DeliveryInsights/DeliveryInsightsPage.tsxplugins/openchoreo-observability/src/components/DeliveryInsights/DoraBreakdownTable.test.tsxplugins/openchoreo-observability/src/components/DeliveryInsights/DoraBreakdownTable.tsxplugins/openchoreo-observability/src/components/DeliveryInsights/DoraEnvironmentCards.tsxplugins/openchoreo-observability/src/components/DeliveryInsights/DoraMetricTile.tsxplugins/openchoreo-observability/src/components/DeliveryInsights/DoraTrendChart.tsxplugins/openchoreo-observability/src/components/DeliveryInsights/index.tsplugins/openchoreo-observability/src/components/DeliveryInsights/useDoraBreakdown.tsplugins/openchoreo-observability/src/components/DeliveryInsights/useDoraInsights.tsplugins/openchoreo-observability/src/components/DeliveryInsights/utils.tsplugins/openchoreo-observability/src/components/ScopeBreadcrumb/ScopeBreadcrumb.test.tsxplugins/openchoreo-observability/src/components/ScopeBreadcrumb/ScopeBreadcrumb.tsxplugins/openchoreo-observability/src/components/ScopeBreadcrumb/index.tsplugins/openchoreo-observability/src/index.tsplugins/openchoreo-observability/src/types.ts
Comment on lines
+55
to
+57
| if (!environmentName) { | ||
| return this.resolver.resolveForNamespace(namespaceName, userToken); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline packages/openchoreo-client-node/src/observability-url-resolver.ts --items all
rg -n --type ts -C 8 \
'\bresolveForNamespace\b|\bresolveForEnvironment\b|\bobserverUrl\b' \
packages/openchoreo-client-node/src/observability-url-resolver.ts \
packages/openchoreo-client-node/src/observability-url-resolver.test.ts
rg -n --type ts -C 6 \
'\bgetDoraMetrics\b|\bresolveUrls\b|\bobserverUrl\b' \
plugins/openchoreo-observability/src \
plugins/openchoreo-observability-backend/srcRepository: openchoreo/backstage-plugins
Length of output: 50385
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## Relevant source slices"
sed -n '40,75p' plugins/openchoreo-observability-backend/src/services/ObservabilityService.ts
sed -n '148,215p' packages/openchoreo-client-node/src/observability-url-resolver.ts
sed -n '240,365p' packages/openchoreo-client-node/src/observability-url-resolver.ts
echo
echo "## Behavioral probe of resolver loop semantics"
node - <<'JS'
async function fakeResolveForEnvironment(namespaces) {
return (input) => new Promise((resolve) => {
const ns = input.url.match(/namespaces\/([^/]+)\b/)?.[1];
const env = input.url.match(/\benvironments\/([^?]+)/)?.[1];
const planes = namespaces[ns]?.planes?.[env] ?? {};
resolve({
ok: true,
status: 200,
response: {},
data: planes
});
});
}
async function resolveForNamespace(namespaces, envOrder) {
let lastError;
for (const envName of envOrder) {
try {
const result = await fakeResolveForEnvironment(namespaces)({ url: `/namespaces/n1/environments/${envName}` });
if (result.data.observabilityPlaneRef) {
const op = await fakeResolveForEnvironment(namespaces)({ url: `/namespaces/n1/observabilityPlanes/${result.data.observabilityPlaneRef.name}` });
if (op.data?.spec?.observerURL) {
return { observerUrl: op.data.spec.observerURL };
}
}
} catch (error) {
lastError = new Error(String(error));
}
}
throw lastError || new Error('No observer found');
}
(async () => {
const cases = [
{
name: 'environments share one observer',
namespaces: { n1: { planes: { dev: { observabilityPlaneRef: { name: 'obs-common' } }, prod: { observabilityPlaneRef: { name: 'obs-common' } } } } },
},
{
name: 'environments point to different observers',
namespaces: { n1: { planes: { dev: { observabilityPlaneRef: { name: 'obs-dev' } }, prod: { observabilityPlaneRef: { name: 'obs-prod' } } } } },
},
];
for (const c of cases) {
let first, second;
try {
first = await resolveForNamespace(c.namespaces, ['dev', 'prod']);
try { second = await resolveForNamespace(c.namespaces, ['prod', 'dev']); } catch (e) {}
} catch (e) {
first = e.message;
}
console.log(c.name, 'first=', first.observerUrl || first, 'second=', second.observerUrl || second);
}
})();
JSRepository: openchoreo/backstage-plugins
Length of output: 7507
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## Behavioral probe of resolver loop semantics"
node - <<'JS'
function flattenEnvOrder(envs) {
// Mirrors the resolver code: first pick an environment, then pick an observer plane.
const order = [];
for (const environment of envs) {
for (const observerEnv of environment) {
order.push({ environment, observerEnv });
}
}
return order;
}
async function simulateResolve(namespaces, envOrder) {
for (const { environment, observerEnv } of envOrder) {
const observerUrl = namespaces[environment]?.observerURLForEnvironment?.[observerEnv];
if (observerUrl) {
return { observerUrl };
}
}
throw new Error('No observer found');
}
(async () => {
const namespaces = {
n1: {
observerURLForEnvironment: {
dev: 'https://observer-dev.example.com',
prod: 'https://observer-prod.example.com',
},
},
};
for (const inputOrder of [
[['dev'], ['prod']],
[['prod'], ['dev']],
]) {
const order = flattenEnvOrder(inputOrder);
console.log('input order:', JSON.stringify(inputOrder));
for (const e of order) {
console.log('attempt', e.environment, '<-', e.observerEnv, '=>', namespaces.n1.observerURLForEnvironment[e.observerEnv]);
}
try {
const first = await simulateResolve(namespaces, order);
console.log('resolved observer=', first.observerUrl);
console.log('metrics from another environment appear missing when request targets', order[order.length - 1].environment);
} catch (error) {
console.log('resolved observer=', error.message);
}
}
})();
JS
echo
echo "## Additional search for multi-entry behavior / tests"
rg -n --type ts -C 4 "environments?:|environment?:|observerURLForEnvironment|ObservabilityPlaneRef|observabilityPlaneRef" packages/openchoreo-client-node/src/observability-url-resolver.ts packages/openchoreo-client-node/src/observability-url-resolver.test.tsRepository: openchoreo/backstage-plugins
Length of output: 11199
Enforce one observer URL per namespace, or aggregate cross-environment calls.
resolveForNamespace returns the first environment’s observer URL it can resolve. If environments in the same namespace can point to different observers, namespace-scoped requests may call an observer that does not have the requested environment’s metrics. Add invariant validation and a test for this case, or aggregate calls across all namespace observers.
🤖 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
`@plugins/openchoreo-observability-backend/src/services/ObservabilityService.ts`
around lines 55 - 57, Update the namespace-scoped branch in ObservabilityService
to avoid returning only the first observer URL when environments in the
namespace may use different observers: either validate and enforce a single
observer URL per namespace, or aggregate requests across every namespace
observer. Add coverage for multiple environments resolving to different
observers and preserve the existing environment-specific behavior.
Comment on lines
+68
to
+78
| const { items: envEntities } = await catalogApi.getEntities({ | ||
| filter: { | ||
| kind: 'Environment', | ||
| 'metadata.namespace': scope.namespace, | ||
| }, | ||
| fields: ['metadata.name'], | ||
| }); | ||
| const envNames = envEntities.map(e => e.metadata.name); | ||
| if (!cancelled) { | ||
| setEnvironments(envNames); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find where Environment entities are created and how namespace is recorded.
rg -nP -C6 "kind:\s*['\"]?Environment" --type=ts -g '!**/*.test.*'
# Inspect the annotation constants.
rg -nP -C2 'NAMESPACE\s*[:=]' --type=ts -g '**/*annotation*'
# Compare other Environment catalog queries in the repo.
rg -nP -C6 "getEntities\(" --type=ts | rg -n -C6 'Environment'Repository: openchoreo/backstage-plugins
Length of output: 166
🏁 Script executed:
#!/bin/bash
set -u
echo "== repo files matching useDoraBreakdown.ts =="
fd -a 'useDoraBreakdown\.ts$' . || true
file="$(fd 'useDoraBreakdown\.ts$' . | head -n 1 || true)"
if [ -n "${file:-}" ]; then
echo "== file: $file =="
wc -l "$file"
sed -n '1,140p' "$file"
fi
echo "== search Environment kind strings =="
rg -nP -C6 "Environment['\"]|kind: ['\"]?Environment|['\"].*Environment" -t ts -g '!**/*.test.*' . || true
echo "== search annotation NAMESPACE constants =="
rg -nP -C4 "NAMESPACE|CHOREO_ANNOTATIONS|CHOREO_NAMESPACE|namespace" -t ts -g '**/*annotation*' . || true
echo "== catalog queries with Environment =="
rg -nP -C4 "getEntities|ApiRef|CatalogApi" -t ts . || true
echo "== search openchoreo namespace annotation =="
rg -nP -C4 "choreo|OpenChoreo|NAMESPACE|openchoreo" -t ts . || trueRepository: openchoreo/backstage-plugins
Length of output: 50385
🏁 Script executed:
#!/bin/bash
set -u
echo "== candidate files =="
git ls-files | rg 'useDoraBreakdown\.ts|environment|Environment|annotations|catalog|ingest|CHOREO|openchoreo\.io' | head -n 200 || true
echo "== exact useDoraBreakdown file location =="
use_dora="$(git ls-files | rg '(^|/)useDoraBreakdown\.ts$' | head -n 1 || true)"
if [ -f "${use_dora:-}" ]; then
echo "FILE=$use_dora"
wc -l "$use_dora"
sed -n '1,180p' "$use_dora"
fi
echo "== Environment entity kind exact =="
rg -n "kind:\s*['\"]Environment|['\"]Environment['\"]" -g '*.ts' -g '*.tsx' -g '*.yaml' -g '*.yml' . | head -n 200 || true
echo "== annotation NAMESPACE exact =="
rg -n "CHOREO_ANNOTATIONS|ANNOTATIONS|NAMESPACE|openchoreo\.io" -g '*.ts' -g '*.tsx' . | head -n 300 || true
echo "== targeted getEntities calls =="
rg -n "getEntities\(" -g '*.ts' -g '*.tsx' . | head -n 300 || trueRepository: openchoreo/backstage-plugins
Length of output: 50384
🏁 Script executed:
#!/bin/bash
set -u
echo "== env processors =="
for f in plugins/catalog-backend-module-openchoreo/src/processors/EnvironmentEntityProcessor.ts plugins/catalog-backend-module-openchoreo/src/utils/entityTranslation.ts plugins/catalog-backend-module-openchoreo/src/kinds/EnvironmentEntityV1alpha1.ts; do
echo "--- $f ---"
if [ -f "$f" ]; then
wc -l "$f"
sed -n '1,220p' "$f"
fi
done
echo "== annotation constants =="
for f in plugins/openchoreo-backstage-plugin-common/src/annotations.ts plugins/openchoreo-react/src/utils/platformOverviewConstants.ts; do
echo "--- $f ---"
if [ -f "$f" ]; then
wc -l "$f"
sed -n '1,120p' "$f"
fi
done
echo "== targeted entityTranslation lines 380-480 =="
sed -n '380,485p' plugins/catalog-backend-module-openchoreo/src/utils/entityTranslation.tsRepository: openchoreo/backstage-plugins
Length of output: 20974
🏁 Script executed:
#!/bin/bash
set -u
echo "== common annotation constants file =="
fd -a 'annotations\.ts$' . | while read -r f; do
echo "--- $f ---"
grep -n -C3 "CHOREO_ANNOTATIONS\|NAMESPACE\|ENVIRONMENT" "$f" || true
done
echo "== exact annotation key occurrences =="
rg -n "openchoreo\.io/namespace|CHOREO_ANNOTATIONS\.NAMESPACE|ENVIRONMENT" plugins/openchoreo-backstage-plugin-common/src packages/openchoreo-client-node/src/generated/openchoreo/types.ts | head -n 200 || true
echo "== translation namespace call sites =="
rg -n -C4 "translateEnvironmentToEntity|translate.*Environment|createEnvironment" plugins/catalog-backend-module-openchoreo/src plugins/openchoreo-backend/src plugins/scaffolder-backend-module-openchoreo/src | head -n 240 || trueRepository: openchoreo/backstage-plugins
Length of output: 23154
Filter environments by the OpenChoreo namespace annotation.
translateEnvironmentToEntity stores the OpenChoreo namespace in metadata.annotations[CHOREO_ANNOTATIONS.NAMESPACE], so the environment catalog query must use that annotation like the System and Component queries do. Filtering by metadata.namespace makes non-default namespaces return no envNames, leaving the environment filter and per-environment DORA cards empty.
🤖 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
`@plugins/openchoreo-observability/src/components/DeliveryInsights/useDoraBreakdown.ts`
around lines 68 - 78, Update the environment query in useDoraBreakdown to filter
by metadata.annotations[CHOREO_ANNOTATIONS.NAMESPACE] using scope.namespace,
matching the System and Component queries. Remove the metadata.namespace filter
while preserving the Environment kind and environment name mapping.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
- Refresh now reloads the breakdown as well as the headline metrics. useDoraBreakdown issues its own requests and had no reload path, so the table and environment cards could show an older snapshot than the tiles and charts. - useDoraInsights stores the query key alongside its response and only returns data whose key still matches, so a failed or in-flight request for a newly selected scope can no longer leave the previous scope's numbers on screen. A failed refresh of the *same* scope still keeps its last good data, which is intended. - Change Failure Rate tile now shows an em dash when the window has no deployments, matching DoraEnvironmentCards and DoraBreakdownTable instead of reporting a misleading 0.0%. - Lead time and MTTR trend lines no longer bridge across buckets that had no measurement. Both series omit empty buckets, so they are aligned to the zero-filled deployment-frequency buckets with nulls for the gaps (recharts renders those as gaps). - Breakdown metric requests are capped at 6 in flight instead of firing one per project plus one per environment in the same tick. - Documented that resolveForNamespace assumes one observability plane per namespace, and corrected its cache comment: the namespace entry is token-partitioned but the pre-existing per-environment entry it delegates to is not. Signed-off-by: LakshanSS <lakshan230897@gmail.com>
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
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
`@plugins/openchoreo-observability/src/components/DeliveryInsights/useDoraInsights.ts`:
- Around line 94-97: Update the error state in useDoraInsights so each error is
stored with the queryKey that produced it, and expose it only when that key
matches the active queryKey; preserve current-data behavior for successful
results and null scopes. Add a regression test covering a failed query followed
by a scope change, including changing to a null scope, and verify the stale
error is not rendered by DeliveryInsightsContent.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 394d0f4a-332b-4442-aecb-0113b4a24d59
📒 Files selected for processing (8)
packages/openchoreo-client-node/src/observability-url-resolver.tsplugins/openchoreo-observability/src/components/DeliveryInsights/DeliveryInsightsContent.tsxplugins/openchoreo-observability/src/components/DeliveryInsights/DoraTrendChart.tsxplugins/openchoreo-observability/src/components/DeliveryInsights/useDoraBreakdown.tsplugins/openchoreo-observability/src/components/DeliveryInsights/useDoraInsights.test.tsplugins/openchoreo-observability/src/components/DeliveryInsights/useDoraInsights.tsplugins/openchoreo-observability/src/components/DeliveryInsights/utils.test.tsplugins/openchoreo-observability/src/components/DeliveryInsights/utils.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- plugins/openchoreo-observability/src/components/DeliveryInsights/DoraTrendChart.tsx
- packages/openchoreo-client-node/src/observability-url-resolver.ts
- plugins/openchoreo-observability/src/components/DeliveryInsights/DeliveryInsightsContent.tsx
- plugins/openchoreo-observability/src/components/DeliveryInsights/useDoraBreakdown.ts
Comment on lines
+94
to
+97
| // A stale result (a scope/window that has since changed) is withheld rather | ||
| // than shown under the current selection. A failed *refresh* of the current | ||
| // key keeps its last good data alongside the error, which is intended. | ||
| const data = result?.key === queryKey ? result.data : null; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Scope error to the active query.
Line 97 suppresses stale data, but error has no queryKey. After a failed request, changing to a null scope leaves the old error exposed. Changing to another scope can also render the prior error before the effect clears it. Store the error with its query key and return it only when the key matches. Add a regression test for a failed query followed by a scope change.
Proposed change
- const [error, setError] = useState<string | null>(null);
+ const [errorResult, setErrorResult] = useState<{
+ key: string;
+ message: string;
+ } | null>(null);
- setError(null);
+ setErrorResult(null);
- setError(
- err instanceof Error ? err.message : 'Failed to fetch DORA metrics',
- );
+ setErrorResult({
+ key: queryKey,
+ message:
+ err instanceof Error
+ ? err.message
+ : 'Failed to fetch DORA metrics',
+ });
const data = result?.key === queryKey ? result.data : null;
+ const error =
+ errorResult?.key === queryKey ? errorResult.message : null;plugins/openchoreo-observability/src/components/DeliveryInsights/DeliveryInsightsContent.tsx renders the hook error directly.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // A stale result (a scope/window that has since changed) is withheld rather | |
| // than shown under the current selection. A failed *refresh* of the current | |
| // key keeps its last good data alongside the error, which is intended. | |
| const data = result?.key === queryKey ? result.data : null; | |
| // A stale result (a scope/window that has since changed) is withheld rather | |
| // than shown under the current selection. A failed *refresh* of the current | |
| // key keeps its last good data alongside the error, which is intended. | |
| const data = result?.key === queryKey ? result.data : null; | |
| const error = | |
| errorResult?.key === queryKey ? errorResult.message : null; |
🤖 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
`@plugins/openchoreo-observability/src/components/DeliveryInsights/useDoraInsights.ts`
around lines 94 - 97, Update the error state in useDoraInsights so each error is
stored with the queryKey that produced it, and expose it only when that key
matches the active queryKey; preserve current-data behavior for successful
results and null scopes. Add a regression test covering a failed query followed
by a scope change, including changing to a null scope, and verify the stale
error is not rendered by DeliveryInsightsContent.
useDoraBreakdown hardcoded weekly, so the breakdown table and environment cards described a different window shape than the headline tiles above them. That was visible as a headline of 263 deployments over a breakdown summing to 313, because summary totals used to depend on the requested granularity (fixed in the observer alongside this). Only `summary` is read here, and summaries now cover the exact requested window regardless of granularity, so this no longer changes any number — but every request on the page now describes the same view, and the magic 'weekly' no longer invites the question. Signed-off-by: LakshanSS <lakshan230897@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a Delivery Insights page showing the four DORA metrics, reached from the
sidebar and scoped by a Namespace → Project → Component breadcrumb.
This supersedes #724, which put the same metrics behind an Insights tab on the
namespace/project/component entity pages. Delivery performance is read by
engineering leadership looking across an organisation, while entity pages are a
developer's view of a single component — so this follows the placement
Cost Insights (#723) established. Entity pages are left byte-identical to
mainby this PR.#724 stays open as a draft for now so the two placements can be compared; only
one of them should merge.
What's in it
MTTR, each with its DORA classification, delta vs the previous equal-length
window, and a sparkline.
time as p50/p75/p95).
components, component → environments), sorted by deployment frequency. Each row
carries its own metrics plus an overall rating (the scope's weakest tier).
Project/component rows drill the page scope down; environment rows apply the
environment filter.
"how these metrics are calculated" footnote.
the URL, so a given view can be shared or saved.
extracted as
ScopeBreadcrumband now backs both Delivery and Cost Insights(
CostInsightsBreadcrumbbecomes a thin wrapper, ~290 lines de-duplicated).Data layer
ObservabilityClientgainsgetDoraMetrics/getDoraDeploymentsagainst theobserver's
POST /api/v1alpha1/insights/dora/queryand.../insights/dora/deployments/query, called directly like the otherobservability APIs.
Observer URL resolution gains namespace-level support:
/resolve-urlsnow workswithout an
environmentNameby resolving through the namespace's environments(new
resolveForNamespacein the client-node resolver) — this is what theorg-wide scope needs.
Testing
yarn tsc— cleanyarn prettier --check .— cleanno-mixed-plugin-importstest-utils warnings)
observability 94/839, portal-app 23/200, client-node 4/65,
observability-backend 3/18. New suites cover the page's URL↔state wiring
(scope/level derivation, filter persistence, drill-down, clearing the env
filter across a namespace switch), the shared breadcrumb, and the breakdown
table's drill-vs-filter behaviour.
Depends on
The observer-side endpoints land in openchoreo/openchoreo#4248, which is held
open until the full Delivery Insights feature is complete — so this is a
draft until that merges.
Open question for reviewers
Delivery Insights and Cost Insights are now two sidebar items. A single
Insights entry with
Delivery/Costtabs inside may be the betterlong-term information architecture. Kept as separate items here to avoid
reworking #723 so soon after it landed — happy to unify in a follow-up if
that's preferred.
Summary by CodeRabbit