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
More details (URL, method, body preview) in FeignClient(Server)Exception message #1048
Comments
I agree that the structure of To accomplish this, I think we'll need to make a few small updates:
Thoughts? |
O think it is a good idea to create log formatter to give clients a chance to decide how to write message to logs. Also Logger now has some unexpected behaviour #985 and I'd like to develop new version. |
@kdavisk6 @finnetrolle From my understanding, One note: As a developer using Feign, I'd be happy to be able to do these, independently:
This one is less likely, but it might happen. For example, when I use a DNS-load-balanced service, I want to log both the service hostname and the actual IP I connected to.
It is nice to have something by default - like what I wrote above. However, I usually have to customise that further, anyway. For example:
We cannot incorporate all above in Feign, so I would love some interface for this, as I usually need some of these. I cannot now think of a simple way to do that. I would probably extend Any thoughts? |
The Logging and exception handling go hand-in-hand when running any software at scale. These areas are very sensitive and highly specific. Given Feign's place as a middleware, we must do our best to remain neutral and provide ways for users to manage logging and exception handling, delegating that responsibility and not be too restrictive or too opinionated. The request here is quite simple, a better error message. However, given that this information will be consumed primarily via log files, we need to be very careful how to approach the structure of the message and we must provide a way to override our format. I'm suggesting that we approach this ask as follows:
|
Looks like we should add Response to FeignException first (or may be only to derived http-code-specific classes) and develop points then. I can take this task |
Hi @kdavisk6 @finnetrolle , Sorry, I was not familiar with the Could you point me to the right code? And, maybe could I see an example, how this would work eg. with SLF4J or some other logging API? So that I understand this better? Thanks. |
@jerzykrlk Also, now you can get a response from If you need to work with logs - read this first https://github.com/OpenFeign/feign#slf4j https://github.com/OpenFeign/feign#logging When you want to log in your own way you can just extend
|
Thank you for explaining. I have read the docs. I am not worried about request/response split here - adding some request details to an exception would fix that. And, I feel that in production, the overhead of adding request details to the exception would not be a big overhead - compared to building the stacktrace or executing the actual request. This might need to be measured, though. One more question. Do I understand it right that If yes, then I can see one thing here that could be a challenge. Right now, I get a
And, I have control of which exceptions get logged. Some exceptions are expected, like this:
or this:
Usually, I don't log all of them, as they're not errors. And only unexpected exceptions make it to the logger. Just as a result of the above business logic, with no extra effort. Do you think the above would be possible with |
@jerzykrlk If I got your question correct, I think you should use your own logger. Feign log some inner procedures. On the feignException level you gain request to get needed info and write it to log via SLF4J or else. Add private static logger |
Hi @finnetrolle I'm sorry for the delay. I wrote some code clarify things. Can you look at #1095 ? It is just an example. It just naively decodes the response body without any checks. But illustrates my idea. All information is just visible in the exception's And also, the information propagates nicely, when wrapped with other business-like exceptions. I'm aware that the approach with |
Can we make the FeignException message look like:
409 Conflict during [POST] to [http://localhost:8080/endpoint], response: [{"error":{"code":409,"message":"You already own this bucket. Please select another name."}}]
instead of just:
status 409 reading FeignClient#getResponse()
Reason:
Applications using Feign, communicate with multiple external systems.
As a developer, when I try to diagnose an issue in production, from the default exception message I cannot find out about:
I think that the new format would improve developer experience. I would like to see the complete information from the moment when the issue occurred: which external system returned an error, what endpoint and response body was. Just like that.
This would automatically appear in the logs, so I do not need to 1st - search through the stacktraces to see which request that was, 2nd - try to reproduce the issue to see the actual response body. Especially hard with non-deterministic errors.
There are other ways to do it, but I think the exception message is most natural. Other ways I know, involve extra error handling code all around the application. And the new format would be almost no changes to users' existing code base.
Actual (sample of current Feign logs):
Expected (what the log entry could look like):
I have implemented this multiple times already, in multiple projects. I think Feign would be a better place to do that.
Note - back in Feign 10.0.1, the message contained the complete body of the response (any size, even 1MB worked), which might cause problems. But I think the response can be abbreviated to just first 100 - 500 characters of the body.
This is basically the same idea as here: spring-projects/spring-framework#1956 . I can provide a pull request for this, if you like the general concept.
The text was updated successfully, but these errors were encountered: