feat(scorecard): scorecard timeseries scalar aggregation - #4476
feat(scorecard): scorecard timeseries scalar aggregation#4476dzemanov wants to merge 10 commits into
Conversation
PR Summary by QodoAdd scalar aggregation time-series support
AI Description
Diagram
High-Level Assessment
Files changed (42)
|
Code Review by Qodo
1. UTC grouping shifts days
|
rhdh-qodo-merge
Bot
added
documentation
|
🤖 Finished Review · ✅ Success · Started 4:35 PM UTC · Completed 4:55 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $15.88 |
ReviewFindingsCritical
Medium
Low
Next steps:
Previous runReviewFindingsCritical
Medium
Low
Next steps:
Previous run (2)ReviewFindingsHigh
Medium
Low
Next steps:
Previous run (3)ReviewFindingsHigh
Medium
Low
Next steps:
Previous run (4)ReviewFindingsHigh
Medium
Low
Next steps:
Previous run (5)ReviewFindingsCritical
High
Medium
Low
Next steps:
Previous run (6)ReviewFindingsCritical
High
Medium
Low
Next steps:
Previous run (7)ReviewFindingsMedium
Low
Previous run (8)ReviewFindingsCritical
Medium
Low
Next steps:
Previous run (9)ReviewFindingsCritical
Medium
Low
Next steps:
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
b9acf33 to
6b9c6c1
Compare
|
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 · ❌ Terminated · Started 8:58 AM UTC · Ended 9:16 AM UTC Commit: |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
🤖 Finished Review · ✅ Success · Started 8:58 AM UTC · Completed 9:16 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $11.75 |
Scorecard uses |
|
🤖 Review · ❌ Terminated · Started 11:19 AM UTC · Ended 11:56 AM UTC Commit: |
Superseded by updated review
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
the
requires-manual-review
|
🤖 Finished Review · ✅ Success · Started 11:19 AM UTC · Completed 11:56 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $13.78 |
|
🤖 Review · ❌ Terminated · Started 12:16 PM UTC · Ended 12:31 PM UTC Commit: |
|
🤖 Finished Review · ✅ Success · Started 1:45 PM UTC · Completed 2:04 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $12.27 |
|
🤖 Review · ❌ Terminated · Started 7:04 AM UTC · Ended 7:22 AM UTC Commit: |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
🤖 Finished Review · ✅ Success · Started 7:04 AM UTC · Completed 7:22 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $12.55 |
Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Dominika Zemanovicova <dzemanov@redhat.com>
Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Dominika Zemanovicova <dzemanov@redhat.com>
Signed-off-by: Dominika Zemanovicova <dzemanov@redhat.com>
Signed-off-by: Dominika Zemanovicova <dzemanov@redhat.com>
Signed-off-by: Dominika Zemanovicova <dzemanov@redhat.com>
Signed-off-by: Dominika Zemanovicova <dzemanov@redhat.com>
Signed-off-by: Dominika Zemanovicova <dzemanov@redhat.com>
54ce9c7 to
e967e83
Compare
|
Rebased to fix conflicts |
|
🤖 Review · ❌ Terminated · Started 12:48 PM UTC · Ended 1:07 PM UTC Commit: |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
🤖 Finished Review · ✅ Success · Started 12:48 PM UTC · Completed 1:07 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $12.42 |
Signed-off-by: Dominika Zemanovicova <dzemanov@redhat.com>
|
🤖 Review · ❌ Terminated · Started 2:15 PM UTC · Ended 2:58 PM UTC Commit: |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
🤖 Finished Review · ✅ Success · Started 2:15 PM UTC · Completed 2:58 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $18.59 |
Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Dominika Zemanovicova <dzemanov@redhat.com>
|
🤖 Review · ❌ Terminated · Started 9:18 AM UTC · Ended 9:35 AM UTC Commit: |
|
There was a problem hiding this comment.
Note: The following review comments could not be posted on the diff (GitHub returned 422) and are included here instead:
workspaces/scorecard/plugins/scorecard-common/src/types/Metric.ts(file-level): Line 30 · [medium] breaking-type-removal
The publicly exported type MetricDefaultVisualization = 'value' | 'sparkline' is deleted without a deprecation path. The replacement ScorecardVisualizationType is not a drop-in substitute: different name and 'value' replaced by 'donut'. See also: [breaking-change-semver-mismatch] finding.
Suggested fix: Add a deprecated re-export: export type MetricDefaultVisualization = ScorecardVisualizationType; and consider keeping 'value' as a member until a major release.
|
|
||
| Add `GET /aggregations/:aggregationId/time-series` for daily scalar portfolio aggregation (`sum`, `average`, `max`, `min`, `count`). Returns aggregated metric values per UTC days. Days with no data are omitted. Aggregation type `statusGrouped` and `weightedStatusScore` return `400`. Sparkline metrics without a KPI block default to aggregation type `average`. | ||
|
|
||
| Adds `metadata.visualization` type to `GET /aggregations/:aggregationId/metadata` response. |
There was a problem hiding this comment.
[critical] breaking-change-semver-mismatch
The changeset marks @red-hat-developer-hub/backstage-plugin-scorecard-common as a minor bump, but the changeset body explicitly states BREAKING: MetricDefaultVisualizationType is removed, 'value' is no longer supported, and the default visualization changed from 'value' to 'donut'. Removing a public exported type and dropping an enum member are backward-incompatible changes per semver. Any downstream consumer importing MetricDefaultVisualizationType or using 'value' will fail to compile after upgrading. This requires a major bump for scorecard-common.
Suggested fix: Change the changeset to '@red-hat-developer-hub/backstage-plugin-scorecard-common': major, or preserve MetricDefaultVisualizationType as a deprecated type alias and keep 'value' in the union to maintain backward compatibility.
|
|
||
| if (!config) { | ||
| this.logger.warn( | ||
| const metric = metricProviderRegistry.getMetric(aggregationId); |
There was a problem hiding this comment.
[medium] behavioral-contract-change
Default aggregation type for metrics without a KPI block changed from always statusGrouped to average when metric.defaultVisualization === 'sparkline'. This affects the existing GET /aggregations/:aggregationId endpoint: consumers relying on the statusGrouped response shape for sparkline metrics will receive a differently-shaped scalar response. Log level also changed from warn to info, reducing operator visibility during the transition.
Suggested fix: Document this behavioral change prominently. Consider whether the log level change from warn to info might reduce visibility for operators during the transition.
| unit: z.string().optional(), | ||
| history: z.boolean().optional(), | ||
| defaultVisualization: z.enum(['value', 'sparkline']).optional(), | ||
| defaultVisualization: z.enum(['donut', 'sparkline']).optional(), |
There was a problem hiding this comment.
[medium] api-validation-change
Zod validation for defaultVisualization changes from z.enum(['value', 'sparkline']) to z.enum(['donut', 'sparkline']). Any metric provider still returning defaultVisualization: 'value' will now fail validation at the API boundary with a 400 error. Same change in listMetrics.ts.
Suggested fix: Ensure all metric providers have been updated, or accept both 'value' and 'donut' during a transition period: z.enum(['value', 'donut', 'sparkline']).
| type: MetricType; | ||
| unit?: string; | ||
| history?: boolean; | ||
| visualization?: ScorecardVisualizationType; |
There was a problem hiding this comment.
[low] api-response-schema-change
AggregationMetadata gains a new optional field visualization?: ScorecardVisualizationType. While backward-compatible per semver, any strict schema validators or snapshot tests in downstream consumers may break.
| ); | ||
|
|
||
| const latestIdsPerEntityPerUTCDay = this.getLatestIdsPerUtcDaySubquery( | ||
| catalogEntityRefs, |
There was a problem hiding this comment.
[low] naming-convention
Variable latestIdsPerEntityPerUTCDay uses UTC in all-caps while the method getLatestIdsPerUtcDaySubquery uses mixed case Utc. Minor casing inconsistency.
|
🤖 Finished Review · ✅ Success · Started 9:18 AM UTC · Completed 9:35 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $12.62 |
djanickova
left a comment
djanickova
left a comment
There was a problem hiding this comment.
Thank you for the PR! I tested it locally and everything worked as expected. I only noticed two small things in docs, other than that, I haven't found any problems.
| - If **`aggregationKPIs` is omitted** or a given id is not listed, aggregation KPIs still work, See [Default aggregation](#default-aggregation). | ||
| - **Startup validation**: the backend validates every **`scorecard.aggregationKPIs`** entry when the plugin loads. Invalid configuration (including **`weightedStatusScore`** KPIs without **`options.statusScores`**, non-count scalar types on boolean metrics, invalid **`filter.status`** keys on scalar types, bad threshold expressions, or unregistered **`metricId`**) causes the backend to **fail to start** with a clear error. At runtime, some edge cases may still be logged (for example skipping a KPI with unusable weights); prefer correcting app-config. See [aggregation.md](./docs/aggregation.md#configuration-validation). | ||
|
|
||
| ### Default aggregation |
There was a problem hiding this comment.
I am wondering whether we have to add this section to this README, or we could just provide a link to the aggregation.md file? I believe most of this is information is now duplicated across both of these files
|
|
||
| Returns a **daily** history of a **scalar** KPI (`sum`, `average`, `max`, `min`, or `count`) across entities you own. Each response point is one UTC day: Scorecard takes **latest stored row** for each owned entity that day (including calculation failures), then rolls successful values up with the KPI’s aggregation type. Optional **`filter.status`** applies only to successes. UTC days with no rows are omitted; a day with only failures is included with **`value: null`**, **`status: error`** and **`errors`** list. | ||
|
|
||
| Only [scalar](./docs/aggregation.md/#scalar-types) aggregation types are supported. **`statusGrouped`** and **`weightedStatusScore`** return **`400 Bad Request`**. See [aggregation.md](./docs/aggregation.md#get-aggregationsaggregationidtime-series) for details. |
There was a problem hiding this comment.
I believe the link should be only ./docs/aggregation.md#scalar-types
2 participants
Hey, I just made a Pull Request!
GET /aggregations/:aggregationId/time-seriesGET /aggregations/:aggregationId/metadatanow includesmetadata.visualization, so frontend can determine what endpoint to call for aggregation data - use new endpointGET /aggregations/:aggregationId/time-seriesif visualization =sparkline, useGET /aggregations/:aggregationIdotherwise.MetricDefaultVisualizationTypetype in favor ofScorecardVisualizationType. The default visualization changes from'value'to'donut'.averagewhen the metric’sdefaultVisualizationissparklinestatusGroupedotherwise.warninglog to.infolog when no aggregation KPI is configured and scorecard uses default aggregation, as it is not an invalid state, it's normal operation. It also reduces log noise.GET /aggregations/:aggregationId/time-series:Only scalar aggregation types (
sum,average,max,min,count) are supported.AggregationTypes
statusGroupedandweightedStatusScoreare not supported and return 400.aggregationChartDisplayColor(in line with naming inweightedStatusScoreaggregation result) comes from value of the last successful point classified against KPI thresholds.No data UTC days are omitted from response.
Aggregation uses
aggregationKPIs.<aggregationId>.options.thresholdsif configured in app-config or by default DEFAULT_NUMBER_THRESHOLDS. This is in line with how scalar aggregation works. For me it would make sense to load default thresholds of a metric instead of using DEFAULT_NUMBER_THRESHOLDS to avoid redefining them in app-config and confusion with filter filtering by original threshold names, to be worked on in a separate ticket.Supports default aggregation that uses
:metricId, when no aggregation config is defined.Example request
Fixes
Fixes https://redhat.atlassian.net/browse/RHIDP-14402
How to test
Requirements: jq, podman, sqlite3
1. Create test data
a) postgres
App-config
b) sqlite
App-config
To view data, you can use SQLite Viewer VS Code Extension. Or you can use any preferred DBMS.
2. Test scalar time-series aggregations
Results for points for different scalar operations:
valueResults for points for different scalar operations with elite filter KPI:
valueCustom KPI
Custom filter KPI
Default KPI (metricId) - uses metric title and description
empty points
only errors
You can test out different scalar aggregation types by updating app-config KPI name and type:
For elite filter:
Note: you can update
app-config.yamlwithout stopping running instance, make sure when using different custom KPI, to also update:aggregationIdin URL fromavgDeploymentFrequencyoravgEliteDeploymentFrequencyTest cases covered by test data:
aggregationChartDisplayColoris classified against latest successful value of metric in time range3. Test invalid inputs
No provider registered
Invalid from
Invalid to
invalid range
invalid from>to
No permissions:
✔️ Checklist