Skip to content

Conversation

sdeleuze
Copy link
Contributor

@sdeleuze sdeleuze commented Feb 5, 2018

This commit updates HttpWebHandlerAdapter and ResponseStatusExceptionHandler in order to specify the method/uri in the logged message.

It also logs a WARN message for bad request (400) HTTP responses in order to get some logs when an exception is thrown due to client error (unable to deserialize request body for example).

Issue: SPR-15083

This commit updates HttpWebHandlerAdapter and
ResponseStatusExceptionHandler in order to specify the method/uri in the
logged message.

It also logs a WARN message for bad request (400) HTTP responses in
order to get some logs when an exception is thrown due to client error
(unable to deserialize request body for example).

Issue: SPR-16447
@sdeleuze sdeleuze self-assigned this Feb 5, 2018
@sdeleuze sdeleuze removed the request for review from bclozel February 5, 2018 13:50
@rstoyanchev
Copy link
Contributor

@sdeleuze, it looks fine but a 5xx would now be logged twice at error level it seems. Perhaps the RSEH can skip logging 5xx errors, knowing that those will be logged in HWHA. It'd be useful to have a comment to that extent in the code to make it clear not logging the error is intentional.

@sdeleuze
Copy link
Contributor Author

sdeleuze commented Feb 5, 2018

@rstoyanchev I am not sure 5xx would now be logged twice at error level since it seems that HttpWebHandlerAdapter#handleFailure is only invoked when ResponseStatusExceptionHandler#handle returns Mono.error(ex) (for exception that are not instance of ResponseStatusException), not when it returns exchange.getResponse().setComplete() (for ResponseStatusException instances) so I can't see an overlap between both cases.

Also HttpWebHandlerAdapter and ResponseStatusExceptionHandler logging are slightly different because ResponseStatusExceptionHandler only logs the message (with contains the nested cause) and not the whole stacktrace in order to improve readability of logs and for consistency with Boot (which catches exceptions before our ResponseStatusExceptionHandler but I have replicated the same logic via this PR), so I would be for keeping the proposed arrangement if you are ok with that, if I am correct on the non-duplication.

Any thoughts?

@rstoyanchev
Copy link
Contributor

Yes, you're right. Looks good :)

@sdeleuze
Copy link
Contributor Author

sdeleuze commented Feb 5, 2018

Merged via 196f3f8.

@sdeleuze sdeleuze closed this Feb 5, 2018
@sdeleuze sdeleuze changed the title SPR-16447: Improve WebFlux exception logging SPR-15083: Improve WebFlux exception logging Feb 5, 2018
@sdeleuze sdeleuze deleted the SPR-15083 branch April 28, 2021 09:58
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.

2 participants