Improving error handling

[xerial]

I and @komamitsu san discussed how to improve error reporting of the current 0.2.x versions.

The major changes we considered are:

• Add setHandler(handler) method to FluentLogger to accept an application-specific error handler.
• In the error handler, provide a method for retrieving the remaining logs (the last one or all logs) that are not yet sent to fluentd.
• The last logs are message packed Event objects. We need to provide a decoder so that the last log is meaningful to the user.

In this change, we should consider the following problem:

• (Plan 1) A timing to report error. Currently errors can be reported in three ways: return value of log method (true or false), unmanaged exceptions or exceptions thrown when the buffer is full. If an error handler is added, log method should be non-blocking method, and the error must be handled in the user-defined error handler (in the subsequent code or in another thread). Does it the right choice?

Another option would be:

• (Plan 2) Making log a blocking method and reporting errors by Exception rather than returning true or false. The last log event should be included in the thrown exception.

After writing this ticket, Plan 2 now looks simpler to me. Any idea?

[xerial]

(Plan3) Catch all of the exception within log method, then return an Success or Failure object. Both objects define boolean isSuccess() and boolean isFailure() method, and when Failure provides last logs etc.

[xerial]

(Plan4) Returns true/false and when false, users retrieve lastUnsentLogs from the FluentLogger instance.

[muga]

@xerial

Thank you for your suggestions. Interesting. To customize fluent-logger behavior, users can extend to Sender class and implement it. But I think that your approach is better and more beautiful instead.

Plan 2 looks simpler and better to me now. Plan 1 is also interesting. But it takes a long time for considering the design and APIs and needs trial and error.

@komamitsu @oza

What do you think about his plans?

[oza]

Hi @xerial and @muga,

IMO, Plan 2 looks much simplest way to implement and use. I believe that it can be implemented straightforwardly. Is the error handler interface like this?

public interface ErrorHandler {
   void handleIOException(Event ev);
   void handleBufferFullException(Event ev);
   void handleUnexcpectedException(Event ev);
   ...
}

[xerial]

I continued the discussion with @komamitsu in Twitter. In his idea, handling checked exception with try and catch clause is cumbersome for the users. I agree with him in this point. If we adopt Exception based API, backward compatibility to 0.2.x will be lost and force the users to wrap their code with try and catch.

@oza Thanks. The default implementation of that interface would report error messages to a slf4j logger.

To make the problem clear, I am going to present several code examples of reporting system information to fluentd:

This part is common for all plans:

FluentLogger logger = FluentLogger.getLogger(...);

Plan1: Using ErrorHandler

final AtomicBoolean toContinue = new AtomicBoolean(true);
logger.setErrorHandler(new DefaultErrorHandler {    // Extending the default error handler
     @Override 
      public void handleIOException(IOException e, Event lastEvent) {
          toContinue.set(false);
     }
});

while(toContinue.get()) {
  // This can be a blocking or non-blocking method.  
   logger.log(tag, (system info data));   
   TimeUnit.SECONDS.sleep(1);
}

Plan2: Checked exception

boolean toContinue = true;
while(toContinue) {
    try {
       logger.log(tag, (system info data));
       TimeUnit.SECONDES.sleep(1);
    }
    catch(IOException e) {
        toContinue = false;
    }
    catch(OtherException e) {
       ....
    }
 }

Plan3: Returns rich Success/Failure object

boolean toContinue = true;
while(toContinue) {
   LoggingResult rep = logger.log(tag, (system info data));
   if(rep.isFailure) {
       rep.cause();        // Retrieve exception 
       rep.lastLog();      // Retrieve last log
       toContinue = false;
   } 
   TimeUnit.SECONDES.sleep(1);
}

Plan4: Returns true/false

boolean toContinue = true;
while(toContinue) {
   boolean success = logger.log(tag, (system info data));
   if(!success) {
       logger.lastError();        // Retrieve exception 
       logger.lastLog();      // Retrieve last log
       toContinue = false;
   }
   TimeUnit.SECONDES.sleep(1);
}

[xerial]

Wow, Plan1 (Using an error handler) makes the user code simplest. The other options are a little bit intimidating to the users.

[komamitsu]

Plan2 looks simple. But as mentioned earlier, if we adopt checked exception, the users need to change their code. If adopting unchecked exception, the users' applications can be troubled because of the unexpected exceptions that haven't occurred until now.

For plan3, actually I like this style (ideally speaking, I want to use pattern matching syntax...), but it changes the type of return value.

For plan4, it looks simple and doesn't force the user change their code. But it returns false value if log() fails, not only when the buffer is full, right? @xerial As the result, the users would be confused by the unexpected behavior.

Plan1 totally looks good to me. The casual users don't need to change their code because the default behavior is same as the current version. For those who want to know all errors that occurs in FluentLogger, I think it can satisfy their requirements.

[xerial]

@komamitsu Right. if plan4 returns false, its cause can be buffer full, send error, message pack error, etc.

[xerial]

+1 Plan1

[komamitsu]

Plan1++

[muga]

@xerial @komamitsu @oza

I agree with you. Plan1 allows user to customize error handling easier. I think that it's good to design and implement the feature in the next major version.

Next, to design error handling as @oza and @xerial said before, we should consider, design and implement exceptions (and errors) that fluent-logger can (and cannot) handle.

[xerial]

@muga Thanks. I and @komamitsu will take care of implementing a prototype of this error handling API.

[komamitsu]

@xerial I just sent a PR for this issue. Can you look at it when you have a time? In the PR, I added only ServerErrorHandler which deals with errors related to Fluentd since some of fluent-logger-java users told us that they need to know this sort of errors. On the other hand, I don't know if the other errors should be noticed via this kind of handler. I think we can add other handler such as MsgpackErrorHander if needed in the future.

[xerial]

Adding error handler looks good. Commented to #32. Key points include:

• Default error handling (logging to slf4j?)
• There might be another good name other than ServerErrorHandler since the meaning of Server is not clear in this context.

[komamitsu]

@xerial Thanks for the comments. I added additional changes according to your comment.

But I'm still wondering if we should prepare a default error handler since I have a concern about it https://github.com/fluent/fluent-logger-java/pull/32#discussion_r17524752. What do you think?