feat(scorecard): return threshold config, threholdEvaluation per point - #4685
feat(scorecard): return threshold config, threholdEvaluation per point#4685djanickova wants to merge 3 commits into
Conversation
Signed-off-by: Diana Janickova <djanicko@redhat.com>
|
Important This PR includes changes that affect public-facing API. Please ensure you are adding/updating documentation for new features or behavior. Changed Packages
|
PR Summary by QodoExpose thresholds and evaluations in metric time series
AI Description
Diagram
High-Level Assessment
Files changed (7)
|
|
🤖 Finished Review · ✅ Success · Started 12:11 PM UTC · Completed 12:31 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Cost: $6.67 |
Code Review by Qodo
1.
|
rhdh-qodo-merge
Bot
added
documentation
|
Important The |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4685 +/- ##
=======================================
Coverage 62.58% 62.58%
=======================================
Files 2634 2634
Lines 105187 105199 +12
Branches 29528 29525 -3
=======================================
+ Hits 65833 65843 +10
- Misses 37543 37545 +2
Partials 1811 1811
*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:
|
Review — approveSummaryThis PR extends the entity time-series API ( Changes reviewed
AnalysisCorrectness ✅
Security ✅
API contract ✅
Test coverage ✅
Documentation ✅
Style & conventions ✅
VerdictClean, well-scoped feature addition with good test coverage and documentation. The implementation correctly follows the existing threshold resolution patterns already established in Previous runReviewFindingsMedium
Low
Labels: PR enriches scorecard time-series API with threshold data; enhancement label fits the feature addition. |
![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.
fullsend-ai-review
Bot
added
the
requires-manual-review
Signed-off-by: Diana Janickova <djanicko@redhat.com>
|
🤖 Review · Commit: |
Signed-off-by: Diana Janickova <djanicko@redhat.com>
|
|
🤖 Finished Review · ✅ Success · Started 2:28 PM UTC · Completed 2:35 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Cost: $1.12 |
fullsend-ai-review
Bot
added
ready-for-merge
|
|
||
| let thresholds: ThresholdConfig; | ||
| try { | ||
| thresholds = this.thresholdResolver.resolveEntityThresholds( |
There was a problem hiding this comment.
I don't know if we want to silently switch what thresholds are used if there is error with them?
For snapshot, frontend shows there was error with evaluating entity thresholds because thresholds are malformed.
| let thresholdEvaluation: string | null = null; | ||
| if (row.value !== null) { | ||
| try { | ||
| thresholdEvaluation = |
There was a problem hiding this comment.
Maybe we need to also let frontend know there were evaluation problems.
We will need to fix error handling in backend for next release I think.
Right now everywhere we use:
error?: string
We might deprecate it and use something like:
errors?: [{code: string, message: string}]
This way with codes, frontend can also do translation and it is secure.
There was a problem hiding this comment.
(Almost everywhere, aggregated time-series returns: errors?: [{message: string, count: num}]) so we can follow this pattern here as well, without count, so errors?: [{message: string}])
Eswaraiahsapram
left a comment
Eswaraiahsapram
left a comment
There was a problem hiding this comment.
Thanks @djanickova , tested the changes on top of UI branch. Looks good to me. 🎉
/lgtm
3 participants
Hey, I just made a Pull Request!
Entity time-series API (
GET /metrics/catalog/:kind/:namespace/:name/time-series) now returns entity-resolved thresholds and per-point thresholdEvaluation, so the frontend can render sparkline legends and chart colors without calling the snapshot metrics endpoint.Current types:
How to test
Start Postgres locally and point app-config.local.yaml at it, e.g.:
From workspaces/scorecard:
yarn startSign in as Guest, copy a Bearer token from a successful localhost:7007 request, then:
export TOKEN=<token>Default github.openPRs thresholds are:
thresholdEvaluation is computed at read time from each point’s value against those current rules — the DB status column is ignored for this field.
Seed data
docker exec -it backstage-psql psql -U postgres -d backstage_plugin_scorecardScenarios
Expected:
Top-level thresholds.rules: success / warning / error (or app-config / entity overrides if set)
Points:
Apr 27:
{ "value": 9, "thresholdEvaluation": "success", ... }Apr 28:
{ "value": null, "error": "timeout", ... }— no thresholdEvaluationApr 29:
{ "value": 25, "thresholdEvaluation": "warning", ... }(not "success" from DB)Apr 30:
{ "value": 60, "thresholdEvaluation": "error", ... }(not "warning" from DB)Expected:
value: 25, thresholdEvaluation: "warning"(DB status was success).Expected: one point with
value: null, error: "timeout", no thresholdEvaluation. Response still has thresholds.Expected:
"points": [], metadata present, thresholds present.✔️ Checklist