Add `ExceptionReportingServerErrorHandler` to log unhandled exceptions #4687

seonWKim · 2023-02-21T00:28:33Z

Motivation:

If LoggingService is not added to services, exceptions(without appropriate exception handlers) thrown by those services are not caught. This default behavior can be confusing for users because, even though exceptions are being thrown, there are no logs to indicate that anything is wrong.
The purpose of this feature is to enable the reporting of unhandled exceptions by default, so that users can be alerted to any issues and take appropriate action.

Modifications:

com.linecorp.armeria.defaultUnhandledExceptionsReportIntervalMillis vm option can be used to configure intervals between reporting unhandled exceptions in milliseconds.
Add shouldReportUnhandledExceptions to DefaultServiceRequestContext which determines whether to log unhandled exceptions.
Add ExceptionReportingServerErrorHandler to report unhandled exceptions.

Result:

Closes #<4324>. (If this resolves the issue.)
User will be able to check the number of uncaught exceptions and thrown exception periodically by default.
If multiple servers with same MeterRegistry exist, armeria.server.exceptions.unhandled metrics will be the sum of all the number of unhandled exceptions on those servers. However, individual servers will be reported with logs that include total number of unhandled exceptions specific to that server only.

Example

User can change default settings by using ServerBuilder

final ServerBuilder sb = Server.builder();
sb.http(port);
sb.unhandledExceptionsReportIntervalMillis(1000L);
configureServices(sb);
return sb.build();

Result

[armeria-common-worker-nio-2-1] WARN com.linecorp.armeria.server.ExceptionReportingServerErrorHandler - Observed 1 unhandled exceptions in last 1000ms(1000000000ns). Please consider adding the LoggingService decorator to get detailed error logs. One of the thrown exceptions:
example.armeria.server.annotated.ExceptionHandlerService$LocallySpecificException
	at example.armeria.server.annotated.ExceptionHandlerService.exception1(ExceptionHandlerService.java:36)
	at com.linecorp.armeria.internal.server.annotation.AnnotatedService.invoke(AnnotatedService.java:355)
	at com.linecorp.armeria.internal.server.annotation.AnnotatedService.lambda$serve0$8(AnnotatedService.java:324)
	at java.base/java.util.concurrent.CompletableFuture.uniApplyNow(CompletableFuture.java:684)
...

Closes #4324

…nSeconds` to `ServerConfig`

…ds` in `ServerBuilder`

trustin

Looks like we're going into the right direction. Just minor comments only!

core/src/main/java/com/linecorp/armeria/internal/server/DefaultServiceRequestContext.java

core/src/main/java/com/linecorp/armeria/server/DefaultServerConfig.java

core/src/main/java/com/linecorp/armeria/server/DefaultServerErrorHandler.java

core/src/main/java/com/linecorp/armeria/server/UncaughtExceptionsServerErrorHandler.java

core/src/test/java/com/linecorp/armeria/server/ServerBuilderTest.java

- use `exceptionReportingInterval` instead of `shouldLogUncaughtExceptions` and `logUncaughtExceptionsIntervalInSeconds` - remove `needsToWarn` - rename `UncaughtExceptionsServerErrorHandler` to `ExceptionReportingServerErrorHandler` - Add tests

seonWKim · 2023-02-22T13:42:03Z

@trustin Thank you for the review, I've applied the comments. 🙇‍♂️

seonWKim · 2023-02-23T00:17:57Z

I'm considering whether it's worthwhile to unify the terms log and report, since some methods and variables use log while others use report. For instance, there is method called logExceptions and some variables called shouldLogException, and there's a class named ExceptionReportingServerErrorHandler that's used for reporting exceptions.

core/src/main/java/com/linecorp/armeria/server/ExceptionReportingServerErrorHandler.java

core/src/main/java/com/linecorp/armeria/server/ServerBuilder.java

trustin · 2023-02-23T09:29:06Z

I'm considering whether it's worthwhile to unify the terms log and report, since some methods and variables use log while others use report. For instance, there is method called logExceptions and some variables called shouldLogException, and there's a class named ExceptionReportingServerErrorHandler that's used for reporting exceptions.

That's a good point, @seonwoo960000. How about this:

Rename {ServerBuilder,ServerConfig}.exceptionReportingInterval() to unloggedExceptionReportInterval()
Rename ServiceRequestContext.shouldLogException to shouldReportUnloggedException

…ingServerErrorHandler.java Co-authored-by: Trustin Lee <t@motd.kr>

Co-authored-by: Trustin Lee <t@motd.kr>

