1715 http client5 fixes #1719
1715 http client5 fixes #1719
Conversation
…this should resolve the NPE. Also added a case for the Message<HttpResponse, T> case. Updated tests
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1719 +/- ##
=============================================
- Coverage 70.73% 36.24% -34.50%
+ Complexity 9926 4865 -5061
=============================================
Files 828 828
Lines 39855 39855
Branches 6033 6033
=============================================
- Hits 28193 14444 -13749
- Misses 8931 23505 +14574
+ Partials 2731 1906 -825 ☔ View full report in Codecov by Sentry. |
| @@ -58,6 +58,7 @@ public static void handleUnknownHost(Exception e) { | |||
| } | |||
|
|
|||
| public static void processResponse(URI requestURI, HttpResponse response, Segment segment) { | |||
| if (response == null) return; | |||
There was a problem hiding this comment.
Do you know what triggers the response to be null?
Does it still make sense to create the external in that case?
The HttpParameters could be created calling noInboudHeaders instead of inboundHeaders(Response).
There was a problem hiding this comment.
Good question, I'm not sure what causes that. What are your thoughts on creating an external anyway?
| HttpResponse resp2 = (HttpResponse)(((Message)response).getHead()); | ||
| InstrumentationUtils.processResponse(request.getUri(), resp2, segment); | ||
| } else { | ||
| AgentBridge.getAgent().getLogger().log(Level.FINER, "Unhandled response type: "+response.getClass()); |
There was a problem hiding this comment.
This creates a StringBuilder even if the message is not going to be logged.
Could you use:
AgentBridge...log(Level.FINER, "Unhandled response type: {0}", response.getClass());
or guard with a check to whether FINER level is enabled.
And regardless, if response is null here, it will throw a NPE.
| private void testError(boolean async) throws Exception { | ||
| @Test | ||
| public void testErrorAsyncWithMessage() throws Exception { | ||
| testError(true, false); |
There was a problem hiding this comment.
shouldn't this be true, true?
instanceof checks when handling the response
and another null check