2
\$\begingroup\$

I have a redis caching server. If that caching server is down, my app will query the database directly.

I want to know from the logs if my caching server is not reachable. However, if I add a log error, it would completely spam my logfiles if a high amount of users would try to access data.

As a result, I would like to only log the very first error occurrence, resetting only if no errors occurred for an hour.

Is this a feasable approach at all? If so, please review my implementation:

public class CustomCacheErrorHandler implements CacheErrorHandler {

   private Logger logger = LoggerFactory.getLogger(this.getClass());
   private long errorOccurred;
   private final long ONEHOUR = 3600000;

   public void handleCacheGetError(RuntimeException exception, Cache cache, Object key) {
      if (errorOccurred < (System.currentTimeMillis() - ONEHOUR))
         logger.error("Error while getting cache " + cache.getName() + " for Key " + key);
      errorOccurred = System.currentTimeMillis()
   }
}

Using currentTimeMillis over System.nanoTime() and a final long ONEHOUR over TimeUnit.HOURS.toMillis(1) for performance.

Thanks in advance!

\$\endgroup\$
  • \$\begingroup\$ An alternate way of doing this is to count the number of identical errors, and then add a line "Repeats X times." after the first error message. This way the frequency of the error is still evident without having to fill a log file with redundant messages. \$\endgroup\$ – markspace Dec 27 '19 at 1:25
  • \$\begingroup\$ @markspace at the point "after the first error message" you cannot yet know "repeats X times". How exactly do you think this should work? \$\endgroup\$ – Roland Illig Dec 27 '19 at 7:33
  • \$\begingroup\$ @RolandIllig 1. Read message. 2. String compare with previous message. 3. If same increment counter. 4. If different and counter > 1, insert message "Repeats X times". It's not rocket science. I think there's already existing log parsers and other object that do this right now, look around. \$\endgroup\$ – markspace Dec 27 '19 at 16:36
1
\$\begingroup\$

Your code currently checks when the last error occurred. This might not be what you actually want. Assuming that an error occurs every 30 minutes, I would expect to have a log entry every 60 minutes. Your current code produces one log entry at the beginning, and no more log entries after that.

I once wrote similar code, and I separated the code into two Java classes: the Throttler for the actual algorithm and the ThrottledLogger for using the algorithm to throttle log messages.

I'm still happy with that old code, and it had the following additional features:

  • It allows several events before throttling kicks in.
  • When throttling kicks in, that is logged as well ("further log messages will be suppressed").
  • When throttling has finished, that is logged as well ("suppressed {} messages").

With these changes, the log messages do not hide any important information. The throttled logger I used was created like this:

new ThrottledLogger(
    logger,
    5,  // number of allowed messages
    1, TimeUnit.HOUR  // 1 more message every hour
);

With this information you should be able to write the same code as I did. Be sure to write some unit tests for the throttling algorithm, to demonstrate that it works indeed like you want it to work.

If you don't need this additional complexity, you should at least move the errorOccurred = assignment into the body of the if statement.

\$\endgroup\$
  • \$\begingroup\$ Thanks for input! The idea of using such a ThrottledLogged is interesting and surely has its place, in my case however I think it would not work, because the message limit would be already be consumed after the first second of my server being down. \$\endgroup\$ – CoffeeCups Dec 27 '19 at 0:58
  • \$\begingroup\$ The errorOccured must be outside the if statement, because the method handleCacheGetError is being executed only if the server is down, hence it will update only if the server is down:) \$\endgroup\$ – CoffeeCups Dec 27 '19 at 1:01
  • \$\begingroup\$ In that case, maybe you need 2 variables: errorOccurred and errorLogged. \$\endgroup\$ – Roland Illig Dec 27 '19 at 7:29

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.