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

Log the response cause that is raised after a request timed out #5150

Open
minwoox opened this issue Aug 28, 2023 · 4 comments
Open

Log the response cause that is raised after a request timed out #5150

minwoox opened this issue Aug 28, 2023 · 4 comments
Labels
improvement sprint Issues for OSS Sprint participants

Comments

@minwoox
Copy link
Member

minwoox commented Aug 28, 2023

Currently, an exception that is raised after a request is timed out on the server side is silently ignored.
Because the RequestLog is complete when the timeout response is sent to the client, there's no way to record the exception.

This makes user hard to diagnose the problem. The only exception that users would see is the RequestTimeouException instead of the actual exception that may be the cause of the timeout.
So, it would be nice if we could also log the exception so that users would use it for debugging.
One possible way to do this is using UnhandledExceptionsReporter here:

reqCtx.logBuilder().responseCause(cause);

@trustin
Copy link
Member

trustin commented Mar 28, 2024

Another option could be to modify RequestLog API to allow storing more than one request/response exceptions.

@coderclcl
Copy link

Let me try this one~! 🚀

@jrhee17 jrhee17 added the sprint Issues for OSS Sprint participants label Apr 1, 2024
@ikhoon
Copy link
Contributor

ikhoon commented Apr 3, 2024

Another option could be to modify RequestLog API to allow storing more than one request/response exceptions.

It would be nice if we also implemented a way to notify or log the late exceptions. My concern was that causes added after RequestLog is completed may not be logged by Logging{Client,Service}.

I propose the following idea:

  • Add RequestLogProperty.ADDITIONAL_RESPONSE_CAUSE is used to listen to an additional exception.
  • Provide an option in LoggingClient or LoggingService to log these additional exceptions even after a request is completed.
if (shouldLogAdditionalException) {
    ctx.log().whenAvailable(RequestLogProperty.ADDITIONAL_RESPONSE_CAUSE).thenApply(log -> {
        // log the cause
        logger.warn("A late response exception occurred after the response completes. ctx: {}",
                    ctx, log.additionalResponseCause());
    });

}

@jrhee17
Copy link
Contributor

jrhee17 commented Apr 4, 2024

I propose the following idea:

Add RequestLogProperty.ADDITIONAL_RESPONSE_CAUSE is used to listen to an additional exception.
Provide an option in LoggingClient or LoggingService to log these additional exceptions even after a request is completed.

This has already been discussed, but for the record: Another problem with the above approach is that the garbage collection timing of ctx may be ambiguous.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement sprint Issues for OSS Sprint participants
Projects
None yet
Development

No branches or pull requests

5 participants