seonWKim · 2023-02-24T00:12:30Z

I'm considering whether it's worthwhile to unify the terms log and report, since some methods and variables use log while others use report. For instance, there is method called logExceptions and some variables called shouldLogException, and there's a class named ExceptionReportingServerErrorHandler that's used for reporting exceptions.

That's a good point, @seonwoo960000. How about this:

Rename {ServerBuilder,ServerConfig}.exceptionReportingInterval() to unloggedExceptionReportInterval()

Rename ServiceRequestContext.shouldLogException to shouldReportUnloggedException

Sure ~ I'll apply your comments. I'll also change some methods or javadocs that contain the word log to report if it's more appropriate.

Btw, I'll apply the renaming as follow. It's just about capital letters fix.

unloggedExceptionReportInterval -> unLoggedExceptionReportInterval
shouldReportUnloggedException -> shouldReportUnLoggedException

core/src/main/java/com/linecorp/armeria/internal/server/DefaultServiceRequestContext.java

core/src/main/java/com/linecorp/armeria/server/DefaultServerConfig.java

core/src/main/java/com/linecorp/armeria/server/ExceptionReportingServerErrorHandler.java

core/src/main/java/com/linecorp/armeria/server/Server.java

core/src/main/java/com/linecorp/armeria/server/ServerBuilder.java

core/src/main/java/com/linecorp/armeria/server/ServiceRequestContext.java

core/src/test/java/com/linecorp/armeria/server/ExceptionReportingServerErrorHandlerTest.java

- unLog -> unlog - stop all running servers in tests - use assert instead of throwing exceptions(on which arguments have already been checked)

trustin

I think we're getting close to completion now. Great! There's one ask left - Could you add Flags.defaultUnloggedExceptionReportIntervalMillis() and use it as a default value in ServerBuilder, so that a user can override the default without touching code?

core/src/main/java/com/linecorp/armeria/server/ServerBuilder.java

trustin · 2023-02-24T08:13:59Z

Test failure - https://github.com/line/armeria/actions/runs/4260354188/jobs/7413433723#step:7:1541 Seems trivial to fix 😉

seonWKim · 2023-03-08T23:30:23Z

@trustin I've fixed as below:

Add MicrometerCounter which counts the total number of unhandled exceptions on a single MeterRegistry. ExceptionReportingServerErrorHandler uses MicrometerUtil to register MicrometerCounter to MeterRegistry

core/src/test/java/com/linecorp/armeria/server/ExceptionReportingServerErrorHandlerTest.java

ikhoon · 2023-03-09T07:40:54Z

@seonwoo960000 Could you resolve the conflicts?

- `MicrometerCounter` is added which can be shared across multiple servers with same `MeterRegistry` - Singleton is removed from `ExceptionReportingServerErrorHandler`

seonWKim · 2023-03-09T12:45:34Z

@seonwoo960000 Could you resolve the conflicts?

Sure, I've resolved the conflict.

…nt multiple threads creating multiple schedules at the same time

core/src/main/java/com/linecorp/armeria/server/ExceptionReportingServerErrorHandler.java

…ing compareAndSet

core/src/main/java/com/linecorp/armeria/internal/server/DefaultServiceRequestContext.java

core/src/main/java/com/linecorp/armeria/server/ExceptionReportingServerErrorHandler.java

- change `log` position - use `Counter` for meterRegistry

trustin

Thanks for putting your effort on this, @seonwoo960000. LGTM 👍 🙇

ikhoon

Thanks, @seonwoo960000! 🎉 🎉

core/src/main/java/com/linecorp/armeria/common/FlagsProvider.java

core/src/main/java/com/linecorp/armeria/common/Flags.java

Co-authored-by: Ikhun Um <ih.pert@gmail.com>

jrhee17

Looks really nice! Thanks @seonwoo960000 🙇 👍 🚀

minwoox

Thanks a lot for your hard work!

minwoox · 2023-03-24T05:06:49Z

core/src/main/java/com/linecorp/armeria/server/ServerBuilder.java

@@ -1961,6 +1987,14 @@ private DefaultServerConfig buildServerConfig(List<ServerPort> serverPorts) {
            errorHandler = errorHandler.orElse(ServerErrorHandler.ofDefault());
        }

