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

Add UncaughtExceptionsLogger to log uncaught service exceptions. #4680

Closed

Conversation

seonWKim
Copy link
Contributor

Motivation:

If LoggingService is not added to services, exceptions thrown by those services are not caught. This default behavior might be confusing to users because even though exceptions are thrown, users can't check the exceptions.
The purpose of this feature is to provide users with logging uncaught exceptions by default.

Modifications:

  • ServerConfig
    • logUncaughtExceptions is added to allow users select whether to log uncaught exceptions(true by default)
    • logUncaughtExceptionsIntervalInSeconds is added to allow users set the time interval between logging uncaught excptions
  • ServiceRequestContext
    • shouldLogUncaughtException is added to determine whether to log uncaught exceptions(true by default)
    • LoggingService sets shouldLogUncaughtException to false before serving the request.
  • Armeria server will schedule the logging of uncaught exceptions by default and unschedule it when being stopped.
  • UncaughtExceptionsLogger is added which tracks the number of uncaught exceptions and the last thrown exception.

Result:

  • Closes #<4324>. (If this resolves the issue.)
  • User will be able to check the number of uncaught exceptions and last thrown exception periodically by default.

Example

[armeria-common-worker-nio-2-1] WARN com.linecorp.armeria.server.logging.UncaughtExceptionsLogger - Observed 15 uncaught exceptions in last 60 seconds. If you don't see error logs please use LoggingService decorator.
[armeria-common-worker-nio-2-1] WARN com.linecorp.armeria.server.logging.UncaughtExceptionsLogger - Last exception occurred with following message: null
  • User can change default settings by using ServerBuilder
final ServerBuilder sb = Server.builder();
sb.http(port);
sb.logUncaughtExceptions(false); 
sb.logUncaughtExceptionsInterval(120); 

@CLAassistant
Copy link

CLAassistant commented Feb 19, 2023

CLA assistant check
All committers have signed the CLA.

@seonWKim seonWKim closed this Feb 20, 2023
@trustin
Copy link
Member

trustin commented Feb 20, 2023

@seonwoo960000 and I had some discussion about this PR and decided to improve it a little bit so it works for both non-annotated and annotated services. We're going to use ServerErrorHandler interface to implement this feature.

@seonWKim
Copy link
Contributor Author

@trustin Thank you for the support! 👍 👍

@seonWKim seonWKim deleted the log-uncaught-service-exceptions branch February 20, 2023 06:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants