Support for the extension - Add hello and flush routes and metric support #61
Conversation
maxday
changed the title
build.gradle
Outdated
| testImplementation 'com.github.stefanbirkner:system-rules:1.19.0' | ||
| testImplementation 'com.amazonaws:aws-java-sdk-kinesis:1.11.980' | ||
|
|
||
| // Use wiremock for stubing http calls |
There was a problem hiding this comment.
Nit: stubing -> stubbing
Also, do we need this comment at all?
| } | ||
| } | ||
|
|
||
| public static boolean isExtensionRuning(final String extensionPath) { |
There was a problem hiding this comment.
isExtensionRuning -> isExtensionRunning
| if(hitHelloRoute(AGENT_URL, HELLO_PATH)) { | ||
| shouldUseExtension = true; | ||
| } else { | ||
| DDLogger.getLoggerImpl().debug("Impossible to call the hello route"); |
There was a problem hiding this comment.
Impossible to call -> Could not call (throughout file)
"Impossible" would generally mean that there is no way this could ever happen, rather than there was an error this one time.
|
LGTM, I just left some spellcheck comments, you should get a review from someone who actually knows Java though |
agocs
left a comment
agocs
left a comment
There was a problem hiding this comment.
This looks good to me too. I think that hitFlush and hitHello should be protected, but the long rant in CustomMetricTest is a nit.
| return (f.exists() && !f.isDirectory()); | ||
| } | ||
|
|
||
| public static boolean hitHelloRoute(final String agentUrl, final String helloPath) { |
There was a problem hiding this comment.
It might be better to make hitHelloRoute and hitFlushRoute protected instead of public. That way they'll still be accessible from the tests, but not from the user's code.
I'm debating whether or not it makes sense to remove their arguments and have them access AGENT_URL and HELLO_PATH directly. I think you're doing it this way to support testing. I guess this is one of the best ways of going about that.
There was a problem hiding this comment.
💯 You're absolutely right, I've made all of them protected.
Yeah I've passed the values for unit testing so I can control the input.
| } | ||
|
|
||
| @Test public void testExtensionMetricWriter() { | ||
| Map<String, Object> map = new LinkedHashMap<>(); // to save the order to avoid flaky test |
| e.printStackTrace(); | ||
| } finally { | ||
| if (ds != null) { | ||
| ds.close(); |
| ddm.write(); | ||
|
|
||
| try { | ||
| Thread.sleep(1000); |
There was a problem hiding this comment.
This will probably work, but there's a chance any test that relies on something happening in one thread before a sleep in another expires has a chance of failure. I've had similar tests become suddenly flaky, but have only been able to make it fail by running tests in a docker container with .01 cpu.
It might be more stable to communicate the status of our DatagramSocket thread by setting the value of text[0]. E.g. (pseudocode)
final String[] text = new String[1];
text[0] = "not started"
new thread(new runnable() {
blahblah...
try {
text[0] = "nothing received"
ds = new DatagramSocket(8125);
ds.receive(dp);
text[0] = new String(dp.getData())
} catch .....
text[0] = "failure"
} finally {
.....
}
}
}
}.start())
while (true) {
if ( !text[0].charAt(0).equals("not started") && !text[0].charAt(0).equals("nothing received") && !text[0].charAt(0).equals("failure") ) {
// then we've actually gotten something
break;
// otherwise loop until the test times out
}
}
This also might be overly complicated, so feel free to disregard.
There was a problem hiding this comment.
Thanks for the feedback! I've adjusted the test
maxday
changed the title
What does this PR do?
Support the extension by :
sending metrics to the extension
adding the file detection + hello + flush route calls
heavy dependence on
finishfix a bug where metrics could have been sent even if disabled
Motivation
Start the extension support/tests without being blocked by the flush removal task.
Testing Guidelines
Deploy a function, be able to see logs for instance.
Additional Notes
Types of changes
Checklist