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

Improving error handling #15

Open
xerial opened this issue Feb 25, 2014 · 15 comments
Open

Improving error handling #15

xerial opened this issue Feb 25, 2014 · 15 comments

Comments

@xerial
Copy link
Contributor

xerial commented Feb 25, 2014

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
Copy link
Contributor Author

xerial commented Feb 25, 2014

(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
Copy link
Contributor Author

xerial commented Feb 25, 2014

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

@muga
Copy link
Member

muga commented Feb 25, 2014

@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
Copy link
Member

oza commented Feb 25, 2014

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
Copy link
Contributor Author

xerial commented Feb 26, 2014

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
Copy link
Contributor Author

xerial commented Feb 26, 2014

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

@komamitsu
Copy link
Member

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
Copy link
Contributor Author

xerial commented Feb 28, 2014

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

@xerial
Copy link
Contributor Author

xerial commented Mar 1, 2014

+1 Plan1

@komamitsu
Copy link
Member

Plan1++

@muga
Copy link
Member

muga commented Mar 2, 2014

@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
Copy link
Contributor Author

xerial commented Mar 2, 2014

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

@komamitsu
Copy link
Member

@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
Copy link
Contributor Author

xerial commented Sep 14, 2014

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
Copy link
Member

@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 #32 (comment). What do you think?

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

No branches or pull requests

4 participants