Make Logbook interceptors fault tolerant

[marcindabrowski]

Since Logbook interceptors are not altering HTTP requests and responses they should do not throw Exceptions on they failure, because it could break HTTP connection.

Detailed Description

LogbookHttpRequestInterceptor.process method according to Apache HttpRequestInterceptor.process is allowed to throw HttpException and IOException, but actually it could throw any RuntimeExcpetion that happens during processing, ie.: https://github.com/zalando/logbook/issues/1693.

Context

With this change we will be sure that even if there will be some issues, for example after some libraries version bump, it will not affect my HTTP communication. Logbook is just logging, so if it fails, it should not break my HTTP communication.

Possible Implementation

This is example for LogbookHttpRequestInterceptor.process

try {
    LocalRequest request = new LocalRequest(httpRequest, entity);
    final ResponseProcessingStage stage = logbook.process(request).write();
    context.setAttribute(Attributes.STAGE, stage);
  } catch (HttpException | IOException exception) {
    throw exception;
  } catch (Exception ignored) {
  }
}

Of course it should be applied to all request and response interceptors.

Your Environment

• Version used: 3.6.0

[kasmarian]

@whiskeysierra @msdousti @ChristianLohmann what do you all think? I'm wondering why this wasn't the case all along for Logbook interceptors, as failures to log request/response shouldn't result in the whole request failure. On the other hand, I don't see this approach adopted in logbook and wonder if it's for a good reason.

[msdousti]

@kasmarian

Willi and I had a related discussion here: https://github.com/zalando/logbook/pull/1589#discussion_r1292238602

My two cents is that under no circumstances should a fault in Logbook result in a failure in the application.

[kasmarian]

@marcindabrowski now interceptors as well as DefaultLogbook will have the logic inside try-catch blocks, which should prevent failures of request/response processing and will only result in not logging them. Thanks for raising the issue. The fix will be part of the next release, I'll close the issue so that it's added to the release notes. But feel free to reopen it, if it's not fully addressed.