-
Notifications
You must be signed in to change notification settings - Fork 142
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Java11 httpclient instrumentation #251
Conversation
I thought the unexpired token issue was resolved so the integration test was merged. The test caught the unexpired tokens are still an issue. The merged test has been added to the ait stop list. https://source.datanerd.us/java-agent/agent-integration-tests/pull/42 |
Early tests yielded positive results, NR trace.id is now being passed to the downstream entities. About a ~1/5 of the requests could be seen in the portal (10min delay). |
This is 🟢 for the AITs - https://javaagent-build.pdx.vm.datanerd.us/job/JavaAgent_PR_AIT/113/ But this 🟢 does not include the Java11 HttpClient specific AIT. That AIT is on the "stop list" because the instrumentation here has not been merged yet. However, I've tested this branch against the specific Java11 HttpClient AIT locally and it passes. |
9d0bf61
to
1f83b8f
Compare
1f83b8f
to
cde9435
Compare
|
||
public class InboundWrapper implements InboundHeaders { | ||
|
||
HttpResponse delegate; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this package private?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, will fix in update PR
import java.util.function.BiConsumer; | ||
import java.util.logging.Level; | ||
|
||
public class Util { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might not be a big deal, but we might want to qualify our "Util" classes with a semi-unique prefix so when searching in IntelliJ its easier to resolve. Once again, maybe not a big deal for our work but in other projects I've been on it was.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. What do you suggest here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once again, not suggesting you change now after the fact, but if you think its helpful in the future SomethingSpecificUtil
might make it easier to find and distinguish.
throws IOException, InterruptedException { | ||
URI uri = req.uri(); | ||
Segment segment = startSegment(uri); | ||
HttpResponse<T> response; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This block would be one of those opportunities we discussed where a private (maybe static) method might help document what it does real quick. 'tryToProcessResponse' perhaps is quicker to understand than scanning the code. Just a thought, not meant to be a criticism by ANY means.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think it would not work or be helpful here now that I look at more closely. The Weaver.callOriginal()
and segment stuff probably works better inline here rather than pulled out. I'll try to give a better example at some point.
} | ||
} | ||
|
||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does Javadoc even get generated on private methods? If not, you can probably boil this one down to a regular comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to know on the javadoc for private methods. I’ve never thought about it so I did some research and it seems there are good opinions on this. I'm going to keep it as is with the current comment style. There are other private method javadocs in that class and probably throughout the agent. Whether right or wrong, there appears to be precedent in the agent.
Happy to change it if the team decides this format is not preferred!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess my only follow up point here is if our javadoc generator does not run against private methods currently, there is little point in using the verbose Javadoc formatting here.
/*
Modify the java.net.http module so that it can read from the platform classloader's
unnamed module. The agent http client instrumentation utility classes are
in this specific unnamed module.
ModuleUtil is compiled in a multi-release jar. In Java < 11, this
results in a no-op implementation
*/
is probably sufficient without extra @param etc. that probably just clutters things up.
this seems to have broken scala on java 11 - see #438 |
This adds instrumentation for the Java 11 HttpClient, for #128
ISSUE: The transactions take a long time to report to NR because there is an unexpired token in every. transaction. That token takes a default 180 seconds to timeout, finishing the transaction.
https://staging.one.nr/01qwL4EArw5
The token is created in the process of CompletableFutures. The issue is being tracked here #344
Non-instrumentation changes:
getRuntimeMaxSupportedClassVersion
in WeaveUtils, increased the max supported version to Java 11.addReadUnnamedModuleToHttpModule
so java.net.http module can call into the unnamed module that holds the agent instrumentation utility classes. This required updating the module-utils dependency with this PR - add unnamed module to java.net.http for java 11 newrelic-module-util-java#13For the instrumentation:
send
orsendAsync
, under the hood, both call intosendAsync
on the HttpClientImpl. This is where the agent reports as external, using a segmentUnresolvedAddressException
instead ofUnknownHostException