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

Implement http-client error log for non-CompletableFuture responses #10585

Closed
yawkat opened this issue Mar 7, 2024 Discussed in #10551 · 5 comments
Closed

Implement http-client error log for non-CompletableFuture responses #10585

yawkat opened this issue Mar 7, 2024 Discussed in #10551 · 5 comments
Labels
info: good first issue Good for newcomers type: improvement A minor improvement to an existing feature

Comments

@yawkat
Copy link
Member

yawkat commented Mar 7, 2024

Discussed in #10551

Originally posted by stefanos-kalantzis February 27, 2024
Case in point:

LOG.debug("Client [{}] received HTTP error response: {}", declaringType.getName(), t.getMessage(), t);

@yawkat yawkat added info: good first issue Good for newcomers type: improvement A minor improvement to an existing feature labels Mar 7, 2024
@meetpatel0963
Copy link

Hello, I am new to micronaut-core and I would like to work on this issue. Is it still open?

@yawkat
Copy link
Member Author

yawkat commented Mar 25, 2024

yes. go ahead.

@meetpatel0963
Copy link

meetpatel0963 commented Mar 26, 2024

Cool, Thanks! So, the http-client error log is already there for the case of COMPLETION_STAGE. And for the PUBLISHER case, the subscribers will be responsible to add the required logs in doOnError.

To add the similar log for the SYNCHRONOUS case, we can just wrap the code performing the blocking calls with try/catch, log the error and re-throw the exception to be handled by the outer catch block to centralize the error handling for all 3 cases.

Am I right? I am so sorry if the question sounds silly. Just want to make sure I understand the requirement correctly. Thank you in advance! 😄

@yawkat
Copy link
Member Author

yawkat commented Mar 26, 2024

that seems sensible for the sync case. I think it is also worthwhile to add the doOnError you mention on our own, just like it's implemented for CompletableFuture.

@meetpatel0963
Copy link

Thank you for the confirmation and the suggestion @yawkat. I'll get right to it.

yawkat pushed a commit that referenced this issue Nov 11, 2024
…onses (#11304)

* Add http-client error log for non-CompletableFuture responses

* Fix http-client error logging

* Refactor if conditional
@yawkat yawkat closed this as completed Nov 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
info: good first issue Good for newcomers type: improvement A minor improvement to an existing feature
Projects
No open projects
Status: No status
Development

No branches or pull requests

2 participants