Skip to content

Commit

Permalink
Refine status KeyValue for HTTP server observations
Browse files Browse the repository at this point in the history
Prior to this commit, a cancelled exchange would always result in an
`"status":"UNKNOWN"` KeyValue. This only applied to reactive variants,
as cancelled exchanges are not currently detected for Servlet
implementations.

In some cases, exchanges can be cancelled by clients before they are
completed, but the response was actually received by the client. The
response status information has been set by the application and the
response has been committed. For those cases, we shouldn't assume an
"UNKNOWN" value.

This commit assumes that committed responses have a response status set
by the application and that the observations should reflect that. From
now on, we only assume an "UNKNOWN" status if the response has not been
commited.

Fixes gh-31388
  • Loading branch information
bclozel committed Oct 11, 2023
1 parent ee9dff3 commit e9fcb21
Show file tree
Hide file tree
Showing 2 changed files with 17 additions and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ protected KeyValue method(ServerRequestObservationContext context) {
}

protected KeyValue status(ServerRequestObservationContext context) {
if (context.isConnectionAborted()) {
if (context.isConnectionAborted() && (context.getResponse() == null || !context.getResponse().isCommitted())) {
return STATUS_UNKNOWN;
}
return (context.getResponse() != null && context.getResponse().getStatusCode() != null) ?
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2002-2022 the original author or authors.
* Copyright 2002-2023 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -147,6 +147,21 @@ void addsKeyValuesForCancelledExchange() {
.contains(KeyValue.of("http.url", "/test/resource"));
}

@Test
void addsKeyValuesForCancelledExchangeWhenResponseCommitted() {
ServerWebExchange exchange = MockServerWebExchange.from(MockServerHttpRequest.get("/test/resource"));
ServerRequestObservationContext context = new ServerRequestObservationContext(exchange.getRequest(), exchange.getResponse(), exchange.getAttributes());
context.setConnectionAborted(true);
exchange.getResponse().setRawStatusCode(404);
exchange.getResponse().setComplete().block();

assertThat(this.convention.getLowCardinalityKeyValues(context)).hasSize(5)
.contains(KeyValue.of("method", "GET"), KeyValue.of("uri", "NOT_FOUND"), KeyValue.of("status", "404"),
KeyValue.of("exception", "none"), KeyValue.of("outcome", "UNKNOWN"));
assertThat(this.convention.getHighCardinalityKeyValues(context)).hasSize(1)
.contains(KeyValue.of("http.url", "/test/resource"));
}

@Test
void supportsNullStatusCode() {
ServerWebExchange exchange = MockServerWebExchange.from(MockServerHttpRequest.get("/test/resource"));
Expand Down

0 comments on commit e9fcb21

Please sign in to comment.