accessman: add logging to new sub-system #9719
Conversation
|
Important Review skippedAuto reviews are limited to specific labels. 🏷️ Labels to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 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:
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 adds logging support to the new Access Manager sub‑system, enabling easier monitoring and debugging.
- Introduces a new logger (acsmLog) for the Access Manager.
- Inserts various debug, info, and warning log statements in key state transitions and error branches.
- Enhances error reporting in the access management logic.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| log.go | Added new Access Manager logger (acsmLog). |
| accessman.go | Inserted logging calls in permission assignment, pending channel and connection methods. |
Comments suppressed due to low confidence (2)
accessman.go:86
- [nitpick] Consider storing the serialized peer key as a []byte instead of converting it to a string and then back to []byte in logging calls; this can improve clarity and help avoid potential formatting inconsistencies.
peerMapKey := string(remotePub.SerializeCompressed())
accessman.go:435
- Consider using %s instead of %x when formatting the peer identifier, or convert peerMapKey to a []byte to match the %x verb. This will ensure the error message is formatted as intended.
err := fmt.Errorf("invalid peer access status: new open channel for restricted peer %x", peerMapKey)
There was a problem hiding this comment.
We can make the diff way smaller by not using a global context but instead deriving one at the start of a call (using context.TODO for now to indicate that it should eventually be replace by a ctx param) and then adding certain info (like the peer pub key) to that initial context - then it doesnt need to be re-added each time you log
SGTM. I did the first commit, then decided it would be better to use the latest n greatest logging. |
There was a problem hiding this comment.
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
accessman.go:85
- [nitpick] Consider replacing context.TODO() with a proper context passed from upstream to improve context propagation and cancellation support in production scenarios.
ctx := btclog.WithCtx(context.TODO(), lnutils.LogPubKey("peer", remotePub))
accessman.go:302
- [nitpick] Consider removing the trailing colon in the log message to maintain consistency with other log messages.
acsmLog.WarnS(ctx, "Peer last pending channel closed: ", ErrNoMoreRestrictedAccessSlots, "num_restricted", a.numRestricted, "max_restricted", a.cfg.maxRestrictedSlots)
This captures a common pattern where we want to log a peer's public key along side each logging statement.
This commit adds logs to the new access manager. This'll help us monitor the new system behavior, and may make debugging easier in the future.
4 participants
In this PR, we add logging to the new Access Manager. It was committed without any logs at all, which otherwise would've made it hard to monitor and debug the new sub-system.