chore: CNS - Only log CreateOrUpdateNetworkContainer on failure #4047
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR reduces log noise by modifying CNS to only log CreateOrUpdateNetworkContainer responses on failure. Previously, every successful Network Container creation/update was logged, creating excessive noise when DNC constantly refreshes its associations.
- Moved the logger.Response call inside an else block to only execute on non-success return codes
- Added explanatory comment about the purpose of conditional logging
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| logNCSnapshot(req) | ||
| } else { | ||
| // Only emit response trace/log for non-success responses to avoid noisy success logs. | ||
| logger.Response(service.Name, reserveResp, resp.ReturnCode, err) |
There was a problem hiding this comment.
The variable reserveResp appears to be incorrect in this context. Based on the function name createOrUpdateNetworkContainer, this should likely be referencing the create/update response variable, not a reserve response.
Copilot uses AI. Check for mistakes.
peterfelts
changed the title
peterfelts
force-pushed
the
pfelts/remove-noisy-cns-log
branch
from
e9a4708 to
3a3700f
Compare
| logNCSnapshot(req) | ||
| } else { | ||
| // Only emit response trace/log for non-success responses to avoid noisy success logs. | ||
| logger.Response(service.Name, reserveResp, resp.ReturnCode, err) |
There was a problem hiding this comment.
as the Lint says, should we switch to cns/logger/v2?
| logNCSnapshot(req) | ||
| } else { | ||
| // Only emit response trace/log for non-success responses to avoid noisy success logs. | ||
| logger.Response(service.Name, reserveResp, resp.ReturnCode, err) |
There was a problem hiding this comment.
I like this, but want to note: if this logs, it will always result in a redundant log. The only definition of err that reaches here is from common.Encode, which has logging logic within it in the case when err != nil. We should remove that logging logic because, IMO, it should always be the caller's responsibility to do something useful with an error.
There was a problem hiding this comment.
As discussed, I'll leave the logging in common.Encode as that doesn't appear to be happening often. Also, it looks like service.saveNetworkContainerGoalState will return 0, "" is most cases, and Id on't see any case where it returns a success error code along with a message, so this should be safe.
3 participants
Reason for Change:
CNS logs every time a Network Container is created or updated. DNC will constantly call into CNS to refresh its association, resulting in a very high level of the following lines being logged:
[azure-cns] Sent *cns.CreateNetworkContainerResponse &{Response:{ReturnCode:Success Message:}}.This line offers little value as it doesn't have context about which NC was updated.
This change will result in this response line only being logged on error.
Requirements:
Notes: