Skip to content

Conversation

@bohuini
Copy link
Contributor

@bohuini bohuini commented Apr 24, 2023

Reason for Change:
DNC requiring to refresh CNS NC associations is not expected to happen too often, but when it does, we need to log all pertinent details. Added the log in CNS should to show refresh detected and initiated by DNC.

Issue Fixed:

Requirements:

Notes:

@bohuini bohuini requested a review from a team as a code owner April 24, 2023 16:51
@bohuini bohuini requested review from rsagasthya and removed request for a team April 24, 2023 16:51
@bohuini bohuini requested a review from a team as a code owner April 27, 2023 21:36
@bohuini bohuini requested review from huntergregory and removed request for a team April 27, 2023 21:36
@rbtr
Copy link
Collaborator

rbtr commented May 2, 2023

@bohuini pls read this #1921 (comment)

Copy link
Collaborator

@rbtr rbtr left a comment

Choose a reason for hiding this comment

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

the log should not speculate on the intent of what is happening, the log should describe what is happening concisely and in enough detail to be useful during debugging

// If received "GET": Return all NCs in CNS's state file to DNC in order to check if NC refresh is needed
// If received "POST": Store all the NCs (from the request body that client sent) into CNS's state file
func (service *HTTPRestService) getOrRefreshNetworkContainers(w http.ResponseWriter, r *http.Request) {
logger.Printf("[Azure CNS] NC refresh is initiated by DNC. The state file in CNS will get updated if it is lost.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

unnecessary to add an informational log here if all branches of the switch have a log

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.


switch r.Method {
case http.MethodGet:
logger.Printf("[Azure CNS] DNC is attempting to check if the state file in CNS is intact.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

or, GET from getOrRefreshNetworkContainers

Copy link
Collaborator

Choose a reason for hiding this comment

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

you don't have to use my example, but it provides just as much info as yours without the potential of lying to me if that's not why this method was called

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Make sense. Addressed.

service.handleGetNetworkContainers(w)
return
case http.MethodPost:
logger.Printf("[Azure CNS] The state file in CNS is lost. CNS will store all the NCs sent from DNC into state file.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

or, POST to getOrRefreshNetworkContainers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed.

@timraymond
Copy link
Member

I'm with @rbtr here... I think concise logs are better because they tend to tell fewer lies about what's going on as things change over time. Log just the facts, given what's known at the time the log is generated. Given what I see, it would likely be better to have a middleware that logs requests (path, method, some request ID), and responses (HTTP response code, end-to-end time, the same ID for correlation). All of those should be structured using zap. Something like:

func logHTTP(next http.Handler) http.Handler {
	return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
		logger.Info("request", zap.String("uri", r.URL.RequestURI()), zap.String("method", r.Method))
		// need to wrap the ResponseWriter in something that can capture the response code
		wrw := WrappedResponseWriter{rw} // this doesn't exist and needs to be written
		next(wrw, r)
		logger.Info("response", zap.Int("code", wrw.ResponseCode)) // etc.
	})
}

@bohuini bohuini force-pushed the bohui_addLogToHappyPath branch from 963a4e0 to af66656 Compare May 3, 2023 22:28
@bohuini
Copy link
Contributor Author

bohuini commented May 3, 2023

@rbtr Revised the log and rebased branch. Please review again when you get a chance. Thanks!

@bohuini
Copy link
Contributor Author

bohuini commented May 3, 2023

@timraymond That's a good point and I do agree. But for this time I am just making it simple for integration testing purposes.

@bohuini bohuini force-pushed the bohui_addLogToHappyPath branch from af66656 to 68d49f8 Compare May 4, 2023 05:53
@timraymond
Copy link
Member

LGTM 👍 Awaiting @rbtr to hit the approval button.

@rbtr
Copy link
Collaborator

rbtr commented May 4, 2023

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@rbtr rbtr enabled auto-merge (squash) May 4, 2023 17:08
@rbtr rbtr merged commit b9861e2 into Azure:master May 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants