-
Notifications
You must be signed in to change notification settings - Fork 618
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.
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
Add logic in WriteJSONToResponse to log errors in ECS agent log #2641
Conversation
agent/handlers/utils/helpers.go
Outdated
@@ -129,3 +131,10 @@ func LimitReachedHandler(auditLogger audit.AuditLogger) func(http.ResponseWriter | |||
auditLogger.Log(logRequest, http.StatusTooManyRequests, "") | |||
} | |||
} | |||
|
|||
// WriteErrorToDefaultLogger write the requestType, httpStatusCode and responseJSON of error to the default logger | |||
func WriteErrorToDefaultLogger(httpStatusCode int, responseJSON []byte, requestType string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I think the function name is a bit misleading, there's no error passed as parameter, also the function is not reused anywhere. I think I'd prefer to get rid of the function and just put the code directly in WriteJSONToResponse
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion. function WriteErrorToDefaultLogger
is now integrated into function WriteJSONToResponse
from line 84 to 86.
agent/handlers/utils/helpers.go
Outdated
|
||
// WriteErrorToDefaultLogger write the requestType, httpStatusCode and responseJSON of error to the default logger | ||
func WriteErrorToDefaultLogger(httpStatusCode int, responseJSON []byte, requestType string) { | ||
if httpStatusCode >= 400 && httpStatusCode <= 599 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it be better to have if httpStatusCode<200 || httpStatusCode>299
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As httpStatusCode 1xx and 3xx would not be considered as an error code; therefore the if statement httpStatusCode >= 400 && httpStatusCode <= 599
is used here to only propagate httpStatusCode 4xx and 5xx to the agent log.
00a5b83
to
33ce793
Compare
33ce793
to
eb83b54
Compare
Summary
This PR adds logic to propagate responses of requests to the Amazon ECS Container Agent Log when an error occurs.
Implementation details
This PR includes the following change in agent/handlers/utils/helpers.go:
WriteJSONToResponse
function to propagate HTTP response status codes, request types and responses in JSON to the Amazon ECS Container Agent Log when the response status code is 4xx client error or 5xx server error.Testing
Manually tested the changes when HTTP response status code 500 occurs, and verified the HTTP response status code, the request type and responses are successfully propagated to Amazon ECS Container Agent Log.
This change is not covered by new tests
Description for the changelog
Improvement - Propagate responses to the Amazon ECS Container Agent Log when an error occurs
Licensing
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.