fix(conn): update no-op logger to scheduler logger - #656
Open
thijmv wants to merge 1 commit into
Open
Conversation
|
|
||
| func (c *Conn) readPayload(length int32) ([]byte, error) { | ||
| if err := c.bandwidth.ReserveIngress(int64(length)); err != nil { | ||
| c.log().Errorf("Error reserving ingress bandwidth for piece payload: %s", err) |
Collaborator
There was a problem hiding this comment.
this log tends to show up A LOT for the origin. It's one of our most common logs that we have. I have 2 concerns:
- What if we get it a lot with agent too, causing a lot of noise logging?
- I'm not sure if it's an error, as we are just throttling ourselves intentionally. It should be an Info/Warn or even maybe a Debug log if the amount this happens is a lot.
WDYT?
Collaborator
Author
There was a problem hiding this comment.
- It may happen when the agents are under high load, but I would expect that on the control plane. Furthermore, we currently have a
nocollectrule on the agent's scheduler logs, so we can let it soak without spiking log ingestion rates. - I think this is a valid point. It is not necessarily an error if we deliberately limit the bandwidth. I have downgraded the log level to
INFO, as this is more appropriate thanERROR.
| isPeerOrigin, | ||
| info, | ||
| openedByRemote, | ||
| zap.NewNop().Sugar()) |
Collaborator
There was a problem hiding this comment.
Why did we have a no-op logger in the past and are we 100% sure that we didn't miss any logs (that we shd move from Info/Error to Debug) that this logger is used for?
Collaborator
Author
There was a problem hiding this comment.
It looks like it was refactored but never wired up correctly. The logger is only used for the four logs that this PR touches.
thijmv
force-pushed
the
thijmv/conn-noop-logger
branch
from
August 11, 2026 15:40
2e187d3 to
656d0cb
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Currently, we have no observability on the scheduler's connection logs, since a no-op logger is being passed.
Therefore, this PR: