feat(#3529): move ThresholdConfig from MetricProvider to individual Metrics#3560
feat(#3529): move ThresholdConfig from MetricProvider to individual Metrics#3560fullsend-ai-coder[bot] wants to merge 1 commit into
Conversation
fullsend-ai-coder
Bot
requested review from
a team,
Eswaraiahsapram,
christoph-jerolimov and
its-mitesh-kumar
as code owners
Missing ChangesetsThe following package(s) are changed by this PR but do not have a changeset:
See CONTRIBUTING.md for more information about how to add changesets. Changed Packages
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3560 +/- ##
==========================================
- Coverage 51.07% 51.06% -0.01%
==========================================
Files 2346 2346
Lines 89269 89236 -33
Branches 25020 25001 -19
==========================================
- Hits 45590 45566 -24
+ Misses 43507 43498 -9
Partials 172 172
*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:
|
|
🤖 Finished Review · ✅ Success · Started 12:49 AM UTC · Completed 1:03 AM UTC |
ReviewFindingsHigh
Medium
Low
Previous runReviewFindingsCritical
Medium
Low
Info
|
![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.
|
/fs-fix |
|
🤖 Finished Fix · ❌ Failure · Started 2:42 AM UTC · Completed 4:16 AM UTC |
dzemanov
left a comment
dzemanov
left a comment
There was a problem hiding this comment.
I went briefly through the code and looks very nice, I have only 1 small comment.
I have tested all metrics except for sonarqube, for that one I tested that metric errors correctly display as I don't have it setup.
I have tested that metrics endpoint works correctly when filtered by datasource or metric ids.
Custom thresholds in app config work. Invalid thresholds in entity annotation or app config work. Custom thresholds in entity annotations work. Aggregation cards work. Custom thresholds for statusWeight (average) aggregation cards work correctly.
Can you please add changeset with major breaking change?
Posting this as a comment to avoid blocking this as I will be on PTO.
| ); | ||
| } | ||
|
|
||
| async calculateMetric( |
There was a problem hiding this comment.
Is calculateMetric needed? Can it be removed?
|
/fs-fix
|
|
🤖 Fix · ❌ Terminated · Started 2:13 PM UTC · Ended 3:49 PM UTC |
fullsend-ai-coder
Bot
force-pushed
the
agent/3529-refactor-metric-provider
branch
from
1874c31 to
8fb5093
Compare
|
🤖 Finished Fix · ❌ Failure · Started 2:13 PM UTC · Completed 3:49 PM UTC |
|
🤖 Finished Review · ❌ Failure · Started 3:52 PM UTC · Completed 3:59 PM UTC |
|
/fs-fix revert the changes in workspaces/extensions/plugins/extensions/src/components/TabPanel.tsx |
|
🤖 Finished Fix · ✅ Success · Started 8:59 AM UTC · Completed 9:04 AM UTC |
…etrics Refactor the scorecard MetricProvider interface to move threshold configuration from provider-level methods to individual Metric objects. Core interface changes: - Add `threshold: ThresholdConfig` field to the `Metric` type - Remove `getMetricType()`, `getMetric()`, `getMetricThresholds()`, `calculateMetric()`, and `getMetricIds()` from MetricProvider - Make `getMetrics()` and `calculateMetrics()` required on MetricProvider - Consolidate single/batch provider code paths into batch-only Provider migrations: - All providers (dependabot, filecheck, github, jira, openssf, sonarqube) updated to the new interface with thresholds on metric objects - ThresholdResolver API: `resolveProviderThresholds(provider)` → `resolveMetricThresholds(metric, providerId)` - ThresholdResolver API: `resolveEntityThresholds(entity, provider)` → `resolveEntityThresholds(entity, metric, providerId)` - mergeEntityAndProviderThresholds signature updated similarly - PullMetricsByProviderTask consolidated to always use calculateMetrics() - MetricProvidersRegistry uses provider.getMetrics() for all operations All tests updated. API reports regenerated. Note: DatabaseMetricValues tests could not run (missing better-sqlite3 native binding in sandbox). All other 91 test suites (976 tests) passed. Closes #3529 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
fullsend-ai-coder
Bot
force-pushed
the
agent/3529-refactor-metric-provider
branch
from
8fb5093 to
dbcc8c6
Compare
|
🔧 Fix agent — iteration 1 (bot-triggered)No review feedback was available to address. The review body file was empty, no PR_NUMBER was set, and GH CLI was not authenticated. No code changes were made. Disagreed (1):
Tests: passed Updated by fullsend fix agent |
|
🤖 Finished Review · ✅ Success · Started 9:07 AM UTC · Completed 9:22 AM UTC |
| @@ -38,6 +38,7 @@ export type Metric<T extends MetricType = MetricType> = { | |||
| title: string; | |||
| description: string; | |||
| type: T; | |||
There was a problem hiding this comment.
[high] breaking-public-api
The threshold field is added as a required (non-optional) property on the public Metric type. Any external consumers constructing Metric objects will get compile errors. Additionally, MetricProvider has five methods removed and two previously optional methods made required. This is a semver-breaking change.
Suggested fix: Ensure this ships with a major or minor version bump with appropriate changelog entries and document the migration path for third-party MetricProvider implementors.
|
|
||
| private setConfiguredThresholds(provider: MetricProvider): void { | ||
| const providerId = provider.getProviderId(); | ||
| const metrics = provider.getMetrics(); |
There was a problem hiding this comment.
[low] logic-error
In setConfiguredThresholds, config-level threshold override is validated using metrics[0].type. Config overrides stored per providerId apply to all metrics from that provider, validated only against the first metric's type. The generic constraint MetricProvider ensures type homogeneity, making this theoretical.
| ) {} | ||
|
|
||
| abstract getMetricThresholds(): ThresholdConfig; | ||
| abstract getDefaultThresholds(): ThresholdConfig; |
There was a problem hiding this comment.
[low] naming-consistency
The abstract method getDefaultThresholds() in MockMetricProvider introduces a novel naming pattern; production providers inline threshold constants directly in getMetrics().
| `Invalid default thresholds for metric provider '${providerId}', metric '${metricId}'`, | ||
| error, | ||
| ); | ||
| } |
There was a problem hiding this comment.
[low] error-message-consistency
The test regex for threshold validation error does not verify the new ', metric ...' suffix.
| title: m.metadata.title, | ||
| description: m.metadata.description, | ||
| type: m.metadata.type, | ||
| threshold: m.result.thresholdResult.definition ?? { rules: [] }, |
There was a problem hiding this comment.
[low] fallback-pattern
The threshold field uses m.result.thresholdResult.definition ?? { rules: [] }, producing empty threshold rules as a fallback in dev/mock code. Same pattern in legacy.tsx.
2 participants
Refactor the scorecard MetricProvider interface to move threshold configuration from provider-level methods to individual Metric objects.
Core interface changes:
threshold: ThresholdConfigfield to theMetrictypegetMetricType(),getMetric(),getMetricThresholds(),calculateMetric(), andgetMetricIds()from MetricProvidergetMetrics()andcalculateMetrics()required on MetricProviderProvider migrations:
updated to the new interface with thresholds on metric objects
resolveProviderThresholds(provider)→resolveMetricThresholds(metric, providerId)resolveEntityThresholds(entity, provider)→resolveEntityThresholds(entity, metric, providerId)All tests updated. API reports regenerated.
Note: DatabaseMetricValues tests could not run (missing better-sqlite3 native binding in sandbox). All other 91 test suites (976 tests) passed.
Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com
Closes #3529
Post-script verification
agent/3529-refactor-metric-provider)b57a430ea0c48b4dff885dc812072d5eccb6e0a2..HEAD)