interagent / pliny

An opinionated toolkit for writing excellent APIs in Ruby.
MIT License
802 stars 73 forks source link

Pluggable metrics backend #286

Closed appleton closed 7 years ago

appleton commented 7 years ago

This PR abstracts the metrics interface from the implementation allowing for different metric collection strategies. It also adds a new metrics backend for direct to Librato logging.

appleton commented 7 years ago

One big thing about this which I think we still need to address is that this implementation will just make all librato metric calls in-process and in the same thread. I have a feeling that's not going to work too well for us.

I was wondering if there's a way we can use Librato::Queue to gather metrics and then have that "submit" to a sidekiq job every so often for reporting.

appleton commented 7 years ago

TODO:

appleton commented 7 years ago

This should be ready for an initial review now. Still todo:

gudmundur commented 7 years ago

@brandur I'd love to get your eyes on this as well, as we discussed this at some point as well.

appleton commented 7 years ago

I've extracted a gem https://github.com/appleton/pliny-librato

gudmundur commented 7 years ago

@appleton @joshwlewis This stuff is awesome! 👍 Before bringing this in, I'd love to get @brandur's eyes on this as we discussed this at some point.

joshwlewis commented 7 years ago

I think that if you just upgraded your app's pliny gem without getting the initializer you'd just end up with a metrics module with an empty set of backends.

Yeah, that's certainly true. I think removing the initializer and defaulting the backends to [Pliny::Metrics::Backends::Logger] should work well for backwards compatibility, and still provide the flexibility we're looking for.

has the l2met style logging been determined to just be too heavyweight and so it's set to be dropped in favor of a more direct interface?

Yes, exactly. We'd like to be more explicit about what we send to our metrics services. Sending the entire log stream to metrics providers is a bit heavy-handed, and doesn't comply with our latest security model.

brandur commented 7 years ago

Yeah, that's certainly true. I think removing the initializer and defaulting the backends to [Pliny::Metrics::Backends::Logger] should well for backwards compatibility, and still provide the flexibility we're looking for.

Yeah, I think this is the right call (along with leaving in the initializer so that new projects get it). If we do another major version increment, we can always remove the default at that time (and replace a call to an empty set of backends with an error).

I left a minor comment on your patch, but +1.

Yes, exactly. We'd like to be more explicit about what we send to our metrics services. Sending the entire log stream to metrics providers is a bit heavy-handed, and doesn't comply with our latest security model.

Makes sense! I'm glad to hear about progress on that front. The l2met style logging was always an interesting idea, but was very obviously wasteful once you hit any kind of non-trivial scale.

I also like how this opens a few more doors: I'd strongly champion certain services that are full stop amazing (say a Mailgun or a Rollbar), but Librato isn't one of them. At Stripe we're now using DataDog after moving over from a self-managed Graphite installation, and it compares quite favorably to both our former setup and Librato. It's nice to know that after this patch lands, I'd easily be able to plug in a metrics driver for it.

joshwlewis commented 7 years ago

Cool, thanks @brandur. That's good feedback all around. I've updated this PR with everything mentioned.

I'd easily be able to plug in a metrics driver for it.

Yep, this is working nicely for heroku/pliny-librato. Not only can we control how and where metrics are reported, but how we schedule the API calls. We're just kicking off threads right now, but we could also queue them up and report in batches.

gudmundur commented 7 years ago

Awesome work @appleton and @joshwlewis!