search

addthis / metrics-reporter-config

Apache License 2.0
74 stars 52 forks source link

Implement statsd for both metrics2/metrics3 #19

Closed nickbp closed 8 years ago

nickbp commented 8 years ago

Includes a full set of tests and example configs.

The metrics2 version contains a workaround for an interface problem in the underlying reporter library. The workaround can be removed once https://github.com/ReadyTalk/metrics-statsd/pull/27 (or a similar solution) has been applied.

tea-dragon commented 8 years ago

Wow, this is a really beautiful pull request. Completely mimics the existing code patterns and styles, useful feature, unit tests.

If I had to pick one thing to comment on, it'd be the primary test class for the metrics 2 version. I'm not certain, but I couldn't find other tests that started up actual reporter instances (which are apparently unstoppable, and unreachable via this library anyway). Might be some dangling resources to look out for even if it won't be able to really send anywhere real.

Speaking of which, naming the hosts "test_host1" etc. sort of makes senses since iirc, underscores aren't legal in hostnames, but at the same time, if someone later adds validation to that class (not likely, but who knows), they'd have to fix up the test.

I don't know, neither really bothers me much. If it looks good to someone else, then let me be the first to say, "thank you, and I'm envious of such a tidy contribution".

nickbp commented 8 years ago

Thanks!

I've added a "stopForTests" call to the metrics2 version, similar to what the metrics3 tests use. So that should be getting correctly cleaned up now? The test hostnames have also been switched to use dashes instead of underscores.

nickbp commented 8 years ago

Hello, how's this PR look at this point?

yuesong commented 8 years ago

Hi Nicholas,

I'd change the public stopForTests methods in both Config class to package visible. Ideally we'd annotate them with guava's VisibleForTest annotation. But guava is test scope now - it's not worth blowing up the compile dependency just for that.

Everything else looks good otherwise. Thanks for your contribution!

Yuesong

nickbp commented 8 years ago

Thanks! Good point, I've made the two stopForTests() functions package-private.