MetricsWebFilter assigns UNKNOWN outcome to 200 OK responses

[hban]

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

[michaelmcfadyen]

I can confirm this behaviour is present in spring boot 2.2.2

[michaelmcfadyen]

The behaviour looks to be caused by the following method:

spring-boot/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/web/reactive/server/WebFluxTags.java

Line 145 in b15e427

if (response instanceof AbstractServerHttpResponse) {

There is a discrepancy between how the status code is resolved for the status code tag how it is resolved for the outcome tag.

[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

spring-boot/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/web/reactive/server/WebFluxTags.java

Lines 137 to 141 in bb3b6db

	public static Tag outcome(ServerWebExchange exchange) {
	Integer statusCode = extractStatusCode(exchange);
	Outcome outcome = (statusCode != null) ? Outcome.forStatus(statusCode) : Outcome.UNKNOWN;
	return outcome.asTag();
	}

@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?

[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;
}

[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!

[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?

[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.

[mbhave]

ah, I see what you mean now. Thanks @rstoyanchev.