Skip to content
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

Add go-metrics listeners for metric syndication #127

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

redhotpenguin
Copy link
Contributor

Work in progress. Looking for feedback on the work done so far.

Go-metrics provides a MetricSink interface which supports metrics
delivery to several different types of backends. These changes add a
set of listeners to veneur which use the MetricSink interface to
deliver the UDPMetric packets to the go-metrics backends for
syndication of the veneur metrics.

Summary

Added a listener package which uses the go-metrics package to syndicate UDPMetrics to different backends via a MetricSink interface. Created backend implementations for Circonus, Datadog (yeah this is redundant, but go-metrics supports it so why not), Prometheus, and Statsd listeners. There's a few other go-metrics backends that can be added later.

Initially I looked at using the plugin system in veneur, but ended up going with the listener approach so I had access to the UDPMetric data via a channel. There may be a better name for these workers than listeners, perhaps backends? Code has been golinted and hopefully conform with veneur style expectations.

Motivation

We use a few different monitoring systems at work, and being able to syndicate data to several of them via veneur agents would simplify our code and open up a lot of possibilities for comparison and analysis between those systems.

Test plan

Unit tests for the NewListener() constructor have been added. Probably a good idea to add tests for the Listen() function also, will require some mocking.

Have tested in a development environment with Statsd and Circonus backends and verified data syndication is working as expected. Need to verify that listener writes won't block HandleMetricPacket() at load and degrade performance. Need to test the Prometheus backend.

Rollout/monitoring/revert plan

Listeners must be specified in the configuration file, so veneur should behave normally if no listeners are configured.

@cory-stripe
Copy link
Contributor

Hey @redhotpenguin! This looks interesting! @aditya-stripe and I will take a look today. I'm not familiar with go-metrics so will need to do some sleuthing. 😄

@gphat
Copy link
Contributor

gphat commented Feb 3, 2017

Ok, maybe not today, but soon. 😸

@redhotpenguin
Copy link
Contributor Author

Sounds good, no rush 😄

@cory-stripe
Copy link
Contributor

Hey @redhotpenguin! Sorry for the delay, but I had some other projects to finish up on Friday!

So from what I can tell, this PR intends to leverage go-metrics' MetricsSink to give Veneur more backend sinks to write to? Is there some larger goal?

(I didn't really read it thoroughly yet, just investigated go-metrics!

@redhotpenguin
Copy link
Contributor Author

Yes, this PR uses the MetricsSink interface to syndicate the data to multiple backends. The larger goal there would be the ability to use additional tools on the data besides DataDog, such as some of the high resolution histogram based interfaces in Prometheus or Circonus. Or more unstructured tools like R; go-metrics provides an InmemSink backend that could export the raw metrics for analysis in R.

Let me know if that perspective helps - we have a bit of a potpourri of tools which inspired this work, which might not be for everyone. This feature would basically give developers, operators, and data scientists access to the same data rather than writing it to several different systems from the applications.

@cory-stripe
Copy link
Contributor

Thanks for the clarification. As you astutely pointed out in your opener, this required some manipulation on your part because our interfaces for it aren't great. I want to spend some time chatting with @aditya-stripe and the rest of the team to see if we can maybe make this easier or leverage your work as the opening salvo in an effort to make Veneur pluggable in this manner.

@ChimeraCoder
Copy link
Contributor

Thanks for submitting this @redhotpenguin!

This is in line with some work that we've been planning on doing, to make our metric interfaces better. Would you mind giving us write access to this feature branch (there's a option on the PR, or the repo itself)? That way we can keep this branch in sync with those changes, without having to block one on the other - and we can also take care of resolving the merge conflicts ourselves.

@redhotpenguin
Copy link
Contributor Author

Thanks @ChimeraCoder, I've added you and @cory-stripe as collaborators to my fork. I'm got a few tuits still to ensure these changes work well under load, if I have any additions I'll post a branch of this fork for review.

@kiyose
Copy link

kiyose commented Aug 14, 2017

This would be very handy for own environment but the PR hasn't been updated recently. What is the status of this?

Thank you

@cory-stripe
Copy link
Contributor

Good question! Sorry for the radio silence.

We've not integrated this change because we have plans to abstract the metrics flushing code to not be so dependent on Datadog. Basically, make Veneur agnostic about this work. We've gotten involved in other work, however, and this hasn't been a priority. We do intend to pull this in, but I have no ETA at present.

@redhotpenguin
Copy link
Contributor Author

Thanks for the update here; I have some tuits freeing up soon and can put some additional work into it on this direction if that helps!

@cory-stripe
Copy link
Contributor

It might not hurt, I just hate to have you poke at something if we internally go in another direction.

At present you can find some work in server.go related to setting up tracing as "sinks". The idea is that if LightStep or Datadog are configured as tracing targets then we'll add them as one of many sinks and dispatch to each one. The idea is that metrics will work the same way.

There's some hink tho, in that the in-memory metric is called a "DDMetric" and that stuff should likely be changed. But if you have some ideas for tweening us to that stage we'd certainly thoroughly appreciate any effort there. I'd encourage you to keep your PRs small jic. Again, I really want to avoid having our internal work conflict to as not to waste your time!

Go-metrics provides a MetricSink interface which supports metrics
delivery to several different types of backends. These changes add a
set of listeners to veneur which use the MetricSink interface to
deliver the UDPMetric packets to the go-metrics backends for
syndication of the veneur metrics.
@stripe-ci
Copy link

Gerald Rule: Copy Observability on Veneur and Unilog pull requests

cc @stripe/observability
cc @stripe/observability-stripe

@redhotpenguin
Copy link
Contributor Author

Thanks for the tips. I went ahead and rebased, and resolved a couple of conflicts in config.go and example.yaml. The current PR should be up to date with master.

@cory-stripe
Copy link
Contributor

Excellent, thanks @redhotpenguin. I started hacking on some changes to make this easier last night. We'll get there!

@kiyose
Copy link

kiyose commented Sep 18, 2017

How are things going for this PR?

Thanks!

@cory-stripe
Copy link
Contributor

Still making progress! We merged a few PRs last week that are continuing a refactor.

@kiyose
Copy link

kiyose commented Nov 2, 2017

Just checking back in. How's it going?

@cory-stripe
Copy link
Contributor

It's going! We're closer now with #292 merged. I've been on a different project this week and haven't been able to spend time unifying this, but you can expect PRs consolidating the various plugin/sink combinations over the next few weeks from which this PR will gain steam.

Again, sorry for the delay but we really wanted this to be the best it could be!

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@redhotpenguin
Copy link
Contributor Author

Just signed the CLA. Ready to merge! (Lol)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants