WebClientTracerSubscriber.terminateSpan() throws IllegalArgumentException for non-standard status code

[blake-bauman]

When using Spring Cloud Sleuth with a WebClient call, and the downstream service responds with a non-standard HTTP code (say 499), then WebClientTracerSubscriber.terminateSpan() throws an IllegalArgumentException because the status code cannot be mapped to one of the HttpStatus enum values.

The offending code is here:

    void terminateSpan(@Nullable ClientResponse clientResponse,
                       @Nullable Throwable throwable) {
        if (clientResponse == null || clientResponse.statusCode() == null) {

From the Javadoc, ClientResponse.statusCode() will never return null, but will instead throw an IllegalArgumentException. As suggested by spring-projects/spring-framework#21289 clientResponse.getRawStatusCode() should be used if there's a possibility of a non-standard response code. Our app will call something like:

        webClient.get()
                 .uri(...)
                 .exchange()
                 .flatMap(response -> {
                     if (response.getRawStatusCode() == ...)
                         ...
                 })

However, this response handler never gets control because of the exception thrown when terminateSpan() calls clientResponse.statusCode(). When we remove spring-cloud-sleuth, it works fine, but since we would like the OpenTracing support, we still want to use sleuth.

[blake-bauman]

We are using the Greenwich.SR1 release train.

[marcingrzejszczak]

Hi @blake-bauman ! Are you interested in filing a PR with a failing test and the impl that fixes it?

[blake-bauman]

I wish, but at the moment, the Legal department at my company hasn't approved me for public open source code contributions. I'm going through the vetting process, but that can take months. I'm working on a simple project that demonstrates this.

Browsing through the code, however, it does appear that the problem is localized to this null check, the error validation, and the reason phrase in the exception. walking through handleReceive(), that part appears to be ok.

[codefromthecrypt]

ok so the fix for who can do this would be to try/catch and coerce IAE to null here, with a mock test with some invalid status from the server.. who knows, maybe 0? https://github.com/spring-cloud/spring-cloud-sleuth/blob/65ea711671b00dff591477f881bddbec3b13a287/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/TraceWebClientBeanPostProcessor.java#L400-L402

…

On Wed, Jul 3, 2019 at 2:54 AM blake-bauman ***@***.***> wrote: I wish, but at the moment, the Legal department at my company hasn't approved me for public open source code contributions. I'm going through the vetting process, but that can take months. I'm working on a simple project that demonstrates this. Browsing through the code, however, it does appear that the problem is localized to this null check, the error validation, and the reason phrase in the exception. walking through handleReceive(), that part appears to be ok. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#1382?email_source=notifications&email_token=AAAPVV3BIYW5W7HXKUI37HTP5OP5RA5CNFSM4H3LWQN2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZCHGEA#issuecomment-507802384>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAPVV4H4J2RJ67UOYXABC3P5OP5RANCNFSM4H3LWQNQ> .

[marcingrzejszczak]

Maybe they should be a part of Brave's API? Like statusCodeOrZero()?

[codefromthecrypt]

yeah this current code overrides statusCode, but better to override statusCodeAsInt instead. in any case catch the exception :P

[codefromthecrypt]

actually it can also throw IOE heh https://github.com/spring-projects/spring-framework/blob/master/spring-web/src/main/java/org/springframework/http/client/ClientHttpResponse.java#L46-L55

so something like

  @Override public int statusCodeAsInt(ClientHttpResponse response) {
    try {
      return response.getRawStatusCode();
    } catch (IOException | IllegalArgumentException dontCare) {
      return 0;
    }
  }

[blake-bauman]

I don't think this is going to solve the reported problem. It may solve some problem, but not the reported one. Unless terminateSpan() stops calling ClientResponse.getStatusCode(), the original problem will persist.

Btw, getRawStatusCode() won't throw an exception. It's getStatusCode() that'll throw it.

[marcingrzejszczak]

Now it should be better with 39d5e00

[blake-bauman]

Which snapshot will this be? 2.2.0.BUILD-SNAPSHOT?

[blake-bauman]

Using 2.2.0-BUILD-SNAPSHOT, issue seems to have moved to a different location. Should I open a new Issue to continue discussion? I have a sample project that demonstrates the issue.

[marcingrzejszczak]

Please do.