Don't log entire graph #401
Conversation
WalkthroughThe changes remove detailed entitlement graph objects from debug log statements in the Changes
Sequence Diagram(s)Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Pull Request Overview
This PR aims to reduce the overhead of logging by removing the logging of entire graph data structures in debug and error messages.
- Removed logging of potentially large graph objects in several functions.
- Updated debug and error log statements to improve performance and reduce log noise.
| // Peek the next action on the stack | ||
| if len(graph.Actions) == 0 { | ||
| l.Debug("runGrantExpandActions: no actions", zap.Any("graph", graph)) | ||
| l.Debug("runGrantExpandActions: no actions") // zap.Any("graph", graph), |
There was a problem hiding this comment.
The commented out zap.Any parameter can be removed if it is no longer needed for debugging, to help avoid confusion and improve code clarity.
| l.Debug("runGrantExpandActions: no actions") // zap.Any("graph", graph), | |
| l.Debug("runGrantExpandActions: no actions") |
| l.Debug("expandGrantsForEntitlements: graph is expanded") // zap.Any("graph", graph) | ||
| s.state.FinishAction(ctx) | ||
| return nil | ||
| } | ||
|
|
||
| graph.Depth++ | ||
| l.Debug("expandGrantsForEntitlements: graph is not expanded", zap.Any("graph", graph)) | ||
| l.Debug("expandGrantsForEntitlements: graph is not expanded") // zap.Any("graph", graph) |
There was a problem hiding this comment.
Eliminate the commented out zap.Any field to keep the code clean and maintainable if no additional debug information is needed.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pkg/sync/syncer.go (2)
2335-2336: Delete commented-outzap.Any("graph", …)to avoid dead codeThe payload has already been removed from the structured log call, but the commented argument fragments remain.
They don’t compile-break, yet they count as dead code and noisily remain in everygit blame.- l.Debug("runGrantExpandActions: no actions") // zap.Any("graph", graph), + l.Debug("runGrantExpandActions: no actions")Apply the same surgical removal to the other four occurrences to keep the file tidy.
No functional change – pure cleanup.Also applies to: 2524-2525, 2551-2552, 2587-2588, 2593-2594
2548-2554: Consider logging light-weight graph stats instead of nothingNow that the full graph is suppressed, the “exceeded max depth” branch only carries
depth.
Including cheap metrics such aslen(graph.Edges)orlen(graph.GetEntitlements())would still be O(1) but far more actionable when debugging.l.Error("expandGrantsForEntitlements: exceeded max depth", zap.Int("edges", len(graph.Edges)), zap.Int("entitlements", len(graph.GetEntitlements())), zap.Int64("max_depth", maxDepth), )Keeps logs small while preserving the context that engineers previously relied on when the entire graph was dumped.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/sync/syncer.go(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
pkg/sync/syncer.go (2)
Learnt from: ggreer
PR: ConductorOne/baton-sdk#194
File: pkg/uhttp/gocache.go:125-132
Timestamp: 2024-07-30T19:21:37.891Z
Learning: Errors when clearing the cache in the `GoCache` struct are logged later in the process, so additional logging in the `Clear` method is unnecessary and would cause log spam.
Learnt from: ggreer
PR: ConductorOne/baton-sdk#194
File: pkg/uhttp/gocache.go:125-132
Timestamp: 2024-10-08T21:29:30.695Z
Learning: Errors when clearing the cache in the `GoCache` struct are logged later in the process, so additional logging in the `Clear` method is unnecessary and would cause log spam.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: go-test (1.23.x, windows-latest)
Summary by CodeRabbit