Get CodaHale metrics working by starting up the stream consumers#1586
Get CodaHale metrics working by starting up the stream consumers#1586mattrjacobs merged 1 commit intoNetflix:masterfrom
Conversation
KevinJCross left a comment
KevinJCross left a comment There was a problem hiding this comment.
Suggestions to improve the tests to guard the quality of the code.
| Command command = new Command(); | ||
| command.execute(); | ||
| | ||
| Thread.sleep(1000); |
There was a problem hiding this comment.
surely you can use await here instead of sleep from https://github.com/awaitility/awaitility
such as
Gauge<Long> successGauge = metricRegistry.getGauges().get("test.test.countSuccess"); await().atMost(1, SECONDS).until(successGauge::getValue, isEqualTo(1L)); There was a problem hiding this comment.
I could, but then I introduce an extra dependency for this specific test. You could argue that a utility like this should be applied across all of the unit tests. My personal opinions is that the current state isn't enhanced by that much by a wholesale change to this style, but I'm happy to be convinced otherwise
| | ||
| Thread.sleep(1000); | ||
| | ||
| assertThat((Long) metricRegistry.getGauges().get("test.test.countSuccess").getValue(), is(1L)); |
There was a problem hiding this comment.
Hmm ... should this also test the other gauges ?
how about a counter from each stream ?
RollingCommandEventCounterStream
RollingCommandLatencyDistributionStream
RollingCommandUserLatencyDistributionStream
RollingCommandMaxConcurrencyStream
Which stream to timeouts happen on ?
There was a problem hiding this comment.
Yes, more test coverage here would be great. Would you like to submit a PR?
2 participants
Fix #1581
Issue was twofold: