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

Support multiple metrics libraries #147

Closed
postwait opened this issue Aug 25, 2016 · 11 comments
Closed

Support multiple metrics libraries #147

postwait opened this issue Aug 25, 2016 · 11 comments
Milestone

Comments

@postwait
Copy link

Current the metrics subsystem doesn't support histogram reporting of request latencies. Can we make the metrics reporting interface such that the implementation can be changed out for "gometrics"-style recording that supports histograms?

I'd like to integrate circonus-gometrics, but layered above the go-metrics currently used, all good measurement data is destroyed.

@maier
Copy link
Contributor

maier commented Aug 26, 2016

just discussed a shim approach which sounds like it will solve the issue. if there is a metric interface in fabio itself then swapping out backends which want/support full raw measurement values will be easier.

@magiconair
Copy link
Contributor

@postwait and @maier I've moved the metrics library implementation behind an interface in the metrics/metricslib package where I've extracted the methods from the go-metrics library I'm currently using. This should make it possible to plug in any metrics library you want as long as it supports this interface. If the interface is not sufficient for what you are trying to do feel free to enhance it but keep it simple and generic if possible. Looking forward to the PRs :)

@magiconair magiconair changed the title Histogram metrics support Support multiple metrics libraries Aug 28, 2016
@magiconair
Copy link
Contributor

I think I missed a spot. The current implementation distinguishes between two different namespaces for metrics: the service metrics and the general fabio metrics (like requests). Service metrics get purged when the service or an instance disappears. This was implemented by using two different registries but that abstraction got lost with this change. I need to add that back in or make the purge code aware of what is a service metric and what isn't. Just keep that in mind when implementing your change.

@maier
Copy link
Contributor

maier commented Aug 29, 2016

Cool, thanks, I'm working off that branch to do an integration.

@magiconair
Copy link
Contributor

@maier My previous approach was too complex. The whole notion of the metrics package providing two registries was somewhat broken which made the testing more complex than needed to be. I've dropped the metricslib and the gometrics package and rolled this all into the metrics package. There you can now create multiple registries each with a single name space. The main.go then sets up the two name spaces for global and service metrics. That makes it cleaner and simpler IMO but you need to change something in your code. Sorry about that.

Supporting multiple different providers could then be implemented by adding a MultiRegistry analogous to the io.MultiWriter

@maier
Copy link
Contributor

maier commented Aug 29, 2016

I'll pull down the latest commit and check it out to see what the delta is between what I've got working and the updated method. Ok, I think this will still work. I'll reorganize what I have and kick the tires.

@magiconair
Copy link
Contributor

Merged to master.

@magiconair
Copy link
Contributor

@maier and @postwait please file follow-up tickets for issues with the circonus implementation. This is now on master. It would be great if you could test it and let me know whether it works.

@magiconair
Copy link
Contributor

@maier Thanks for your help with this.

@maier
Copy link
Contributor

maier commented Aug 30, 2016

I just pulled down master, rebuilt and tested. It works, I like the addition of the status code histograms.

@magiconair
Copy link
Contributor

I've tried this myself with a free Circonus account and can see it working as well. However, I notice a lag of a couple of seconds during startup which I suspect is the authentication with Circonus. I'll check whether I can hide this somewhere.

@magiconair magiconair added this to the 1.3 milestone Sep 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants