Conversation
simple cli application and test
| def conditions = new PollingConditions(timeout: INSTRUMENTATION_DELAY, initialDelay: SHUTDOWN_DELAY) | ||
|
|
||
| expect: | ||
| serverProcess.isAlive() |
There was a problem hiding this comment.
This is 'racy'. You may want to consider making your test app exist with some 'special' exit code on success and check for it.
There was a problem hiding this comment.
Agreed. A correct exit code covers both start and stop of the process.
isAlive() isn't available in JDK7 anyway.
|
|
||
| System.out.println("Going to shut down after " + delay + "seconds"); | ||
|
|
||
| Thread.sleep(delay * 1000); |
There was a problem hiding this comment.
You may want to have some loop with with some @Trace or something to make sure tracer actually gets triggered and all things are initialized.
There was a problem hiding this comment.
Testing that traces were written is not quite possible with the current setup of the smoke tests. That said, I added some more to the CliApplication so its closer to the original application that exposed the shutdown issue: making a request using URLConnection then quitting.
There was a problem hiding this comment.
Testing that traces were written is not quite possible with the current setup of the smoke tests.
You could potentially fire up a server and pass it as an agent url... but maybe not this time :)
|
If you comment out this line, does the test fail: https://github.com/DataDog/dd-trace-java/blob/master/dd-java-agent/agent-jmxfetch/src/main/java/datadog/trace/agent/jmxfetch/JMXFetch.java#L78 |
There was a problem hiding this comment.
Laplie great work on this. Just one comment: can we make sure that when JmxFetch is enabled the process shutsdown? What I observed is that I had to put some sleep (e.g. 5 seconds) in order to make sure that JmxFetch processes that were causing the app to hang were all fired up.
I don't think that there is easy to implement programmatic way to make sure that such processes have actually started, so heuristically sleep for 5 secs should be good.
Also, in your local tests (not in CI) might you make sure that rebasing your test on latest release (so not on master which includes the jmxfetch fix) the test actually fails?
Last but not least, please add the label dev/testing and assign this PR to the next milestone.
|
@tylerbenson Yes commenting out @labbati |
labbati
left a comment
labbati
left a comment
There was a problem hiding this comment.
Thanks @randomanderson for taking the time to do this extensive testing. It looks good to me 👍
| public void makeRequest() { | ||
| try { | ||
| final URL url = new URL("http://www.example.com/"); | ||
| final URLConnection connection = url.openConnection(); |
There was a problem hiding this comment.
I don't like making actual requests to external URLs for testing. How important is this?
There was a problem hiding this comment.
Not important at all. Originally, I just had a Thread.sleep(). @mar-kolya commented that the application wasn't doing enough so I added the request. The test doesn't rely on the success or failure of the request.
The example gist that first showed the JMXFetch issue had a request to google.com but I don't think was necessary to show the problem.
I'm fine removing the request and going back to only having a sleep.
There was a problem hiding this comment.
I think I would prefer that. Sorry for the back-and-forth. @mar-kolya you ok with that?
The test still "does something" in that it has a method with a @trace annotation. Doesn't need to call an external url
4 participants
This adds a smoke test that ensures CLI applications correctly exit when running under the java agent. There are 2 parts:
I also manually tested and ensured that the test failed when JMXFetch.daemon = false. This was the issue that prompted the creation of the test.