feat(scorecard): scorecard timeseries scalar aggregation - #4476
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 |
ReviewFindingsHigh
Medium
Low
Next steps:
Previous runReviewFindingsCritical
High
Medium
Low
Next steps:
Previous run (2)ReviewFindingsCritical
High
Medium
Low
Next steps:
Previous run (3)ReviewFindingsMedium
Low
Previous run (4)ReviewFindingsCritical
Medium
Low
Next steps:
Previous run (5)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.
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>
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 |
Signed-off-by: Dominika Zemanovicova <dzemanov@redhat.com>
|
🤖 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 |
Signed-off-by: Dominika Zemanovicova <dzemanov@redhat.com>
|
🤖 Review · ❌ Terminated · Started 12:16 PM UTC · Ended 12:31 PM UTC Commit: |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
removed
the
requires-manual-review
|
🤖 Finished Review · ✅ Success · Started 12:16 PM UTC · Completed 12:31 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $12.45 |
Signed-off-by: Dominika Zemanovicova <dzemanov@redhat.com>
|
🤖 Review · Commit: |
Signed-off-by: Dominika Zemanovicova <dzemanov@redhat.com>
|
🤖 Review · ❌ Terminated · Started 12:57 PM UTC · Ended 1:36 PM UTC Commit: |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4476 +/- ##
==========================================
+ Coverage 61.61% 61.66% +0.04%
==========================================
Files 2565 2568 +3
Lines 102788 102927 +139
Branches 28851 28893 +42
==========================================
+ Hits 63333 63468 +135
- Misses 37577 37581 +4
Partials 1878 1878
*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:
|
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 12:57 PM UTC · Completed 1:36 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $14.48 |
|
🤖 Review · ❌ Terminated · Started 1:45 PM UTC · Ended 2:05 PM 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 · [low] scope-creep
The PR bundles breaking type/enum changes (removing MetricDefaultVisualizationType, replacing 'value' with 'donut', changing default aggregation logic) with the time-series feature. While these may be prerequisites for the feature, they are logically separable concerns that affect downstream consumers independently.
| @@ -0,0 +1,10 @@ | |||
| --- | |||
| '@red-hat-developer-hub/backstage-plugin-scorecard-common': minor | |||
There was a problem hiding this comment.
[high] breaking-api
The changeset bumps both scorecard-common and scorecard-backend as minor, but the PR contains breaking changes explicitly labeled BREAKING in the changeset body: (1) The publicly exported type MetricDefaultVisualization is removed. (2) The literal 'value' is replaced by 'donut' -- code comparing === 'value' will silently stop matching, and backend Zod schemas now reject 'value' at runtime. (3) The default aggregation type for metric IDs without KPI config changes from always statusGrouped to average for sparkline metrics, altering the REST API response shape. Per semver, these require a major bump.
Suggested fix: Change the changeset for scorecard-common from minor to major. Consider whether scorecard-backend also needs a major bump for the behavioral REST API change. Alternatively, re-export MetricDefaultVisualization as a deprecated alias and keep 'value' in the union to preserve backward compatibility under minor.
| @@ -65,6 +70,14 @@ type ScalarAggregationRowResult = { | |||
|
|
|||
| export class DatabaseMetricValues { | |||
| private readonly tableName = 'metric_values'; | |||
There was a problem hiding this comment.
[low] naming-convention
The constructor parameter type was widened from Knex<any, any[]> to plain Knex during the refactoring to hoist isPostgres to a class field.
| 'github.openPRs', | ||
| )}?from=2024-02-01T00:00:00.000Z&to=2024-01-01T00:00:00.000Z`, | ||
| ); | ||
|
|
There was a problem hiding this comment.
[low] test-inadequate
The router integration test for GET /aggregations/:aggregationId/time-series does not cover the filter.status case. Filter forwarding is tested at the unit level but not end-to-end through the router.
|
🤖 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 |
1 participant
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