Refactor report

[lmarburger]

Feeling things out for now. @eric, do you have any thoughts on adding #report to meters? It's spiked on Counter and Meter so far.

[lmarburger]

The regression test added to librato_metrics_reporter_test.rb will only pass on Ruby 1.9. It's only a temporary measure which will be replaced by a better test once complete.

[lmarburger]

@eric Have any opinions on the isolated LibratoMetrics tests? They're only separate for the time being. Some are really messy but I think that's a code smell coming from the reporter doing too much.

[eric]

I realized this code doesn't even need a hash. Would either of these be better than using a hash here?

  report_snapshot = snapshot
  block.call 'count', count
  block.call 'min', min
  block.call 'max', max

or

  report_snapshot = snapshot
  block['count', count]
  block['min', min]
  block['max', max]

[lmarburger]

Great idea. I like this a lot better than the unnecessary hash.

[lmarburger]

This would also make it easier for Metrics to be composed of other Metrics. The parent would call #each on its children passing block instead of merging all responses into a single hash.

On that note, is there a reason why things like Histogram and Meter keep their own counts instead of delegating to an internal Counter?

[lmarburger]

We're not doing anything special with block. Would this be cleaner?

yield 'count', count
yield 'min', min
yield 'max', max

[lmarburger]

Changed in b349e50

[eric]

Works for me.

[eric]

I think this is a great improvement.

Should we make sure we include more of the snapshot in the list, too?

[lmarburger]

Thanks for the feedback! Glad you approve of the direction. I wasn't sure how you'd take to all the mocking but I love testing against an abstraction instead of a concrete object. For example, I like that the :only/:except tests pass in a stub that responds to #=== and not separate tests for a string, regex, etc. I'm still not entirely comfortable with mocha but it's growing on me.

What do you mean by including more of the snapshot in the list?

[lmarburger]

What do you think of this compromise, @eric? :only and :except have two higher level tests using string matchers for clarity and a third expectation test. The later defines the protocol for the matchers and asserts the report is sending the right messages. If the implementation changes from sending #=== to sending another message, the expectation test will fail. That's by design because it means that change may break clients coded against the original protocol.

[eric]

Is there anything that is better by doing this vs just passing in strings directly?

[lmarburger]

The difference is writing an expectation to use #=== means it can't be changed to something like this without failing the mock test:

if matcher.is_a? Regexp
  matcher =~ gauge[:name]
else
  matcher == gauge[:name]
end

[eric]

I don't think it's useful to test the internal mechanics of how the reporter decides when to send stuff.

[lmarburger]

I agree. That TODO is a placeholder to remind me that I hate that test.

[travisbot]

This pull request fails (merged 069c4e6 into eb336ed).

[travisbot]

This pull request fails (merged 461ca3d into eb336ed).

[travisbot]

This pull request fails (merged 98bd87c into eb336ed).

[travisbot]

This pull request fails (merged 9b2f96f into eb336ed).

[lmarburger]

@eric OK most of the big stuff is done. I'd like to get better full-blown integration tests and then combine them all. There's still duplication and interface inconsistencies that could be addressed at some point.

[eric]

The more I've thought about this, the more I've realized that it could be helpful to use some sort of "adapter" classes that could take a reporter and yield, for instance, a flat namespace of metric names to values.

Another example adapter could report rate by storing and recording the difference in Meter#count instead of reporting the rate_per_second.

[lukaszx0]

Omg, started doing exactly the same refactoring as you are doing here without checking open PRs first and notice yours now, when I wanted to open mine. Bummer.

Let me know if I can help in any way with this 👍

[lukaszx0]

raise NotImplementedError maybe?

[lukaszx0]

@lmarburger This looks pretty great! Left few suggestions in comments.

[eric]

I've been re-thinking the idea of having the reporters in this gem at all.

Here's an example of the Librato Metrics reporter that has been extracted:

• https://github.com/eric/metriks-librato_metrics

[eric]

I'm not convinced there's a lot of common logic that is worth encapsulating into a reporter base class or helper class so far.

It's hard to standardize a lot of this, because the correct way to translate the metrics to be sent seems to be fairly different from one destination to the next.

[lukaszx0]

Actual sate yells for refactoring and having base reporter class. I've just implemented custom reporter and 90% of it is copypasted from librato metrics. The only difference are hash keys in prepare_metrics and custom send using different client. Whole write remained the same, not to mention start, stop and restart...

It's true and I agree that many different reporters/destinations will format data differently but I don't think it'll hurt them if the base class will provide some convenience methods which they can use if they want, if not - the base won't add any burden. In most cases though (graphite, librato, riemann) it'll be quite the same and the only difference will be at the end of the process which is actually sending metrics.

Regarding extracting reporters from this gem. Seems like a premature optimization and over kill for me 🙊 Those are only one-class implementations and we have only couple of them right now, the most demanded I'd say. Additonaly, I almost always use 3 of reporters at the time. The main one (graphite, librato, custom one) and proctitle on production and logger for development.

I'd personally love to see this refactor finished and merged to master. I also offer my help in finishing it up 😸

[eric]

The problem with having all of the reporters in this gem is that if a reporter has a dependency (like the library generally used to submit metrics to that service), it means that everyone who uses metriks will be pulling in that dependency, even if they don't use that reporter.

Over time, that becomes a huge pain (especially if a given reporter needs a gem that has a compiled dependency).

[eric]

So far, I've found with the reporters that I have written, that many of them actually need the metric type to be able to know how to act on it.

Having a simple Metriks::Service class that just deals with starting and stopping of threads seems fine to me if people think that's worth extracting.

[lukaszx0]

The problem with having all of the reporters in this gem is that if a reporter has a dependency (like the library generally used to submit metrics to that service), it means that everyone who uses metriks will be pulling in that dependency, even if they don't use that reporter.

There's a simple pattern used in many gems to overcome this:

begin
  require 'librato'
resuce LoadError
  raise "You need to include `librato` gem in your gemfile if you want to use `Metriks::Reporter::Librato`"
end

[eric]

I agree that that can solve the problem, but it feels like a pretty dirty solution to me. You now have to manually start explaining versioning dependencies and checking for them, etc. Yuck.