RHIDP-12121: Support "StatusGrouped" and "Average" types of aggregation - #2923
Conversation
Signed-off-by: Ihor Mykhno <imykhno@redhat.com>
Code Review by Qodo
1.
|
|
Important This PR includes changes that affect public-facing API. Please ensure you are adding/updating documentation for new features or behavior. Changed Packages
|
Review Summary by QodoSupport "Sum" and "Average" types of aggregation with weighted scoring and strategy-based architecture
WalkthroughsDescription• Added support for average aggregation type alongside existing statusGrouped type, enabling weighted scoring of KPIs based on configurable status scores • Implemented strategy-based aggregation architecture with AggregationsService, AverageAggregationStrategy, and StatusGroupedAggregationStrategy for extensible aggregation handling • Created new UI components for average aggregation display: AverageCardComponent with donut chart, center percentage, and weighted score tooltips • Refactored threshold validation into separate validateThresholdsForMetric and validateThresholdsForAggregation functions with different requirements (icon required for metrics, optional for aggregations) • Added comprehensive test coverage for new aggregation strategies, service layer, validation, and e2e scenarios including average KPI rendering and error handling • Extended configuration schema with options field for average KPI containing statusScores map and optional aggregationResultThresholds • Added default threshold configuration for average KPI result thresholds (error <30%, warning 30-79%, success ≥80%) • Updated public API types to include AggregatedMetricAverageResult, StatusGroupedAggregationResult, and discriminated union AggregationResultByType • Added internationalization support with translations for average aggregation UI labels and tooltips in 5 languages (Japanese, German, Italian, French, Spanish) • Enhanced accessibility with aria-labels for loading progress indicators • Comprehensive documentation covering aggregation types, configuration, validation, thresholds, and drill-down behavior Diagramflowchart LR
Config["KPI Config<br/>statusScores<br/>thresholds"]
AggService["AggregationsService"]
Registry["Strategy Registry"]
AvgStrategy["AverageAggregationStrategy"]
StatusStrategy["StatusGroupedAggregationStrategy"]
Loader["AggregatedMetricLoader"]
Result["AggregatedMetricResult"]
UI["UI Components<br/>AverageCard<br/>StatusGroupedCard"]
Config -- "loads" --> AggService
AggService -- "delegates to" --> Registry
Registry -- "routes to" --> AvgStrategy
Registry -- "routes to" --> StatusStrategy
AvgStrategy -- "uses" --> Loader
StatusStrategy -- "uses" --> Loader
AvgStrategy -- "returns" --> Result
StatusStrategy -- "returns" --> Result
Result -- "renders" --> UI
File Changes1. workspaces/scorecard/plugins/scorecard-backend/src/service/router.test.ts
|
imykhno
changed the title
Signed-off-by: Ihor Mykhno <imykhno@redhat.com>
Signed-off-by: Ihor Mykhno <imykhno@redhat.com>
Signed-off-by: Ihor Mykhno <imykhno@redhat.com>
…nagement Signed-off-by: Ihor Mykhno <imykhno@redhat.com>
Eswaraiahsapram
left a comment
Eswaraiahsapram
left a comment
There was a problem hiding this comment.
Thanks @imykhno, tested locally, everything looks good to me 🎉 . Added a few nit comments, PTAL.
There was a problem hiding this comment.
Thank you for the PR!
I confirmed test cases:
- default / status grouped / deprecated work correctly
- average KPI:
Screen.Recording.2026-04-27.at.12.34.54.mov
We should take threshold configuration for aggregation cards from aggregationResultThresholds instead of thresholds in the metric, so the legend colors make sense.
- validation of kpis in config fails fast
- basic permissions work and conditional permissions work correctly
It would be nice if in config.d.ts if there are different keys for each aggregation configuration (especially when for that configuration they are mandatory), then we don't specify each as optional in the big aggregation config, but have something like:
type AggregationKPIConfig =
| AverageAggregationKPIConfig
| StatusGroupedAggregationKPIConfig;
In future users will use UI to define scorecards, for which they define title, description, metricId, thresholds, entity filter, what location.... It looks similar to what we currently have in yaml for aggregationKPIs. Maybe we should have gone with name scorecards.
Right now, thresholds are only supported for aggregation scorecards, we might in future need to support those also for other scorecards defined via yaml. We might in future save scorecard results (aggregation results) in database, right now it is alright to do in runtime.
…tion Signed-off-by: Ihor Mykhno <imykhno@redhat.com>
…lds` in configuration Signed-off-by: Ihor Mykhno <imykhno@redhat.com>
…vice files Signed-off-by: Ihor Mykhno <imykhno@redhat.com>
Signed-off-by: Ihor Mykhno <imykhno@redhat.com>
Signed-off-by: Ihor Mykhno <imykhno@redhat.com>
…ew types" This reverts commit b264e55.
christoph-jerolimov
left a comment
christoph-jerolimov
left a comment
There was a problem hiding this comment.
Hi @imykhno. Nice work.
I didn't tested it myself but there was already other people that has done that.
I just gone over the diff and here are some comments:
| AGGREGATED_CARDS_WIDGET_TITLES.withOpenPrsWeightedKpi, | ||
| ); | ||
| await homePage.saveChanges(); | ||
| await page.reload(); |
There was a problem hiding this comment.
Why is this page.reload needed? Can you please create a bug so that we follow-up. Users should see the right cards and latest data after they save the homepage.
There was a problem hiding this comment.
I will create a bug to follow and resolve all your comments related to e2e tests
| ); | ||
|
|
||
| await homePage.navigateToHome(); | ||
| await page.reload(); |
There was a problem hiding this comment.
Why is this needed?
| homePage, | ||
| AGGREGATED_CARDS_WIDGET_TITLES.withOpenPrsWeightedKpi, | ||
| ); | ||
| await page.reload(); |
There was a problem hiding this comment.
The other comments: why? Can we follow up ok this with a bug please?
| key: z.string(), | ||
| expression: z.string(), | ||
| color: z.string(), |
There was a problem hiding this comment.
We could probably have stricter types here in the future:
Key min max as the metric id?
Expression validation?
Color validation?
But not now.
| componentProps: { | ||
| Renderer: BorderlessHomeWidgetRenderer, | ||
| }, |
There was a problem hiding this comment.
Why does the card needs a rendered prop. How will this work with mount points or NFS? Can we get rid of this?
If not today please create a bug as well so that we can follow up.
There was a problem hiding this comment.
I will create a bug for this comment. Thank you
…average KPI support Signed-off-by: Ihor Mykhno <imykhno@redhat.com>
…oded labels Signed-off-by: Ihor Mykhno <imykhno@redhat.com>
Signed-off-by: Ihor Mykhno <imykhno@redhat.com>
…nfiguration Signed-off-by: Ihor Mykhno <imykhno@redhat.com>
Signed-off-by: Ihor Mykhno <imykhno@redhat.com>
Signed-off-by: Ihor Mykhno <imykhno@redhat.com>
There was a problem hiding this comment.
Thanks @imykhno, tested locally, looks good to me 🎉
expectation that Christoph's comments will be resolved via the follow-up bug.
Screen.Recording.2026-04-28.at.4.43.22.PM.mov
Missing Permission
Missing permission for github
Average
Tooltip
dzemanov
left a comment
dzemanov
left a comment
There was a problem hiding this comment.
Thank you.
I will create a follow up bug for:
We should take threshold configuration for aggregation cards from aggregationResultThresholds instead of thresholds in the metric, so the legend colors make sense.
…egationThresholdRule` Signed-off-by: Ihor Mykhno <imykhno@redhat.com>
|
…on (redhat-developer#2923) * feat(scorecard): add `average` aggregation type Signed-off-by: Ihor Mykhno <imykhno@redhat.com> * fix(scorecard): average gauge out-of-range Signed-off-by: Ihor Mykhno <imykhno@redhat.com> * fix(scorecard): default aggregation threshold value usage Signed-off-by: Ihor Mykhno <imykhno@redhat.com> * fix(scorecard): sonarqube issues Signed-off-by: Ihor Mykhno <imykhno@redhat.com> * refactor(scorecard): update type definitions for tooltip and state management Signed-off-by: Ihor Mykhno <imykhno@redhat.com> * fix(scorecard): update tooltip translations and aggregation configuration Signed-off-by: Ihor Mykhno <imykhno@redhat.com> * refactor(scorecard): rename `aggregationResultThresholds` to `thresholds` in configuration Signed-off-by: Ihor Mykhno <imykhno@redhat.com> * refactor(scorecard): consolidate AggregationConfig imports across service files Signed-off-by: Ihor Mykhno <imykhno@redhat.com> * fix(scorecard): issues after merging main Signed-off-by: Ihor Mykhno <imykhno@redhat.com> * feat(scorecard): enhance aggregation KPI configuration with new types Signed-off-by: Ihor Mykhno <imykhno@redhat.com> * Revert "feat(scorecard): enhance aggregation KPI configuration with new types" This reverts commit b264e55. * refactor(scorecard): update README and aggregation configuration for average KPI support Signed-off-by: Ihor Mykhno <imykhno@redhat.com> * feat(scorecard): use the i18n for loading indicators and remove hardcoded labels Signed-off-by: Ihor Mykhno <imykhno@redhat.com> * refactor(scorecard): rename `aggregationKinds` to `aggregationTypes` Signed-off-by: Ihor Mykhno <imykhno@redhat.com> * feat(scorecard): add detailed documentation for threshold rules in configuration Signed-off-by: Ihor Mykhno <imykhno@redhat.com> * feat(scorecard): enhance threshold configuration with aggregation rules Signed-off-by: Ihor Mykhno <imykhno@redhat.com> * fix(scorecard): average aggregation card percentage value Signed-off-by: Ihor Mykhno <imykhno@redhat.com> * refactor(scorecard): rename `ThresholdRuleAggregationConfig` to `AggregationThresholdRule` Signed-off-by: Ihor Mykhno <imykhno@redhat.com> --------- Signed-off-by: Ihor Mykhno <imykhno@redhat.com>
4 participants
Hey, I just made a Pull Request!
This change adds average as a first-class aggregation KPI type next to statusGrouped, so homepage scorecard cards can show a single portfolio score (donut / gauge) derived from weighted status counts across owned entities, instead of only a status-grouped pie.
Behavior
statusGrouped (unchanged):Aggregates counts per status key across the user’s owned-entity scope, suited to “how many entities are green vs red” style charts.average (new):Uses the same underlying status counts, appliesoptions.statusScores(per threshold rule key → weight), and returnsaverageScorein 0,1 (one decimal). The UI can treataverageScore× 100 as the headline percentage.Donut coloring for average: Optional options.aggregationResultThresholds (same shape as metric thresholds) is evaluated against
averageScore × 100. If omitted, the backend applies built-in defaults (documented in aggregation.md / thresholds.md and aggregationKPIs.ts).Strategy-based backend: Aggregation is wired through a small registry (AverageAggregationStrategy, StatusGroupedAggregationStrategy, shared loader/service) so new types do not sprawl across the router.
Frontend: New aggregated metric card stack (AggregatedMetricCard, AverageCard, StatusGroupedCard, shared chart/tooltip pieces), homepage wiring, translations, mocks/fixtures, and removal of the old monolithic homepage card component in favor of the new layout.
Quality: Unit tests across backend strategies, router, validation, mappers, threshold merge/helpers; Playwright coverage and helpers for the average card on the legacy app.
Configuration
Customization stays under
scorecard.aggregationKPIsinapp-config.yaml. Foraverage, you must supplytype,metricId, andoptions.statusScores(non-empty). Optionaloptions.aggregationResultThresholdscontrols the aggregated result color band.Invalid KPI entries (unknown type, missing options for average, empty statusScores, bad thresholds, unknown metricId, etc.) fail backend startup so misconfiguration is caught early.
New logic implemented for:
✔️ Checklist
How to test
statusGrouped: Confirm existing homepage cards andaggregationIdbehavior unchanged (including legacymetricIdmount patterns if you still support them).averageKPI: Add anaggregationKPIsrow withtype: average, validstatusScores, and mount the homepage card with matchingaggregationId, confirm donut, headline %, tooltips, and colors (default vs customaggregationResultThresholds).statusScores, wrongtype) and confirm the backend fails fast at startup with a clear error.