Fine-tune and review logging output #13511
Comments
|
/cc @rstoyanchev |
|
Also, we should track the results of SPR-16954. |
|
A few concrete items:
|
|
The following output appears for when an error mapping is involved: Notice the "Matched ... RequestPredicate" message which is logged in the Spring Framework at TRACE level. It's not clear that what's going is error mapping. It would help to a) have an extra message before that which says something like "routing error" or "mapping error", and b) maybe the predicate could be refactored into an actual inner class instead of a lambda so it appears under a more readable name or even a toString method to choose the name. |
|
I've just applied the following changes to my working copy:
Spring WebFluxGiven the following Controller, I've tested each endpoint to see the logging output. @RestController
public class HomeController {
@GetMapping("/")
public Mono<String> greet(@RequestParam String name) {
return Mono.just("Hello, " + name);
}
@GetMapping("/boom")
public Mono<String> boom() {
return Mono.error(new IllegalArgumentException("Something illegal"));
}
@GetMapping("/status")
public Mono<String> status() {
return Mono.error(new ResponseStatusException(HttpStatus.BAD_REQUEST));
}
}Spring MVCSame thing for Spring MVC @RestController
public class MvcHomeController {
@GetMapping("/")
public String greet(@RequestParam String name) {
return "Hello, " + name;
}
@GetMapping("/boom")
public String boom() {
throw new IllegalArgumentException("Something illegal");
}
@GetMapping("/status")
public String status() {
throw new ResponseStatusException(HttpStatus.BAD_REQUEST);
}
}Possible improvements
|
|
Neat! What about also Very minor but this seems to have some but not all changes from this commit yestereday spring-projects/spring-framework@14d0fee. For example the The stack trace is coming from Tomcat. Perhaps FrameworkServlet could stop exceptions, or have a property to stop exceptions and set the status to 500. That should still trigger the error dispatch, I hope? The extra dispatcher servlet init comes from the FrameworkServlet writing via |
|
I've just updated the sample logs after configuring Logging request detailsAs you've said, there are now ways to configure whether to log request details (header and body, which might contain sensitive information):
We should support those as configuration properties (and decide whether we want to enable those by default with devtools). spring.mvc.log-request-details=true
spring.webflux.log-request-details=trueThe obvious solution is a bit strange, because one is server only, and the other one server and client, even if they're doing (almost) the same thing. Also, since the webflux one is done on the codecs, this solution is creating a dependency between We could also consider a different solution with another namespace that's about giving insights into the application. I don't know if this really deserve a new namespace, but I'm wondering if more things like this are expected. spring.insights.web.log-request-details=trueI'm marking this issue for team-attention, especially for that last bit. |
bclozel
added
the
for: team-attention
|
|
|
This should also make it easier to cover the scenario Spring MVC Controller + WebClient I think. |
|
SPR-17100 is now resolved, including a new I recommend setting the property to "true" in Spring Boot and I don't see a reason to have to customize it. The Servlet spec says an Error Page is used when: I tried this with a Boot app and it works as expected, i.e. Tomcat no longer logs the stacktrace. |
bclozel
removed
the
for: team-attention
|
This change broke the build. Enabling the Here's the working case: Here is the failing case: @rstoyanchev, @rwinch, |
bclozel
added a commit
that referenced
this issue
Aug 16, 2018
This change broke a Spring Security sample, reverting it. See gh-13511
|
The problem with the dispatcher servlet handling the failure is the same as one that Jersey has by default. See #12995 and this change which documented how to stop Jersey from handling the failure so that Spring Security could see it when using method security. |
wilkinsona
added a commit
that referenced
this issue
Aug 16, 2018
|
In a Boot application propagating exceptions to the container causes them to be logged, needlessly, before being forwarded and handled by the ErrorController. The goal of the change was to eliminate that unnecessary stack trace. I did start with Arguably this isn't a problem for the |
|
Thanks @wilkinsona and @rstoyanchev ! |
bclozel
added a commit
that referenced
this issue
Aug 17, 2018
The `shouldHandleFailure` configuration option has been removed from `DispatcherServlet`. See gh-13511
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
See SPR-16946.
The text was updated successfully, but these errors were encountered: