feat: additional callback methods for Interceptor interface

[manusa]

Description

Relates to:

• Build going into inifinite loop on running mvn oc:build -Dorg.slf4j.simpleLogger.defaultLogLevel=trace` eclipse-jkube/jkube#1950
• Build fails with "-Dorg.slf4j.simpleLogger.defaultLogLevel=trace" option eclipse-jkube/jkube#2000

Added two more methods to the Interceptor interface. The main intention is to provide enough functionality to implement an HttpLoggingInterceptor compatible with every HttpClient implementation.

The provided infrastructure will allow to implement (in a subsequent PR) an HttpLoggingInterceptor to replace the OkHttp-specific intereceptor which is also flawed.

@shawkins thoughts? As far as I can see, these changes will enable the HttpLoggingInterceptor that can be activated based on the configured logging-level. If this one's OK, I'll send the interceptor implementation after merging.

Type of change

• Bug fix (non-breaking change which fixes an issue)
• Feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change
• Chore (non-breaking change which doesn't affect codebase;
  test, version modification, documentation, etc.)

Checklist

• Code contributed by me aligns with current project license: Apache 2.0
• I Added CHANGELOG entry regarding this change
• I have implemented unit tests to cover my changes
• I have added/updated the javadocs and other documentation accordingly
• No new bugs, code smells, etc. in SonarCloud report
• I tested my code in Kubernetes
• I tested my code in OpenShift

[sonarqubecloud]

SonarCloud Quality Gate failed.   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

28.6% Coverage
0.0% Duplication

[shawkins]

thoughts?

I'm presuming that you'll want this interceptor to be last in the chain, and that we'll log just based upon the after method - using the request from the response. This is a probably close enough to the logging users will want to see.

From our perspective correlating to the incoming user level request prior to the before modifications may be interesting, but has been made hard to do because no call spans both and we don't have unique request identifiers. This also won't necessarily correlate intermediate requests by the interceptors - failed backwards compatibility and OpenShiftOAuthInterceptor token refreshes will be logged as separate requests.

As far as I can see, these changes will enable the HttpLoggingInterceptor that can be activated based on the configured logging-level. If this one's OK, I'll send the interceptor implementation after merging.

Are you also looking to log the requests by the TokenRefreshInteceptor?

[manusa]

Are you also looking to log the requests by the TokenRefreshInteceptor?
OpenShiftOAuthInterceptor, and so on

I assume that users want the complete log of the HTTP conversation regardless of its origin (client request, interceptor, etc). So I guess we do want to log these requests too.

TBH I didn't actually consider this scenario, but as far as composition looks, those requests should be logged too.

I'm presuming that you'll want this interceptor to be last in the chain, and that we'll log just based upon the after method - using the request from the response

I'm not sure if we actually want this interceptor to be applied first instead. Anyway, the interceptor should be internal and added by us in a similar way as we are currently adding the OkHttp interceptor. So ensuring it's the first or last in the chain should be easy.

The interceptor should log same as OkHttp (I haven't checked but I assume):

1. Request URI
2. Request Headers
3. Request Body
4. Response Status
5. Response Headers
6. Response Body

So probably 1-5 can be logged in the after method call, and the response body by the encapsulated consumer.

[shawkins]

TBH I didn't actually consider this scenario, but as far as composition looks, those requests should be logged too.

The TokenRefreshInteceptor creates a new client instance, which is not based upon the Configuration - it uses the plain HttpClient.Factory.newBuilder() method.

and the response body by the encapsulated consumer

I'm unsure how easily this will be applied. Proxying the consumer will give us access to the bytes, but not the broader cycle - without additional changes you'd have to log the bytes as they are received because there is no separate notification to the consumer that we're done.

[manusa]

The TokenRefreshInteceptor problem could be tackled by applying the LoggingInterceptor at the HttpClient layer.

It would certainly be much neater to log in a continuous contextual way. I recall you mentioned some time ago that we already did add some improvements that'd allow to get better callbacks of the HTTP request/response processing events (start/.../end). I also see that we could maybe somehow leverage the HttpClientReadableByteChannel.onBodyDone for this purpose.

Did you have something in mind?

It might be more interesting to make the Interceptor.after execute only after the response was completely processed (including the body processing).

[shawkins]

The TokenRefreshInteceptor problem could be tackled by applying the LoggingInterceptor at the HttpClient layer.

Sure, it just becomes less customizable that way.

Did you have something in mind?

There's no single great solution. We'd have to look at content type and length to have some understanding of what's in the response - otherwise we can't easily tell a difference at this layer between binary and test or log streaming (or other large / long running download), and just a more typical response. We'll probably have to buffer that up to a given size, then log in chunks if it looks like it's text. Yes something like onBodyDone could work:

         @Override
          public AsyncBody.Consumer<List<ByteBuffer>> consumer(AsyncBody.Consumer<List<ByteBuffer>> consumer,
              HttpRequest request) {
            AtomicBoolean added = new AtomicBoolean();
            // some other state for buffering / length

            BiConsumer<Void, Throwable> onBodyDone = (v, t) -> {
              // some logging
            };

            return (value, asyncBody) -> {
              if (!added.compareAndSet(false, true)) {
               asyncBody.done().whenComplete(onBodyDone);
              }
              // buffer the bytes if text, keep a count if binary
              // log once the buffer is full
              consumer.consume(value, asyncBody);
            };
          }

[manusa]

OK, so shall I merge this and we can discuss the logging approach once I provide the PoC?

[shawkins]

OK, so shall I merge this and we can discuss the logging approach once I provide the PoC?

Sure, that's fine.