Use compact logging only for List* RPCs#9919
Conversation
|
Hi @fmuyassarov. Thanks for your PR. I'm waiting for a cri-o member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR updates the UnaryInterceptor logging to detect RPCs whose operation name starts with "List" and logs their Request/Response using compact %#v formatting; other RPCs keep the original type-aware ChangesDebug Logging Format for List RPCs*
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested labels
Suggested reviewers
Poem
🚥 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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
|
@fmuyassarov thank you for the PR! Do you mind signing off your commit? @bitoku PTAL |
|
/ok-to-test |
abcf133 to
599a575
Compare
Ops, my bad. Should be signed now :) |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/log/interceptors/interceptors.go (1)
65-65: ⚡ Quick winLGTM — correct revert; consider adding a short comment capturing the why.
The switch back to
%#vis the right fix. Withgoogle.golang.org/protobuf-generated types, the%+vverb triggers the prototext serializer (an expensive, allocation-heavy path), whereas%#vfalls through to Go's standard reflection formatter, matching the fast pre-v1.35.0 behaviour confirmed by issue#9894.Because the next engineer who wants to "clean up the log format" may accidentally reproduce this regression, a one-line comment would prevent future churn:
💬 Suggested protective comment
- log.Debugf(newCtx, "Request: %#v", req) + // Use %#v (Go-syntax representation) rather than %+v/%T:%+v. + // With google.golang.org/protobuf generated types, %+v triggers the + // expensive prototext serializer and can cause deadline-exceeded under load. + log.Debugf(newCtx, "Request: %#v", req)- log.Debugf(newCtx, "Response: %#v", resp) + // Same rationale as the Request log above. + log.Debugf(newCtx, "Response: %#v", resp)Also applies to: 77-77
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/log/interceptors/interceptors.go` at line 65, Add a one-line comment above the log.Debugf call in interceptors.go (the call to log.Debugf(newCtx, "Request: %#v", req)) explaining why the %#v verb is used instead of %+v: note that %+v triggers the protobuf prototext serializer for google.golang.org/protobuf-generated types (causing expensive allocations and regressions) while %#v falls back to Go's standard formatter and preserves the pre-v1.35.0 performance; keep the comment short and reference the protobuf/issue behavior to prevent future accidental reversion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/log/interceptors/interceptors.go`:
- Line 65: Add a one-line comment above the log.Debugf call in interceptors.go
(the call to log.Debugf(newCtx, "Request: %#v", req)) explaining why the %#v
verb is used instead of %+v: note that %+v triggers the protobuf prototext
serializer for google.golang.org/protobuf-generated types (causing expensive
allocations and regressions) while %#v falls back to Go's standard formatter and
preserves the pre-v1.35.0 performance; keep the comment short and reference the
protobuf/issue behavior to prevent future accidental reversion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: feee526b-15f5-452b-8783-fc13c187ff71
📒 Files selected for processing (3)
internal/log/interceptors/interceptors.gotest/ctr.batstest/logs.bats
💤 Files with no reviewable changes (1)
- test/ctr.bats
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9919 +/- ##
==========================================
- Coverage 67.59% 67.49% -0.11%
==========================================
Files 215 215
Lines 29678 29708 +30
==========================================
- Hits 20061 20050 -11
- Misses 7910 7958 +48
+ Partials 1707 1700 -7 🚀 New features to boost your workflow:
|
|
Seeing issues in most of the prow job failures like: Seems somewhat unrelated to the change itself but perhaps someone can confirm it |
|
yeah the failures aren't related thanks for the research and fix! |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fmuyassarov, haircommander 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 |
|
/hold Thanks for the PR! However, I think we should fix this a bit differently. As mentioned in #9501, using (Also, this change qualifies for a release note.) |
621e9e3 to
e2be80c
Compare
Thanks for the review @bitoku . You have a point, it would be too expensive to sacrifice the debugging. Can you please take a look once more. I've updated it now to keep |
|
If looks good, I will squash the commits and update the title, etc. |
|
/release-note-edit |
|
@fmuyassarov can you run |
|
Could you provide the fixed logs? |
Sure, here is a small snippet |
List* RPCs can return large payloads that, when formatted with %+v, produce large protobuf text log messages causing gRPC deadline exceeded errors (see issue cri-o#9894). Signed-off-by: Feruzjon Muyassarov <feruzjon.muyassarov@est.tech>
e2be80c to
7ffa9b6
Compare
Thanks done. I've also updated & squashed the commits. |
|
/lgtm |
|
/retest |
1 similar comment
|
/retest |
this is somewhat similar to the previosly failed tests, which doesn't seem related to the actual changes |
|
@haircommander would you mind to help to get this merged if it LGTY ? |
|
/test e2e-gcp-ovn |
|
/retest |
|
/test ci-rhel-e2e |
|
/cherry-pick release-1.36 |
|
@bitoku: new pull request created: #9951 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 kubernetes-sigs/prow repository. |
Thank you @bitoku |
/kind cleanup
What this PR does / why we need it:
Address regression when handling crictl requests.
Which issue(s) this PR fixes:
Fixes: #9894
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Summary by CodeRabbit