Skip to content

Support sources for increment and timing.#5

Closed
crito wants to merge 5 commits intogoodeggs:masterfrom
crito:sources
Closed

Support sources for increment and timing.#5
crito wants to merge 5 commits intogoodeggs:masterfrom
crito:sources

Conversation

@crito
Copy link
Contributor

@crito crito commented Apr 11, 2014

For my use case, I needed to support sources for librato metrics.I could only find a global configuration option to do so. I tried to be as non intrusive as possible. If you want to supply a source to your metric use the following format:

librato.increment "cpu_usage;web.production.11"
librato.timing "queues.timelag;worker.staging.2", 234.67

Everything after the ; will be regarded as the source. If no source is supplied its simply ignored. If the metric itself provides a source, than that takes precedence over the global configured one. This does what I need it for. Maybe its useful for others too.

@josephruscio
Copy link

@crito neat! I think this is similar to how our Statsd backend supports multiple sources, it's definitely a common use-case. @goodeggs let us know if there's anything we can do to help with the review this :-).

@mattinsler
Copy link

+1 for sources. potentially this should be closer to the librato-rails signature, which would be more like:

librato.increment('cpu_usage', {source: 'web.production.11'});
librato.timing('queues.timelag', 234.67, {source: 'worker.staging.2'});

@bobzoller
Copy link
Contributor

+1 to that syntax.

@crito @josephruscio I know it's been almost a year, so LMK if either of you do or don't plan to update this PR.

@crito
Copy link
Contributor Author

crito commented Feb 18, 2015

I have to say I like the terse syntax of the original pull request. I'm not a big fan of stuffing function arguments into objects, it feels rather bloated to me. But thats my personal opinion and taste.

How about making variadic functions out of increment and timing? So the source becomes optional. That would remove the cryptic me.tric;sou.rce syntax, but keeps the syntax a bit terser. Something like that:

librato.increment('cpu_usage', 'web.production.11')  # with source
librato.increment('cpu_usage')  # without a source

librato.timing('queues.timelag', 'worker.staging.2', 234.67)  # with source
librato.timing('queues.timelag', 234.67)  # without a source

@bobzoller
Copy link
Contributor

I still prefer the options hash style for clarity and parity with the ruby
gem. If your concern is terseness, you could write a wrapper function...

On Wed, Feb 18, 2015 at 2:04 PM, crito notifications@github.com wrote:

I have to say I like the terse syntax of the original pull request. I'm
not a big fan of stuffing function arguments into objects, it feels rather
bloated to me. But thats my personal opinion and taste.

How about making variadic functions out of increment and timing? So the
source becomes optional. That would remove the cryptic `me.tric;sou.rce"
syntax, but keep the syntax a bit terser. Something like that:

librato.increment('cpu_usage', 'web.production.11') # with source
librato.increment('cpu_usage') # without a source

librato.timing('queues.timelag', 'worker.staging.2', 234.67) # with source
librato.timing('queues.timelag', 234.67) # without a source


Reply to this email directly or view it on GitHub
#5 (comment).

@crito
Copy link
Contributor Author

crito commented Feb 18, 2015

Fair enough. If you'd like I could rework the original PR with the new syntax in the next days.

@bobzoller
Copy link
Contributor

That'd be awesome, thank you!

On Wed, Feb 18, 2015 at 2:55 PM, crito notifications@github.com wrote:

Fair enough. If you'd like I could rework the original PR with the new
syntax in the next days.


Reply to this email directly or view it on GitHub
#5 (comment).

@mattinsler
Copy link

+++!

@crito
Copy link
Contributor Author

crito commented Feb 21, 2015

A new PR can be found at #24.

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.

4 participants