Demote replay-fix debug logs to trace level#698
Conversation
The observability logs introduced by the LIVE_START/replay-fix (PR redhat-cne#683) use log.Debug, which fires at the default LOG_LEVEL=debug configured in InitLogger(). This creates excessive log noise in normal operation. Demote all new replay-fix debug logs from Debug to Trace so they remain silent at the default debug level but are still available for deep troubleshooting with LOG_LEVEL=trace. Logs that existed before the replay-fix and were inadvertently downgraded are restored to their original levels (Info/Error).
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughThis PR reduces logging verbosity across the PTP operator codebase by downgrading debug-level logs to trace level in metrics extraction and event generation paths, and in plugin connection message processing. A null check is added to guard the eventManager invocation. ChangesLogging Verbosity Reduction
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@plugins/ptp_operator/ptp_operator_plugin.go`:
- Around line 546-547: In processMessages (the loop using scanner.Scan()), avoid
unconditionally logging an error when scanner.Scan() returns false; instead
check scanner.Err() and call log.Errorf (or similar) only if scanner.Err() !=
nil, and use a debug/trace/info-level log for a normal/clean connection close
when scanner.Err() == nil so that EOF/normal closure doesn't produce error
noise.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 6d880f80-477a-4697-913b-97dc407ba0fb
📒 Files selected for processing (4)
plugins/ptp_operator/metrics/manager.goplugins/ptp_operator/metrics/metrics.goplugins/ptp_operator/metrics/registry.goplugins/ptp_operator/ptp_operator_plugin.go
| log.Errorf("processMessages: scanner returned false (conn closed or error), breaking") | ||
| break |
There was a problem hiding this comment.
Avoid unconditional error logging on normal connection close.
At Line 546, scanner.Scan()==false is logged as Errorf unconditionally, but this also happens on clean EOF. This will produce false error noise and can trigger noisy alerts. Log error only when scanner.Err()!=nil; use trace/debug for normal close.
Proposed fix
ok := scanner.Scan()
if !ok {
- log.Errorf("processMessages: scanner returned false (conn closed or error), breaking")
+ if err := scanner.Err(); err != nil {
+ log.Errorf("processMessages: scanner error: %v", err)
+ } else {
+ log.Trace("processMessages: connection closed by peer")
+ }
break
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| log.Errorf("processMessages: scanner returned false (conn closed or error), breaking") | |
| break | |
| ok := scanner.Scan() | |
| if !ok { | |
| if err := scanner.Err(); err != nil { | |
| log.Errorf("processMessages: scanner error: %v", err) | |
| } else { | |
| log.Trace("processMessages: connection closed by peer") | |
| } | |
| break | |
| } |
🤖 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 `@plugins/ptp_operator/ptp_operator_plugin.go` around lines 546 - 547, In
processMessages (the loop using scanner.Scan()), avoid unconditionally logging
an error when scanner.Scan() returns false; instead check scanner.Err() and call
log.Errorf (or similar) only if scanner.Err() != nil, and use a
debug/trace/info-level log for a normal/clean connection close when
scanner.Err() == nil so that EOF/normal closure doesn't produce error noise.
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: edcdavid, nocturnalastro The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/verified bypass |
|
@nocturnalastro: The 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. |
|
@edcdavid: The following tests failed, say
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. |
|
PR needs rebase. 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. |
1 similar comment
|
PR needs rebase. 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. |
3 participants
The observability logs introduced by the LIVE_START/replay-fix (PR #683)
use log.Debug, which fires at the default LOG_LEVEL=debug configured in
InitLogger(). This creates excessive log noise in normal operation.
Demote all new replay-fix debug logs from Debug to Trace so they remain
silent at the default debug level but are still available for deep
troubleshooting with LOG_LEVEL=trace.
Pre-existing logs that were inadvertently downgraded during the refactoring
are restored to their original levels (Info/Error).