RHIDP-14173: Implement scalar aggregation KPI types#3571
Conversation
|
This pull request adds a new top-level directory under |
|
Important This PR includes changes that affect public-facing API. Please ensure you are adding/updating documentation for new features or behavior. Changed Packages
|
|
🤖 Finished Review · ✅ Success · Started 5:02 PM UTC · Completed 5:19 PM UTC |
ReviewFindingsLow
Previous runReviewFindingsMedium
Low
Previous run (2)ReviewFindingsMedium
Low
Labels: PR implements new scalar aggregation KPI types as a feature enhancement to the scorecard workspace. |
fullsend-ai-review
Bot
added
requires-manual-review
imykhno
force-pushed
the
feat/scorecard-scalar-aggregation-types
branch
from
22368af to
5a7cfb0
Compare
|
🤖 Review · ❌ Terminated · Started 5:31 PM UTC · Ended 5:44 PM UTC |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3571 +/- ##
==========================================
+ Coverage 54.19% 54.26% +0.06%
==========================================
Files 2303 2309 +6
Lines 88305 88434 +129
Branches 24590 24630 +40
==========================================
+ Hits 47858 47989 +131
+ Misses 40238 40236 -2
Partials 209 209
*This pull request uses carry forward flags. Click here to find out more. Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
fullsend-ai-review
Bot
added
requires-manual-review
|
🤖 Finished Review · ✅ Success · Started 5:31 PM UTC · Completed 5:44 PM UTC |
…dStatusScore` Signed-off-by: Ihor Mykhno <imykhno@redhat.com>
… safety and improved structure Signed-off-by: Ihor Mykhno <imykhno@redhat.com>
…dKpi` across tests and configurations Signed-off-by: Ihor Mykhno <imykhno@redhat.com>
Signed-off-by: Ihor Mykhno <imykhno@redhat.com>
Signed-off-by: Ihor Mykhno <imykhno@redhat.com>
imykhno
force-pushed
the
feat/scorecard-scalar-aggregation-types
branch
from
5a7cfb0 to
53fdb59
Compare
|
|
🤖 Finished Review · ✅ Success · Started 1:04 PM UTC · Completed 1:17 PM UTC |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
fullsend-ai-review
Bot
added
ready-for-merge
| }; | ||
| if (aggregationConfig.type === aggregationTypes.weightedStatusScore) { | ||
| aggregationConfig.options = buildWeightedStatusScoreOptions(config); | ||
| } else if (isScalarAggregationType(aggregationConfig.type)) { |
There was a problem hiding this comment.
With average changing meaning from weighted-status-score to scalar mean, an un-migrated config that still has type: average + options.statusScores won't fail startup — this branch only copies thresholds, so the leftover statusScores is silently dropped before the strict Zod schema can reject it, and the KPI quietly starts returning a scalar mean with a different result shape. Should we fail startup when a scalar-type KPI still carries options.statusScores, with a "did you mean weightedStatusScore?" hint? Passing the raw options through to the schema would also catch it via strictObject.
|
|
||
| - **Entity scorecard tab** — View scorecard metrics on catalog entity pages (components, websites, etc.). | ||
| - **Scorecard homepage card** — Show aggregated KPIs on the home page (e.g. GitHub open PRs, Jira open issues). Supports **`statusGrouped`** (multi-slice pie) and **`average`** (weighted health donut) KPI types configured under **`scorecard.aggregationKPIs`**. | ||
| - **Scorecard homepage card** — Show aggregated KPIs on the home page (e.g. GitHub open PRs, Jira open issues). The built-in card renders **`statusGrouped`** (multi-slice pie), **`weightedStatusScore`** (weighted health donut), and scalar types (`sum`, `average`, `max`, `min`, `count`). |
There was a problem hiding this comment.
This says the built-in card renders the scalar types, but AggregatedMetricCard.tsx only handles statusGrouped and weightedStatusScore — sum/average/max/min/count all fall through to UnsupportedAggregationType. If the scalar card UI is a follow-up, maybe say scalar KPIs are API-only for now, so users configuring a sum KPI don't file bugs about the error panel on their homepage.
| z.object({ | ||
| key: z.string(), | ||
| expression: z.string(), | ||
| color: z.string(), |
There was a problem hiding this comment.
color is required here, but thresholds.md section 5 says the rules take an optional color, and AggregationThresholdRule in scorecard-common keeps it optional too (Pick<ThresholdRule, ...> where color?). So a scalar KPI rule without a color fails at startup even though config.d.ts and the docs both allow it. Should this be z.string().optional(), or should the docs/type change?
| } from '@red-hat-developer-hub/backstage-plugin-scorecard-common'; | ||
|
|
||
| function parseAggregationConfig(config: unknown): AggregationConfig { | ||
| function isScalarAggregationType(type: string): boolean { |
There was a problem hiding this comment.
Isn't this the same helper as src/utils/isScalarAggregationType.ts added in this PR? isScalarAggregationRuntimeConfig repeats the .includes pattern a third time. Could we import the one util everywhere — and make it a real type guard (type is (typeof scalarAggregationTypes)[number]) while at it?
| ) | ||
| .first(); | ||
|
|
||
| const value = aggregateRow?.value ? Number(aggregateRow.value) : 0; |
There was a problem hiding this comment.
The truthiness check conflates "NULL aggregate" with "aggregate equals 0" — it works today because a numeric 0 falls through to the same 0, but aggregateRow?.value != null ? Number(aggregateRow.value) : 0 says what's meant and won't break if the fallback ever changes. Same for total on the next line.
| | **`min`** | Minimum latest metric value across owned entities | “Best-case / lowest value in the portfolio.” | | ||
| | **`count`** | Number of entities with a non-null latest stored value | “How many entities have data for this metric.” | | ||
|
|
||
| **Scalar types** (`sum`, `average`, `max`, `min`, `count`) aggregate the **numeric `value`** from each owned entity’s **latest** stored `metric_values` row for the configured **`metricId`**, instead of bucketing by threshold status. Clients can detect scalar responses by checking **`metadata.aggregationType`** against the scalar type literals (or `scalarAggregationTypes` from scorecard-common).\*\*\*\* |
There was a problem hiding this comment.
nit: this paragraph and the one at line 39 both open with "Scalar types (sum, ...)" and partially repeat each other — worth merging into one. There's also a stray escaped \*\*\*\* at the end of this line.
| 3. **Denominator and percentage:** Let **`maxWeight`** be the maximum of **`options.statusScores[rule.key]`** over each **`rule.key`** in the metric’s **merged threshold rules** (missing map entries are treated as **0** here). **`weightedStatusMaxPossible`** = **`maxWeight × total entities`**. If **`total`** is 0 or **`weightedStatusMaxPossible`** is 0, **`weightedStatusScore`** is **0**; otherwise **`weightedStatusScore`** = **`100 × (weighted sum / weightedStatusMaxPossible)`** rounded to **one decimal place** (the API and UI use this **0–100** headline directly). The value can exceed **100** if **`statusScores`** assigns a weight above **`maxWeight`** to a status that still appears in the aggregated counts; keep **`statusScores`** aligned with your metric rules to avoid that. | ||
|
|
||
| **`options.thresholds`:** **number**-style rules (same shape as metric thresholds) evaluated against **`averageScore`** on the **0–100** scale (higher = better for typical setups). The first matching rule supplies **`result.aggregationChartDisplayColor`**. If omitted from **`scorecard.aggregationKPIs`**, it stays unset in the built KPI config; **`AverageAggregationStrategy`** then applies **`DEFAULT_AVERAGE_KPI_RESULT_THRESHOLDS`** from [`src/constants/aggregationKPIs.ts`](../src/constants/aggregationKPIs.ts) when each aggregation runs and logs **info** that the default 0–100% health scale is used: **<30%** = error, **30–79%** = warning, **≥80%** = success. Full detail: [thresholds.md — Aggregation KPI result thresholds](./thresholds.md#4-aggregation-kpi-result-thresholds-average-type) and [backend README — Aggregation KPIs](../README.md#aggregation-kpis-homepage-and-get-aggregations). | ||
| **`options.thresholds`:** **number**-style rules (same shape as metric thresholds) evaluated against **`weightedStatusScore`** on the **0–100** scale (higher = better for typical setups). The first matching rule supplies **`result.aggregationChartDisplayColor`**. If omitted from **`scorecard.aggregationKPIs`**, it stays unset in the built KPI config; **`WeightedStatusScoreAggregationStrategy`** then applies **`DEFAULT_WEIGHTED_STATUS_SCORE_KPI_RESULT_THRESHOLDS`** from [`src/constants/aggregationKPIs.ts`](../src/constants/aggregationKPIs.ts) when each aggregation runs and logs **info** that the default 0–100% health scale is used: **<30%** = error, **30–79%** = warning, **≥80%** = success. Full detail: [thresholds.md — Aggregation KPI result thresholds](./thresholds.md#4-aggregation-kpi-result-thresholds-weighted-status-score-type) and [backend README — Aggregation KPIs](../README.md#aggregation-kpis-homepage-and-get-aggregations). |
There was a problem hiding this comment.
nit: this anchor doesn't match the heading — "### 4. Aggregation KPI result thresholds (weightedStatusScore type)" slugs to #4-aggregation-kpi-result-thresholds-weightedstatusscore-type (the backend README uses the correct form).
| | `description` | Display description for this aggregation. | | ||
| | `type` | Aggregation algorithm: `statusGrouped` (counts per threshold status), `weightedStatusScore` (normalized weighted score), or scalar types `sum`, `average`, `max`, `min`, `count` (numeric rollup over latest metric values). | | ||
| | `metricId` | Metric provider id used to load thresholds and compute counts or values. | | ||
| | `options` | Type-specific options. For **`weightedStatusScore`**: **required** **`options.statusScores`** (status key → weight); optional **`options.thresholds`** (see [thresholds.md — Aggregation KPI result thresholds (weightedStatusScore)](./docs/thresholds.md#4-aggregation-kpi-result-thresholds-weightedstatusscore-type)). For **scalar types**: optional **`options.thresholds`** (see [thresholds.md — Aggregation KPI result thresholds (scalar types)](./docs/thresholds.md#5-aggregation-kpi-result-thresholds-scalar-types)). All scalar types require a **number** metric. Scalar responses include **`value`**, **`total`**, **`entitiesConsidered`**, **`calculationErrorCount`**, **`timestamp`**, and **`thresholds`** — see [aggregation.md — Scalar result fields](./docs/aggregation.md#scalar-result-fields). | |
There was a problem hiding this comment.
nit: aggregation.md#scalar-result-fields points to a heading that doesn't exist — there's no "Scalar result fields" section in aggregation.md (thresholds.md has the same link in section 5). Either add that section or point at the scalar response-shape bullets.
|
|
||
| describe('readScalarAggregatedMetricByEntityRefs', () => { | ||
| describe.each(databases.eachSupportedId())( | ||
| 'should %p raw metric values across latest rows', |
There was a problem hiding this comment.
nit: this describe.each title renders as "should POSTGRES_13 raw metric values across latest rows", which reads oddly next to the per-it titles that already describe the operation. Also, the avg assertion below (value: 5.666666666666667) relies on exact float equality across pg and SQLite — toBeCloseTo would be safer.
| */ | ||
|
|
||
| import { scalarAggregationTypes } from '@red-hat-developer-hub/backstage-plugin-scorecard-common'; | ||
| import { AggregationType } from '@red-hat-developer-hub/backstage-plugin-scorecard-common'; |
There was a problem hiding this comment.
nit: two separate imports from the same package — can be merged into one.
2 participants
Hey, I just made a Pull Request!
Implemented logic to support the following aggregate metric types:
average,sum,max,min, andcountfor Scorecard aggregated KPI cards.This PR for:
✔️ Checklist
How to test
Preparing
app-config.local.yamladd new aggregation types underscorecard.aggregationKPIs(example added to theapp-config.yaml):Testing
Get aggregation result when
sumaggregation type was used (totalOpenBugs):Get aggregation result when
averageaggregation type was used (avgOpenPrs):Get aggregation result when
countaggregation type was used (entitiesWithOpenIssues):Get aggregation result when
maxaggregation type was used (maxOpenPrs):Get aggregation result when
minaggregation type was used (minOpenPrs):Get aggregation results when some entities lack values due to an error
The environment must contain a mix of entities: some with valid values for aggregation, and others without values to simulate an error state.
Use one of the proposed requests below to confirm that:
Get aggregation results when all entities lack values due to an error
The environment must contain a all of entities without values to simulate an error state.
Use one of the proposed requests below to confirm that:
When the threshold is incorrectly configured for new aggregation types
Configure a threshold with incorrect ranges for a new aggregation type. The application is expected to throw an error upon startup.
For example:
When the threshold is correctly configured for new aggregation types
A correctly configured threshold should be present in the response under
result.thresholds. If no threshold is configured underapp-config.yaml, the default threshold should be used.When a new aggregation type is configured for a boolean provider
When a metricId for a boolean metric provider is supplied, an error should be thrown when the application starts up.
For example: