Skip to content

chore(scorecard): log metric provider errors#2844

Merged
christoph-jerolimov merged 1 commit intoredhat-developer:mainfrom
christoph-jerolimov:scorecard/log-metric-provider-errors
Apr 21, 2026
Merged

chore(scorecard): log metric provider errors#2844
christoph-jerolimov merged 1 commit intoredhat-developer:mainfrom
christoph-jerolimov:scorecard/log-metric-provider-errors

Conversation

@christoph-jerolimov
Copy link
Copy Markdown
Member

Hey, I just made a Pull Request!

Logs a warning when a metric provider fails for an entity. Wdyt? :)

✔️ Checklist

  • A changeset describing the change and affected packages. (more info)
  • Added or Updated documentation
  • Tests for new functionality and regression tests for bug fixes
  • Screenshots attached (for UI changes)

Signed-off-by: Christoph Jerolimov <jerolimov+git@redhat.com>
@rhdh-qodo-merge
Copy link
Copy Markdown

rhdh-qodo-merge Bot commented Apr 20, 2026

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (0) 📎 Requirement gaps (0)

Grey Divider


Remediation recommended

1. Unsafe error interpolation 🐞 Bug ☼ Reliability
Description
The new warn message interpolates the caught error directly (`...: ${error}`), which can itself
throw for some non-Error thrown values; if that happens, the per-entity promise rejects and the
failure record is dropped from batchResults. It also omits the error meta for non-Error throws,
reducing debuggability compared to existing patterns in this repo.
Code

workspaces/scorecard/plugins/scorecard-backend/src/scheduler/tasks/PullMetricsByProviderTask.ts[R184-189]

+              logger.warn(
+                `Failed to calculate metric for entity ${stringifyEntityRef(
+                  entity,
+                )}: ${error}`,
+                error instanceof Error ? error : undefined,
+              );
Relevance

⭐⭐⭐ High

Unsafe ${error} can throw; repo prefers robust error handling to avoid dropped results.

PR-#1544
PR-#2509
PR-#2393

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
logger.warn is called before returning the fallback DbMetricValueCreate; if that log statement
throws, the async mapper rejects and the reducer only keeps fulfilled results, so the error record
is not persisted. Elsewhere in this repo, the safer pattern is to convert unknown to a string
first and still pass the original error to the logger.

workspaces/scorecard/plugins/scorecard-backend/src/scheduler/tasks/PullMetricsByProviderTask.ts[142-210]
workspaces/scorecard/plugins/scorecard-backend/src/scheduler/tasks/PullMetricsByProviderTask.ts[181-200]
workspaces/scorecard/plugins/scorecard-backend-module-dependabot/src/clients/DependabotClient.ts[108-114]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`PullMetricsByProviderTask` logs calculation failures with a template literal that interpolates `error` directly and conditionally drops log meta for non-`Error` throws. This can make the logging path throw and cause the per-entity promise to reject (dropping the DB error record), and it reduces debuggability.

### Issue Context
There is an established pattern in the repo (e.g., DependabotClient) of converting `unknown` errors to a safe string (`error instanceof Error ? error.message : String(error)`) while still passing the original `error` as the second argument to the logger.

### Fix Focus Areas
- workspaces/scorecard/plugins/scorecard-backend/src/scheduler/tasks/PullMetricsByProviderTask.ts[181-200]

### Suggested change
- Precompute `entityRef` once.
- Precompute `errorMessage` using `error instanceof Error ? error.message : String(error)`.
- Log `...: ${errorMessage}` and pass `error` as the 2nd arg (or `{ error }` if your LoggerService expects meta objects).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Per-entity warning spam 🐞 Bug ➹ Performance
Description
The new warning runs inside the per-entity loop in a paginated do/while, so a provider outage can
emit one warning per entity across all pages. This can flood logs and slow the scheduled task due to
logging overhead, making the pull less reliable.
Code

workspaces/scorecard/plugins/scorecard-backend/src/scheduler/tasks/PullMetricsByProviderTask.ts[R184-189]

