NO-ISSUE: improve error reporting in cpumetriccollector#31132
NO-ISSUE: improve error reporting in cpumetriccollector#31132simonpasquier wants to merge 1 commit intoopenshift:mainfrom
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@simonpasquier: This pull request explicitly references no jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: simonpasquier The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
WalkthroughError handling in CPU metric collection is enhanced by wrapping errors with contextual messages using ChangesError Handling Improvements
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 🚥 Pre-merge checks | ✅ 12✅ Passed checks (12 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/monitortests/testframework/cpumetriccollector/monitortest.go (1)
124-127: ⚡ Quick winInclude the actual PromQL expression in the query failure error.
At Line 126, the message still doesn’t identify the exact query text, which is the core pain point from the PR description. Including
cpuQuerymakes logs immediately actionable.Proposed patch
- if err != nil { - return nil, fmt.Errorf("failed to query CPU usage per instance: %w", err) - } + if err != nil { + return nil, fmt.Errorf("failed to query CPU usage per instance (query=%q): %w", cpuQuery, err) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/monitortests/testframework/cpumetriccollector/monitortest.go` around lines 124 - 127, The error returned from prometheusClient.QueryRange in monitortest.go does not include the actual PromQL string; update the error handling in the block where cpuMetrics, warningsForQuery, err := prometheusClient.QueryRange(ctx, cpuQuery, timeRange) is checked so that the fmt.Errorf includes the cpuQuery value (the PromQL expression) alongside the wrapped err and context (e.g., "failed to query CPU usage per instance (query=%q): %w"), ensuring logs show the exact query.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/monitortests/testframework/cpumetriccollector/monitortest.go`:
- Around line 124-127: The error returned from prometheusClient.QueryRange in
monitortest.go does not include the actual PromQL string; update the error
handling in the block where cpuMetrics, warningsForQuery, err :=
prometheusClient.QueryRange(ctx, cpuQuery, timeRange) is checked so that the
fmt.Errorf includes the cpuQuery value (the PromQL expression) alongside the
wrapped err and context (e.g., "failed to query CPU usage per instance
(query=%q): %w"), ensuring logs show the exact query.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 47b74056-1182-4d90-b508-189515f11a24
📒 Files selected for processing (1)
pkg/monitortests/testframework/cpumetriccollector/monitortest.go
openshift-merge-bot
Bot
added
the
ready-for-human-review
|
Scheduling required tests: |
|
/cc @rexagod |
|
@simonpasquier: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: d62f037
New tests seen in this PR at sha: d62f037
|
2 participants
While investigating https://redhat.atlassian.net/browse/OCPBUGS-84492, it was hard to understand which PromQL query was failing.
Summary by CodeRabbit