librato / statsd-librato-backend

A StatsD backend that sends metrics to Librato Metrics
MIT License
76 stars 34 forks source link

Enable to use librato statD plugin through http/https proxy #9

Closed geisbruch closed 11 years ago

geisbruch commented 12 years ago

Hi All, I've added the posibility to use the plugin through http/https proxy using the tunnel module for node.js. I am using my plugin version in production now and is running ok. (account arquitectura@mercadolibre.com)

mheffner commented 12 years ago

@geisbruch Thanks for the pull request. I'll take a look at this soon and get a version of this merged. I might look at making the tunnel include conditional based on the configuration.

geisbruch commented 12 years ago

Excelent, tell me if you have any question about the change.

caevyn commented 11 years ago

https://github.com/caevyn/statsd-librato-backend/commit/ba521eba9899895b685de9f7a534556a8864add2#lib/librato.js

I had to tweak the new config section a little for it to work. See the above link for my fork of geisbruch's fork.

mheffner commented 11 years ago

@geisbruch @caevyn Can you take a look at this PR. I made a slight change to the configuration format: https://github.com/librato/statsd-librato-backend/pull/14

geisbruch commented 11 years ago

It's sounds great, but if the proxy object has only one property maybe It's better set a proxy_url or some thing similar in the librato object to simplify the config. Example: { "librato" : { "proxy_uri" : "http://127.0.0.1:8080" } }

caevyn commented 11 years ago

Looks fine, I can't give it a test until next week but the conditional tunnel require looks like a good idea. I don't mind what the proxy config looks like, as long as it can be set easily. Hoping to start using librato in prod sooner than later:)

mheffner commented 11 years ago

@geisbruch I did the proxy config as a sub-section mainly in case we need to support some of the other proxy configuration variables in the future (proxy-specific auth variables, SSL settings/creds, etc.).

geisbruch commented 11 years ago

Perfect, so, could we merge this pull request?

mheffner commented 11 years ago

@geisbruch Done. I've merged PR #14