feat: log http response body in debug mode #95
Conversation
MayurGubrele
commented
Aug 23, 2021
•
MayurGubrele
commented
- Adding functionality to log HTTP response body when log_level=debug
- Classes affected: AbstractHttpSink.java, SerializableHttpResponse.java, SerializableHttpResponseTest.java HttpSinkTest.java, PromSinkTest.java
|
we should log the source message incase we receive no response from downstream. |
We do it when the response received is null |
| import java.util.List; | ||
| import java.util.stream.Collectors; | ||
|
|
||
| public class SerializableHttpResponse implements Serializable { |
There was a problem hiding this comment.
what is the purpose of this class?
we can just call the debug method, if debug level is enabled.
| @@ -85,6 +90,10 @@ private boolean shouldLogRequest(HttpResponse response) { | |||
| return response == null || getRequestLogStatusCodeRanges().containsKey(response.getStatusLine().getStatusCode()); | |||
| } | |||
|
|
|||
| private boolean shouldLogResponse(HttpResponse response) { | |||
| return response != null; | |||
There was a problem hiding this comment.
shouldn't this return true if the debug level is enabled?
There was a problem hiding this comment.
we can add a method in instrumentation if not already there.
and call that
if (instrumentation.isDebugEnabled()) {
instrumentation.logDebug("");
}
| @@ -48,6 +49,10 @@ public AbstractHttpSink(Instrumentation instrumentation, String sinkType, HttpCl | |||
| try { | |||
| response = httpClient.execute(httpRequest); | |||
| getInstrumentation().logInfo("Response Status: {}", statusCode(response)); | |||
| if (shouldLogResponse(response)) { | |||
| SerializableHttpResponse serializableHttpResponse = new SerializableHttpResponse(response); | |||
There was a problem hiding this comment.
I am a little sceptical of creating a new SerializableHttpResponse class for every non-empty response. I guess it won't matter much since it's in the local scope. But still, we can have a check, right ?
|
@MayurGubrele this branch has some conflicts right now. Can you please check and update things accordingly. Will merge post that. |
MayurGubrele
force-pushed
the
log-http-response
branch
from
017d027
to
f6ecc08
Compare
MayurGubrele
force-pushed
the
log-http-response
branch
from
f6ecc08
to
71994b4
Compare
There was a problem hiding this comment.
Let's check the instrumentation.isDebugEnabled() before calling the instrumentation.debug() as the parameter evaluation cost is relatively more.. It's a standard practice, please read it here.
https://logging.apache.org/log4j/1.2/manual.html#Performance
Let's remove the SerializableHttpResponse class entirely, It's not needed.
ravisuhag
changed the title
