-
Notifications
You must be signed in to change notification settings - Fork 51
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Split out metriks into multiple gems to reduce dependencies #23
Comments
Great idea. On the surface I think it makes sense to have individual gems for each reporter. Then using service-specific gems like What about including the Logger reporter in core metriks? Maybe that becomes the base logger? My thought on upgrading would be to deprecate the current set of reporters when upgrading 1.0 (or 0.10) and remove them completely in 1.1 (or 0.11). That leaves the 0.9 line free for bug fixes. It'd be great to upgrade from 0.9 and have metriks continue to work as expected except with warnings. On the flip side, I can't say I would ever see these deprecation messages. I only use the Librato reporter on production so I could imagine upgrading to 1.0 and then 1.1 and only noticing when things broke hard. |
@lmarburger @eric I feel pretty strongly that for broadest adoption there should be a minimal set of functionality in the core
I think the |
I'm leaning towards the option of ripping out the reporters for the next release and having a big ...though I'm wondering if it would cause too many people to have code that breaks at startup. These sorts of decisions are hard. |
@eric the issue is more that people upgrading unawares will lose the reporting piece, so the broken part would be that the metrics stop flowing to their eventual destination? There shouldn't be any exceptions in their running code and as long as you rev the major version number, correctly packaged apps shouldn't be affected? Assuming the above is the case, semantic versioning is supposed to enable breaking changes like this. I don't see a way around it unless you make a new gem like |
The problem I see is if we remove the reporters from the If you had this in your rails initializers (as I do), bad things could happen. |
I would definitely split them out. I'd have all the core stuff in metriks and then have a gem for each reporter (metrics-reporter-liberator, metriks-reporter-log, etc.). I do this with most of my gems that are based on adapters (ie: adapter, adapter-mongo, adapter-redis, flipper, flipper-mongo). There is no reason to install the redis gem for adapter-mongo and vice versa. Same holds true. Just my two cents. I wouldn't have more than 1 gem for the core parts though. |
The other option would be to take the extracted portion and name that something new. Make that a dependency and allow metriks to continue to be a full-stack option without breakage for existing users. Either way my preference would be that the gem that includes the metric types and local storage not include anything having to do with reporters or logging. I think that mixes the concerns and makes the implementation less broadly useful since adoption then requires the user or integrator to agree with your choice/style of reporting/logging, etc. In my mind there is a pretty strict separation of concerns between local aggregation and reporting/transfer to another system/store. If there are concerns about being able to instrument this core gem I'd propose an abstract set of hooks that if unimplemented do nothing but allow gem users to sign up for instrumentation events, etc. |
FWIW I would also steer clear of the Seems like you could make |
Here's my first attempt at extracting a reporter gem: https://github.com/eric/metriks-graphite |
@eric I think that works. |
@josephruscio: I'm not sure if I quite agree in the longer term — there are a lot of things that ended up being repeated (and a lot of niceties in the librato-rails project related to forking, etc) that would be relevant for any kind of reporter. That said, until that stuff is standardized, I don't think there's a lot of value in having a base class just to have one. |
@eric I just saw your tweet wrt Monitorama and this ticket. I'm at Monitorama and would like to help with that. What's your plan regarding all the repositories? Do you want to create a metriks github organization where all metriks related repositores (metriks, metriksd, metriks-reporter-graphite, ...) live? I think that would be nice because it's easier to find the related libraries. |
I hadn't made any concrete decisions about things like github orgs, etc. I was waiting until I actually had a chance to do the legwork on the code first, but I could see it being a helpful thing to have. |
I started some of the legwork based on your metriks-graphite repository. |
In an attempt to try to increase the usage of metriks and make it easier to integrate, @josephruscio has suggested that the reporters be split into separate gems (or maybe their own gems).
This would mean the core metriks gem would just contain:
Timer
,Meter
, etc)Reporters
We would need to decide if it made sense to have a single
metriks-reporter
gem that contains all of the reporters, or if it would make sense to have separate gems for each reporter. The benefit of separate gems is the reduction in dependencies that would be required, which seems like a nice gain.Naming and upgrading
My first thought is to call the core gem
metriks
and have the reporters in their own gems. The downside of this is that anyone who is upgrading metriks will have their code break, as it will no longer contain any of the reporters. I'm not sure what the best way to solve this is a smooth upgrade path, so I'm open to suggestions here.CC: @lmarburger, @josephruscio, @nextmat
The text was updated successfully, but these errors were encountered: