Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Ganglia reporter #4

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants

strongh commented Mar 16, 2012

Added a dependency on the metrics-ganglia module and a report-to-ganglia clojure fn that enables a GangliaReporter.

Thanks for putting this together! I'm excited to be using metrics-clojure.

Owner

sjl commented Mar 19, 2012

Hey, yeah, I've been thinking about this (and Graphite, etc) but I'm not quite sure of the best way to handle it yet.

I don't just want to include all the optional dependencies in the base metrics-clojure library because if people don't need them it'll mean unnecessary dependencies.

One option is to create metrics-clojure-ganglia, metrics-clojure-graphite, etc wrappers, each of which is only a couple of lines long. That would work but seems a bit silly and a pain to maintain.

Another option is to have something like metrics-clojure-reporting that contains wrappers for all those reporting utils like metrics-ganglia and metrics-graphite. That means that if you're only using graphite you'd download a bit of extra stuff you don't need though.

Any thoughts?

strongh commented Mar 19, 2012

Gotcha. As a rule of thumb, I also avoid unnecessary dependencies, but I think this could be a case where the potentially unnecessary dependencies are worth the added convenience for users of the package. Together, metrics-ganglia and metrics-graphite are about ~20K - about 1/5 the size of metrics-core, small enough not to bother anybody. And it seems like there are (at least for now) only those 2 reporting modules outside of metrics-core.

I suggest putting reporting in metrics-clojure, and saving the modularization for instrumentation rather than reporting. This is consistent with how you've broken off metrics-clojure-ring, and also reflected by the majority of the metrics- modules being dedicated to instrumentation, not reporting.

Of your proposals, I prefer the second, that could work well. I also have a third option that's not too radical: rename metrics-clojure to metrics-clojure-core, split off metrics-clojure-reporting, and then make metrics-clojure equivalent to metrics-clojure-core + metrics-clojure-reporting. Sorta like how ring does it, I think.

Owner

sjl commented Apr 19, 2012

Gotcha. As a rule of thumb, I also avoid unnecessary dependencies, but I think
this could be a case where the potentially unnecessary dependencies are worth
the added convenience for users of the package. Together, metrics-ganglia
and metrics-graphite are about ~20K - about 1/5 the size of metrics-core,
small enough not to bother anybody. And it seems like there are (at least for
now) only those 2 reporting modules outside of metrics-core.

Yeah, the problem here is that I can see this growing in the future. For
example, someone makes a metrics-clojure-statsd reporter for reporting to statsd
and it requires some JSON parsing lib or something, so now metrics-clojure users
would get that JSON lib too.

This is what worries me about putting all the reporters into the main
metrics-clojure distribution by default. I think there's too many chances for
dependency problems, especially once more reporters are created that rely on
other libraries.

It definitely sucks. I think I need to let this roll around in my head a bit
more.

Collaborator

michaelklishin commented Jun 14, 2014

Thank you for submitting this. We will have more reporters added in 2.2 as separate sub-projects, and
I'll definitely use your code as an example!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment