If REST controller doesn't explicitly set status code (so it defaults to 200 OK), MetricsWebFilter
will set outcome
tag to UNKNOWN
instead of SUCCESS
.
Example application to reproduce:
package net.example;
import org.springframework.boot.SpringApplication;
import org.springframework.boot.autoconfigure.SpringBootApplication;
import org.springframework.web.bind.annotation.GetMapping;
import org.springframework.web.bind.annotation.RestController;
import reactor.core.publisher.Mono;
@SpringBootApplication
public class ExampleApplication {
public static void main(String[] args) {
SpringApplication.run(ExampleApplication.class, args);
}
}
@RestController
class ExampleController {
@GetMapping("/example")
Mono<String> example() {
return Mono.just("EXAMPLE");
}
}
management:
endpoints:
web:
exposure:
include:
- metrics
curl http://localhost:8080/example
curl http://localhost:8080/actuator/metrics/http.server.requests?tag=uri:/example
# availableTags{tag="outcome"} only has "UNKNOWN" value
Comment From: michaelmcfadyen
I can confirm this behaviour is present in spring boot 2.2.2
Comment From: michaelmcfadyen
The behaviour looks to be caused by the following method: https://github.com/spring-projects/spring-boot/blob/b15e427a3e5d7232e21fcfac43c02763f73b68ad/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/web/reactive/server/WebFluxTags.java#L145
There is a discrepancy between how the status code is resolved for the status code tag how it is resolved for the outcome tag.
Comment From: bclozel
In spring-projects/spring-framework#21901, we can see that the ServerHttpResponse
contract is about modifying the response and falling back on the server defaults (HTTP 200 OK) if nothing has been set by the application.
In gh-18267, we also see that we rely on org.springframework.http.server.reactive.AbstractServerHttpResponse#getStatusCodeValue
to handle non-standard status codes. But this method does not itself look into the native server response and can return null
.
I'm wondering if org.springframework.http.server.reactive.AbstractServerHttpResponse#getStatusCodeValue
should return the underlying default server response status. Of if we should, for consistency, return Outcome.SUCCESS
in https://github.com/spring-projects/spring-boot/blob/bb3b6dbd7d213e7898fb1f2a88bcea8aae8b6c7f/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/web/reactive/server/WebFluxTags.java#L137-L141
@rstoyanchev do you think we should rely at all on org.springframework.http.server.reactive.AbstractServerHttpResponse#getStatusCodeValue
in the first place - if not, how should we handle non-standard HTTP status codes?
Comment From: rstoyanchev
AbstractServerHttpResponse#getStatusCodeValue
could be improved to fall back on getStatusCode()
but I'm reluctant to change it because it is the only way to know if the status has been set at all. It is also a bit of an internal method to begin with.
I think you could update WebFluxTags
to something like this:
private static Integer extractStatusCode(ServerWebExchange exchange) {
ServerHttpResponse response = exchange.getResponse();
Integer statusCode = null;
if (response instanceof AbstractServerHttpResponse) {
statusCode = ((AbstractServerHttpResponse) response).getStatusCodeValue();
}
if (statusCode == null) {
HttpStatus httpStatus = response.getStatusCode();
statusCode = (httpStatus != null ? httpStatus.value() : null);
}
return statusCode;
}
Comment From: bclozel
Thanks Rossen, we'll switch from Outcome.UNKNOWN
to Outcome.SUCCESS
by default, since this will be the default HTTP status if no other information is provided. We can use that opportunity to improve status extraction with the code snippet you suggested. Thanks!
Comment From: mbhave
@rstoyanchev @bclozel I'm trying to understand why the above snippet is better than what we currently have. Is there any situation where an AbstractServerHttpResponse#getStatusCodeValue
would return null
but AbstractServerHttpResponse#getStatusCode
would not?
Comment From: rstoyanchev
@mbhave maybe what you're missing is the fact that getStatusCode() is overridden in sub-classes which fall back on the underlying server default. In short the situation is whenever the status code is not explicitly set.
Comment From: mbhave
ah, I see what you mean now. Thanks @rstoyanchev.
Comment From: rstoyanchev
Note that with https://github.com/spring-projects/spring-framework/issues/24400 ServerHttpResponse
now exposes getRawStatusCode()
so you can avoid the downcast to AbstractServerHttpResponse
which is now a deprecated method anyway.
Comment From: izeye
I created https://github.com/spring-projects/spring-boot/pull/19987 to apply the upstream change.