feat(#4586): add enabled/disabled support for metric providers and metrics - #4589
feat(#4586): add enabled/disabled support for metric providers and metrics#4589fullsend-ai-coder[bot] wants to merge 3 commits into
Conversation
…trics Allow MetricProviders and individual Metrics to be optionally disabled by default. This introduces: - enabled field on the Metric type (scorecard-common) - isEnabled method on MetricProvider interface (scorecard-node) - Config-level enabled attribute for providers and metrics (scorecard-backend) - Resolution chain: config metric > code metric > config provider > code provider > default (true) Disabled metrics are excluded from: - Scheduled data collection (PullMetricsByProviderTask) - Provider task initialization (scheduler) - API responses (router GET /metrics endpoints) - Scaffolder actions (listMetrics) - CatalogMetricService queries Old data for disabled metrics remains in the database. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.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
|
|
🤖 Finished Review · ✅ Success · Started 5:56 PM UTC · Completed 6:30 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Cost: $6.14 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4589 +/- ##
==========================================
+ Coverage 62.37% 62.39% +0.01%
==========================================
Files 2607 2607
Lines 104756 104838 +82
Branches 29439 29449 +10
==========================================
+ Hits 65346 65418 +72
- Misses 37560 37570 +10
Partials 1850 1850
*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 — approvePR: feat(#4586): add enabled/disabled support for metric providers and metrics SummaryThis PR adds an opt-in/opt-out mechanism for scorecard metric providers and individual metrics. It introduces:
Disabled metrics are excluded from scheduling, API listing responses, individual metric endpoints, and scaffolder actions. The implementation correctly returns CorrectnessPrecedence chain implementation — The API endpoint coverage — All relevant routes are protected:
Scheduler belt-and-suspenders — The scheduler check ( Error handling — Backward compatibility — All new fields and methods are optional. Omitting SecurityNo security concerns. Disabled metrics return Intent & coherenceThe implementation matches all three requirements from issue #4586:
The change is well-scoped to the scorecard workspace with no unrelated modifications. The Documentation
Style / conventionsFollows existing codebase patterns consistently:
Notes
Previous runReviewVerdict: Request Changes This PR implements enabled/disabled support for metric providers and metrics (issue #4586). The core resolution function However, the implementation has gaps in endpoint enforcement that directly contradict issue requirement #3, and existing documentation is not updated for the new feature. Incomplete Endpoint Enforcement (medium)Five router endpoints still serve data for disabled metrics, violating the issue requirement that "calls that return metrics just based on the database should check if the metric is enabled or not":
Both the correctness and security reviews independently flagged this gap. Remediation: Add an Documentation Not Updated (high)Three existing documentation files are now stale:
Resolution Chain Priority Concern (medium)The resolution chain places code-level Code Duplication (medium)The Function Naming (medium)
Error Handling Idiom (medium)The bare Information Leakage (low)
Test Gaps (low)
Scheduler Startup Asymmetry (low)The provider-skip check in Unrelated Frontend Changes (low)
|
![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.
|
🤖 Finished Fix · ✅ Success · Started 6:32 PM UTC · Completed 6:55 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Cost: $7.69 |
- Rename isMetricEnabledByDefault to isMetricEnabled for clarity - Reorder resolution chain: config overrides now always take precedence over code defaults (config metric > config provider > code metric > code provider > default) - Add enabled checks to 5 router endpoints that were serving disabled metrics (collectors, deprecated aggregations, aggregations, aggregations metadata, aggregations time-series) - Extract filterEnabledMetrics utility to eliminate try/catch duplication across router, CatalogMetricService, and listMetrics - Add error logging in catch blocks instead of silently swallowing - Fix information leakage: use generic "Metric not found" message instead of revealing disabled state - Add tests for config provider overriding code metric enabled and for disabled metrics on router endpoints - Update docs: providers.md, disabled-metrics-logic.md, README.md Addresses review feedback on #4589
|
🤖 Review · ❌ Terminated · Started 6:56 PM UTC · Ended 7:24 PM UTC Commit: |
Superseded by updated review
| const providers = this.metricProvidersRegistry.listProviders(); | ||
|
|
||
| for (const provider of providers) { | ||
| const hasEnabledMetric = provider |
There was a problem hiding this comment.
[low] test-coverage
The scheduler provider-skip logic (skip providers where all metrics are disabled) lacks a dedicated unit test. The scheduler tests do not exercise the new hasEnabledMetric guard.
Suggested fix: Add a unit test to initializeTasksByProviders that registers a provider with all metrics disabled by config and asserts no task is created for it.
| * is disabled unless the administrator explicitly enables it in | ||
| * `app-config.yaml`. Omitting this field (or setting it to `true`) | ||
| * means the metric is enabled by default. | ||
| */ |
There was a problem hiding this comment.
[low] api-surface
The enabled field on the Metric type is included in API listing responses. When a metric has enabled: false in code but is re-enabled via config, API consumers see enabled: false on an active metric, which could be misleading.
Suggested fix: Consider stripping internal-only fields (like the code-level enabled default) from API responses, or documenting in the API contract that enabled reflects the code-level default, not the effective state.
fullsend-ai-review
Bot
added
the
ready-for-merge
|
🤖 Finished Review · ✅ Success · Started 6:56 PM UTC · Completed 7:24 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Cost: $1.74 |
|
/fs-fix Update api reports to resolve ci issues |
|
🤖 Finished Fix · ✅ Success · Started 10:11 AM UTC · Completed 10:24 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Cost: $2.04 |
Regenerate report.api.md for the scorecard frontend plugin to match API Extractor output. The property ordering in the committed report diverged from what API Extractor generates, causing CI failures. Addresses review feedback on #4589
|
🤖 Review · ❌ Terminated · Started 10:25 AM UTC · Ended 11:07 AM UTC Commit: |
|
|
🤖 Finished Review · ❌ Failure (validation failed after 2 iteration(s)) · Started 10:25 AM UTC · Completed 11:07 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 |
1 participant
Allow MetricProviders and individual Metrics to be optionally disabled by default. This introduces:
provider > default (true)
Disabled metrics are excluded from:
Old data for disabled metrics remains in the database.
Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com
Closes #4586
Post-script verification
agent/4586-metric-provider-enabled-flag)d0f1a88d5157a5d50cff9b7da9bf36eae1ff8742..HEAD)