Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WebClientExchangeTags does not handle non-standard status codes #17695

Closed
druppelt opened this issue Jul 29, 2019 · 4 comments
Closed

WebClientExchangeTags does not handle non-standard status codes #17695

druppelt opened this issue Jul 29, 2019 · 4 comments
Assignees
Labels
type: bug A general bug
Milestone

Comments

@druppelt
Copy link

druppelt commented Jul 29, 2019

Currently when calling a service that returns non-standard status codes like 490 with the reactive webclient, the tagging for the metrics actuator will cause an exception.
I'm using version 2.1.5.RELEASE of spring-boot-starter-actuator and spring-boot-starter-webflux. If I find the time, I'll create a pull request. For which branch should I do that?

In case I don't find the time, here's what I found so far:
The issue is that HttpStatus is used instead of a raw int. org.springframework.boot.actuate.metrics.web.reactive.client.WebClientExchangeTags#status(ClientResponse) itself only needs the raw value, but calls response.statusCode() which calls HttpStatus.valueOf(getRawStatusCode()) which throws an IllegalArgumentException.
At this point the solution is easy, just use response.rawStatusCode().

But after that it fails in WebClientExchangeTags#outcome(ClientResponse), and here I'm not sure what an elegant solution would be. Either directly use the rawStatusCode and ditch the comfort of the HttpStatus enum, or add an option to create HttpStatus.UNKNOWN values with the rawStatusCode as constructor argument I guess. I prefer the latter solution, but it may have side effects, as the Enum is part of spring-web and thus used a lot (681 usages according to my IDE).

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jul 29, 2019
@snicoll
Copy link
Member

snicoll commented Jul 30, 2019

@druppelt support of non-standard status code is a Spring Framework concern so I am wondering if we shouldn't move this one in spring-framework. Let's ask @poutsma first.

@rstoyanchev
Copy link
Contributor

@druppelt can you clarify what the 490 is for this service? One option is to add more enum values if they are common enough.

@druppelt
Copy link
Author

@rstoyanchev In my case 490 and 491 are API specific validation issues, so definitly not common.

From my understanding everything matching ^[1-5]\d{2}$ should be allowed as status code, though unknown codes may be mapped to x00, see https://greenbytes.de/tech/webdav/draft-ietf-httpbis-p2-semantics-25.html#rfc.section.6.p.2

For me it's totally fine if spring doesn't support non standard codes ootb, but there should be hooks available to add custom status codes. And something like metrics imho should try it's best to not cause exceptions.

@wilkinsona wilkinsona added type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged labels Aug 23, 2019
@wilkinsona wilkinsona changed the title Consider support for non-standard status codes in actuator metrics (or spring-web in general) WebClientExchangeTags does not handle non-standard status codes Aug 23, 2019
@wilkinsona wilkinsona added this to the 2.1.x milestone Aug 23, 2019
@wilkinsona
Copy link
Member

I don't think this should move to Framework as there are fixes to be made in Boot. We can work with the raw status from the ClientResponse and also use HttpStatus.Series when determining the outcome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug A general bug
Projects
None yet
Development

No branches or pull requests

5 participants