+        if (unhandledExceptionsReportInterval != Duration.ZERO) {
+            final ExceptionReportingServerErrorHandler reportingErrorHandler =
+                    new ExceptionReportingServerErrorHandler(meterRegistry, this.errorHandler,


This would remove the ServerErrorHandler.ofDefault(). 😄

Suggested change

new ExceptionReportingServerErrorHandler(meterRegistry, this.errorHandler,

new ExceptionReportingServerErrorHandler(meterRegistry, errorHandler,

I found a but that existed after fixing this. 😉
ad948d0

Thank you for your detailed review!

core/src/main/java/com/linecorp/armeria/server/ExceptionReportingServerErrorHandler.java

…ingServerErrorHandler.java

minwoox · 2023-03-27T02:13:07Z

Great job, @seonwoo960000! 🎉 🎉 🎉
Let's get to the next issue. 😆

seonWKim added 7 commits February 20, 2023 23:56

Add UncaughtExceptionServerErrorHandler to log uncaught exceptions

6ff8324

Add shouldLogUncaughtExceptions and `logUncaughtExceptionsIntervalI…

0b03e29

…nSeconds` to `ServerConfig`

Add shouldLogUncaughtExceptions to ServiceRequestContext

b226c29

Add logUncaughtExceptions and `logUncaughtExceptionsIntervalInSecon…

c988115

…ds` in `ServerBuilder`

Decorate errorHandler with UncaughtExceptionServerErrorHandler

73a2ab6

Extract methods and remove unnecessary logs

d82589e

Fix lint

153f890

seonWKim marked this pull request as ready for review February 21, 2023 01:37

seonWKim requested review from ikhoon, jrhee17, minwoox and trustin as code owners February 21, 2023 01:37

trustin reviewed Feb 22, 2023

View reviewed changes

Apply comments

330acff

- use `exceptionReportingInterval` instead of `shouldLogUncaughtExceptions` and `logUncaughtExceptionsIntervalInSeconds` - remove `needsToWarn` - rename `UncaughtExceptionsServerErrorHandler` to `ExceptionReportingServerErrorHandler` - Add tests

trustin reviewed Feb 23, 2023

View reviewed changes

seonWKim and others added 3 commits February 23, 2023 23:25

Update core/src/main/java/com/linecorp/armeria/server/ExceptionReport…

4a77d59

…ingServerErrorHandler.java Co-authored-by: Trustin Lee <t@motd.kr>

Update core/src/main/java/com/linecorp/armeria/server/ExceptionReport…

3c44fdf

…ingServerErrorHandler.java Co-authored-by: Trustin Lee <t@motd.kr>

Update core/src/main/java/com/linecorp/armeria/server/Server.java

596c627

Co-authored-by: Trustin Lee <t@motd.kr>

Apply comments

30d9059

seonWKim force-pushed the log-uncaught-exceptions branch from e96b775 to 30d9059 Compare February 24, 2023 01:56

trustin reviewed Feb 24, 2023

View reviewed changes

Apply comments

64e8fa9

- unLog -> unlog - stop all running servers in tests - use assert instead of throwing exceptions(on which arguments have already been checked)

trustin reviewed Feb 24, 2023

View reviewed changes

core/src/main/java/com/linecorp/armeria/server/ServerBuilder.java Outdated Show resolved Hide resolved

Fix default duration of unloggedExceptionReportInterval to 10 seconds

86cb7cc

Fix failing test

43095fb

ikhoon reviewed Mar 9, 2023

View reviewed changes

core/src/test/java/com/linecorp/armeria/server/ExceptionReportingServerErrorHandlerTest.java Outdated Show resolved Hide resolved

seonWKim added 2 commits March 9, 2023 17:28

Merge branch 'master' into log-uncaught-exceptions

7553945

Apply comments

65be9b4

- `MicrometerCounter` is added which can be shared across multiple servers with same `MeterRegistry` - Singleton is removed from `ExceptionReportingServerErrorHandler`

seonWKim force-pushed the log-uncaught-exceptions branch from 487204d to 65be9b4 Compare March 9, 2023 09:13

seonWKim added 2 commits March 10, 2023 15:21

Update reportingTaskFuture using AtomicIntegerFieldUpdater to preve…

f07ed65

…nt multiple threads creating multiple schedules at the same time

Merge branch 'master' into log-uncaught-exceptions

3916506

trustin reviewed Mar 10, 2023

View reviewed changes

core/src/main/java/com/linecorp/armeria/server/ExceptionReportingServerErrorHandler.java Outdated Show resolved Hide resolved

Add condition to check if reportingTaskFuture is null before execut…

382d12c

…ing compareAndSet

seonWKim force-pushed the log-uncaught-exceptions branch from c18e278 to 382d12c Compare March 10, 2023 06:49

trustin reviewed Mar 10, 2023

View reviewed changes

core/src/main/java/com/linecorp/armeria/internal/server/DefaultServiceRequestContext.java Outdated Show resolved Hide resolved

core/src/main/java/com/linecorp/armeria/server/ExceptionReportingServerErrorHandler.java Outdated Show resolved Hide resolved

Apply comments

13a9e05

- change `log` position - use `Counter` for meterRegistry

trustin approved these changes Mar 10, 2023

View reviewed changes

ikhoon approved these changes Mar 10, 2023

View reviewed changes

core/src/main/java/com/linecorp/armeria/common/FlagsProvider.java Outdated Show resolved Hide resolved

core/src/main/java/com/linecorp/armeria/common/Flags.java Outdated Show resolved Hide resolved

seonWKim and others added 3 commits March 11, 2023 01:07

Update core/src/main/java/com/linecorp/armeria/common/FlagsProvider.java

fb48c38

Co-authored-by: Ikhun Um <ih.pert@gmail.com>

Update core/src/main/java/com/linecorp/armeria/common/Flags.java

4cb1b96

Co-authored-by: Ikhun Um <ih.pert@gmail.com>

minor style fixes

65ddb36

jrhee17 approved these changes Mar 13, 2023

View reviewed changes

Fix a lint error

a326f09

minwoox approved these changes Mar 24, 2023

View reviewed changes

minwoox added 3 commits March 24, 2023 19:10

Update core/src/main/java/com/linecorp/armeria/server/ServerBuilder.java

4c91dbe

Update core/src/main/java/com/linecorp/armeria/server/ExceptionReport…

03624f2

…ingServerErrorHandler.java

Fix to call this instead of super

ad948d0

minwoox merged commit 5f6a3b4 into line:main Mar 27, 2023
8 of 10 checks passed

seonWKim deleted the log-uncaught-exceptions branch March 28, 2023 10:34

trustin mentioned this pull request Apr 9, 2024

Call UnloggedExceptionsReporter.report after calling `ServiceErrorH… #5587

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `ExceptionReportingServerErrorHandler` to log unhandled exceptions #4687

Add `ExceptionReportingServerErrorHandler` to log unhandled exceptions #4687

seonWKim commented Feb 21, 2023 •

edited by ikhoon

trustin left a comment

seonWKim commented Feb 22, 2023

seonWKim commented Feb 23, 2023 •

edited

trustin commented Feb 23, 2023

seonWKim commented Feb 24, 2023 •

edited

trustin left a comment

trustin commented Feb 24, 2023

seonWKim commented Mar 8, 2023

ikhoon commented Mar 9, 2023

seonWKim commented Mar 9, 2023

trustin left a comment

ikhoon left a comment

jrhee17 left a comment

minwoox left a comment

minwoox Mar 24, 2023

minwoox Mar 25, 2023

seonWKim Mar 25, 2023

minwoox commented Mar 27, 2023

	new ExceptionReportingServerErrorHandler(meterRegistry, this.errorHandler,
	new ExceptionReportingServerErrorHandler(meterRegistry, errorHandler,

Add ExceptionReportingServerErrorHandler to log unhandled exceptions #4687

Add ExceptionReportingServerErrorHandler to log unhandled exceptions #4687

Conversation

seonWKim commented Feb 21, 2023 • edited by ikhoon

trustin left a comment

Choose a reason for hiding this comment

seonWKim commented Feb 22, 2023

seonWKim commented Feb 23, 2023 • edited

trustin commented Feb 23, 2023

seonWKim commented Feb 24, 2023 • edited

trustin left a comment

Choose a reason for hiding this comment

trustin commented Feb 24, 2023

seonWKim commented Mar 8, 2023

ikhoon commented Mar 9, 2023

seonWKim commented Mar 9, 2023

trustin left a comment

Choose a reason for hiding this comment

ikhoon left a comment

Choose a reason for hiding this comment

jrhee17 left a comment

Choose a reason for hiding this comment

minwoox left a comment

Choose a reason for hiding this comment

minwoox Mar 24, 2023

Choose a reason for hiding this comment

minwoox Mar 25, 2023

Choose a reason for hiding this comment

seonWKim Mar 25, 2023

Choose a reason for hiding this comment

minwoox commented Mar 27, 2023

Add `ExceptionReportingServerErrorHandler` to log unhandled exceptions #4687

Add `ExceptionReportingServerErrorHandler` to log unhandled exceptions #4687

seonWKim commented Feb 21, 2023 •

edited by ikhoon

seonWKim commented Feb 23, 2023 •

edited

seonWKim commented Feb 24, 2023 •

edited