-
Notifications
You must be signed in to change notification settings - Fork 119
feat(scorecard): return threshold config, threholdEvaluation per point #4685
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 3 commits
1cd8364
e8c7efa
b18c0dd
28c827f
9352f0d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| --- | ||
| '@red-hat-developer-hub/backstage-plugin-scorecard-backend': minor | ||
| '@red-hat-developer-hub/backstage-plugin-scorecard-common': minor | ||
| --- | ||
|
|
||
| Entity time-series API (`GET /metrics/catalog/:kind/:namespace/:name/time-series`) now returns entity-resolved `thresholds` and per-point `thresholdEvaluation` (classified at read time against those current thresholds) so clients can render sparkline legends and chart colors without a separate snapshot call. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -50,6 +50,7 @@ import { isMetricCalculationError } from '../utils/metricCalculationError'; | |
| import { AggregatedMetricMapper } from './mappers'; | ||
| import { DbMetricValue } from '../database/types'; | ||
| import { ThresholdResolver } from '../threshold/ThresholdResolver'; | ||
| import { ThresholdEvaluator } from '../threshold/ThresholdEvaluator'; | ||
|
|
||
| type CatalogMetricServiceOptions = { | ||
| catalog: CatalogService; | ||
|
|
@@ -82,6 +83,7 @@ export class CatalogMetricService { | |
| private readonly registry: MetricProvidersRegistry; | ||
| private readonly database: DatabaseMetricValues; | ||
| private readonly thresholdResolver: ThresholdResolver; | ||
| private readonly thresholdEvaluator = new ThresholdEvaluator(); | ||
|
|
||
| private static readonly MAX_FETCHABLE_ROWS = 10_000; | ||
| private static readonly BATCH_SIZE = 100; | ||
|
|
@@ -233,6 +235,17 @@ export class CatalogMetricService { | |
| to, | ||
| ); | ||
|
|
||
| let thresholds: ThresholdConfig; | ||
| try { | ||
| thresholds = this.thresholdResolver.resolveEntityThresholds( | ||
| entity, | ||
| metric, | ||
| ); | ||
| } catch { | ||
| // Keep app-config / provider thresholds when entity annotation merge fails | ||
| thresholds = this.thresholdResolver.resolveMetricThresholds(metric); | ||
| } | ||
|
|
||
| const points: MetricTimeSeriesPoint[] = rows.map(row => { | ||
| if (isMetricCalculationError(row)) { | ||
| return { | ||
|
|
@@ -241,9 +254,29 @@ export class CatalogMetricService { | |
| error: row.errorMessage!, | ||
| }; | ||
| } | ||
|
|
||
| let thresholdEvaluation: string | null = null; | ||
| if (row.value !== null) { | ||
| try { | ||
| thresholdEvaluation = | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. This way with codes, frontend can also do translation and it is secure.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (Almost everywhere, aggregated time-series returns: |
||
| this.thresholdEvaluator.getFirstMatchingThreshold( | ||
| row.value, | ||
| metric.type, | ||
| thresholds, | ||
| ) ?? null; | ||
| } catch (error) { | ||
| this.logger.warn( | ||
| `Failed to evaluate thresholds for metric '${ | ||
| metric.id | ||
| }' on entity '${entityRef}': ${stringifyError(error)}`, | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| return { | ||
| value: row.value, | ||
| timestamp: row.timestamp.toISOString(), | ||
| thresholdEvaluation, | ||
| }; | ||
| }); | ||
|
|
||
|
|
@@ -260,6 +293,7 @@ export class CatalogMetricService { | |
| defaultVisualization: metric.defaultVisualization, | ||
| collectorIds: metric.collectorIds, | ||
| }, | ||
| thresholds, | ||
| }; | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -138,6 +138,12 @@ export type MetricTimeSeriesPoint = { | |
| timestamp: string; | ||
| /** Present when this point is a calculation failure */ | ||
| error?: string; | ||
| /** | ||
| * Matched threshold rule key from read-time evaluation against the response | ||
| * `thresholds` (e.g., "elite", "success", "warning"). | ||
| * `null` when the value could not be classified. Absent on calculation-error points. | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [low] naming-convention The new field thresholdEvaluation introduces a third name for the same domain concept. The codebase uses evaluation in ThresholdResult and status in EntityMetricDetail/DbMetricValue. |
||
| */ | ||
| thresholdEvaluation?: string | null; | ||
| }; | ||
|
|
||
| /** | ||
|
|
@@ -157,4 +163,9 @@ export type MetricTimeSeriesResponse = { | |
| defaultVisualization?: ScorecardVisualizationType; | ||
| collectorIds?: string[]; | ||
| }; | ||
| /** | ||
|
djanickova marked this conversation as resolved.
|
||
| * Entity-resolved threshold rules (provider defaults, then app-config, then entity annotation overrides). | ||
| * Used for sparkline legend rendering and mapping `thresholdEvaluation` keys to colors. | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [low] api-shape-consistency thresholds is required on MetricTimeSeriesResponse but ThresholdConfig | undefined on MetricResult.result.thresholdResult.definition. Different contracts for the same underlying data, by design due to different error-handling strategies. |
||
| */ | ||
| thresholds: ThresholdConfig; | ||
| }; | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. Updated this in the latest version to be returned as a response thresholdsError