feat: generic HttpLoggingInterceptor #5037
Conversation
manusa requested review from oscerd, rohanKanojia, shawkins and sunix as code owners manusa force-pushed the fix/untyped-module-inference branch 3 times, most recently from 843660d to 67e02d3 Compare 
| @Override | ||
| public void consume(List<ByteBuffer> value, AsyncBody asyncBody) throws Exception { | ||
| // TODO: Should try to detect if the body is text or not? (HttpLoggingInterceptor.isPlainText) | ||
| value.stream().map(BufferUtil::copy).forEach(responseBody::add); // Potential leak |
There was a problem hiding this comment.
Yes doing something like a long running log watch, an http watch, etc. would indefinitely build up here. This will need to be bounded in some way - time, size, or both.
Signed-off-by: Marc Nuri <marc@marcnuri.com>
manusa force-pushed the fix/untyped-module-inference branch from 67e02d3 to 83dbdd4 Compare | Hi @shawkins, |
Checking now. The biggest open question is over the body logging. As long as it's bounded and has the binary detection, I think we're good. |
Signed-off-by: Marc Nuri <marc@marcnuri.com>
Signed-off-by: Marc Nuri <marc@marcnuri.com>
manusa force-pushed the fix/untyped-module-inference branch 2 times, most recently from 25368cf to a53ff64 Compare | @manusa a concern with the current pr is that it will catch exactly 1 response in the chain - depending upon the relative position of the logging interceptor. To catch all intermediate requests, for example backwards compatibility modifications, we need more notification of after so that each can be logged. One possible way of doing this is to expand the after calls (I realize this is different than how before is handled) - https://github.com/manusa/kubernetes-client/compare/fix/untyped-module-inference...shawkins:kubernetes-client:logging?expand=1 such a change would also help cut down on the state held by the logging interceptor |
Signed-off-by: Marc Nuri <marc@marcnuri.com>
Signed-off-by: Marc Nuri <marc@marcnuri.com>
Signed-off-by: Marc Nuri <marc@marcnuri.com>
Signed-off-by: Marc Nuri <marc@marcnuri.com>
Signed-off-by: Marc Nuri <marc@marcnuri.com>
manusa force-pushed the fix/untyped-module-inference branch from 53bbf55 to 40a9164 Compare 
| SonarCloud Quality Gate failed. |
manusa changed the title Signed-off-by: Marc Nuri <marc@marcnuri.com>
2 participants
Description
Relates to (should fix):
mvn oc:build -Dorg.slf4j.simpleLogger.defaultLogLevel=trace` eclipse-jkube/jkube#1950PoC to replace the OkHttp-specific
HttpLoggingInterceptorwith a Fabric8-specific implementation that works across all of ourHttpClients.The
HttpLoggingInterceptorimplementation is following a deferred approach. This means that the messages are only logged once the server responds and the response is completely processed by the client. This allows to log the request (and its body), the response, and the response body in a neat and grouped way. Other alternatives can be considered.The
Interceptorinterfaceaftermethod had to be further modified (on top of changes already provided in #5025), to allow to uniquely identify the original requests. HttpClients freely copy the original request to propagate it around which prevents keeping a consistent identifier (HttpRequests are copied either to propagate them, or to start new ones)Additional changes had to be completed in the WebSocket handling in order to allow to fully intercept and log the HTTP upgrade requests and responses. We should look into completely removing the
WebSocketResponsewrapper class in the future.Some questions/topics
I'll continue by implementing tests and addressing any comments once I get some feedback about this.
Type of change
test, version modification, documentation, etc.)
Checklist