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>
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