Skip to content

chore: CNS - Only log CreateOrUpdateNetworkContainer on failure #4047

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

chore: CNS - Only log CreateOrUpdateNetworkContainer on failure #4047

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions cns/restserver/api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -567,9 +567,10 @@
// If the NC was created successfully, log NC snapshot.
if returnCode == types.Success {
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)

Check failure on line 572 in cns/restserver/api.go

View workflow job for this annotation

GitHub Actions / Lint (windows-latest) 

SA1019: logger.Response is deprecated: The global logger is deprecated. Migrate to zap using the cns/logger/v2 package and pass the logger instead. (staticcheck)

Check failure on line 572 in cns/restserver/api.go

View workflow job for this annotation

GitHub Actions / Lint (ubuntu-latest) 

SA1019: logger.Response is deprecated: The global logger is deprecated. Migrate to zap using the cns/logger/v2 package and pass the logger instead. (staticcheck)
Copy link

Copilot AI Sep 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as the Lint says, should we switch to cns/logger/v2?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

logger.Response(service.Name, reserveResp, resp.ReturnCode, err)
}

func (service *HTTPRestService) getNetworkContainerByID(w http.ResponseWriter, r *http.Request) {
Expand Down
Loading