+              logger.warn(
+                `Failed to calculate metric for entity ${stringifyEntityRef(
+                  entity,
+                )}: ${error}`,
+                error instanceof Error ? error : undefined,
+              );
Relevance

⭐⭐⭐ High

Prior reviews pushed to reduce per-entity log noise; warn-in-loop likely considered spammy.

PR-#2393
PR-#2351
PR-#1544

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
logger.warn is executed inside entitiesResponse.items.map(async entity => ...) and that mapping
happens inside a cursor-based do/while pagination loop, so the warning can be emitted for every
entity processed during a run.

workspaces/scorecard/plugins/scorecard-backend/src/scheduler/tasks/PullMetricsByProviderTask.ts[129-215]
workspaces/scorecard/plugins/scorecard-backend/src/scheduler/tasks/PullMetricsByProviderTask.ts[181-201]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The current implementation logs a warning for every entity whose metric calculation fails. During widespread failures (e.g., provider dependency outage), this creates unbounded log volume and can slow or destabilize the scheduled run.

### Issue Context
The warning is emitted inside the per-entity mapper within a paginated loop.

### Fix Focus Areas
- workspaces/scorecard/plugins/scorecard-backend/src/scheduler/tasks/PullMetricsByProviderTask.ts[129-215]

### Suggested change
Implement one of the following:
- Log per-entity failures at `debug` and add a single `warn` summary per run (e.g., `failedCount`, plus maybe first N entity refs).
- Or log only the first N failures per run at `warn` and then emit a final summary `warn` with total failures.
This keeps the new visibility while preventing log storms.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

ⓘ The new review experience is currently in Beta. Learn more

Grey Divider

Qodo Logo

@rhdh-gh-app
Copy link
Copy Markdown

rhdh-gh-app Bot commented Apr 20, 2026

Missing Changesets

The following package(s) are changed by this PR but do not have a changeset:

  • @red-hat-developer-hub/backstage-plugin-scorecard-backend

See CONTRIBUTING.md for more information about how to add changesets.

Changed Packages

Package Name Package Path Changeset Bump Current Version
@red-hat-developer-hub/backstage-plugin-scorecard-backend workspaces/scorecard/plugins/scorecard-backend none v2.5.1

@sonarqubecloud
Copy link
Copy Markdown

@rhdh-qodo-merge
Copy link
Copy Markdown

Review Summary by Qodo

Log metric provider errors with entity context

✨ Enhancement

Grey Divider

Walkthroughs

Description
• Add warning logs when metric provider fails for entity
• Include error details and entity reference in log message
• Improve observability of metric calculation failures
Diagram
flowchart LR
  A["Metric Calculation"] -->|"Error Occurs"| B["Log Warning"]
  B -->|"Include Entity Ref"| C["Error Details Logged"]
  C -->|"Continue Processing"| D["Return NULL Status"]
Loading

Grey Divider

File Changes

1. workspaces/scorecard/plugins/scorecard-backend/src/scheduler/tasks/PullMetricsByProviderTask.ts ✨ Enhancement +6/-0

Add error logging for metric provider failures

• Add warning log when metric calculation fails for an entity
• Include entity reference and error details in log message
• Pass Error object to logger for better error tracking
• Maintain existing behavior of returning NULL status on failure

workspaces/scorecard/plugins/scorecard-backend/src/scheduler/tasks/PullMetricsByProviderTask.ts


Grey Divider

Qodo Logo

@rhdh-qodo-merge rhdh-qodo-merge Bot added enhancement New feature or request Other labels Apr 21, 2026
Copy link
Copy Markdown
Member

@dzemanov dzemanov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@christoph-jerolimov can you add changeset?

@christoph-jerolimov christoph-jerolimov enabled auto-merge (squash) April 21, 2026 08:58
@christoph-jerolimov christoph-jerolimov merged commit 51550a4 into redhat-developer:main Apr 21, 2026
12 checks passed
@christoph-jerolimov christoph-jerolimov deleted the scorecard/log-metric-provider-errors branch April 21, 2026 08:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants