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 post-timer labels (#1) #30

Merged
merged 4 commits into from
Sep 6, 2016
Merged

Add post-timer labels (#1) #30

merged 4 commits into from
Sep 6, 2016

Conversation

notkevin888
Copy link
Contributor

One common use case I've run into while using Prometheus at Tinder is having a label for success/failure on a single metric, like the following:

var promClient = require('prom-client');
var metric = promClient.metric('metric_name', 'metric_help', ['success_response']);

var timer = metric.startTimer();
someFunction((err, result) => {
  if (err) {
    timer({ success_response: 'ERROR' });
    console.log('uh-oh');
    // do error logic
  } else {
    timer({ success_response: 'SUCCESS' });
    console.log('yay!');
    // do success logic
  }
})

With the original code, it seemed like there wasn't a very clean way to handle this; the only way we had solved the problem before was by creating two timers (eg, a successTimer and an errorTimer) and only ever calling one of the timers later, but that seems both prone to error and not very memory-safe.

In addition, we might also want to do something with more than just 2 options, such as tracking average response times for every HTTP code we have in an application. In this case, the multiple-timer approach is also not very scalable, since we'd have to add an additional timer for each new code we wanted to send.

This was the approach I came up with; let me know if there's a better way of handling these kinds of use cases.

* add post-timer label support to summary

no reason to add a parameter into the first nested function, since that's only used in Summary.labels

* allow startLabels to be undefined

* add post-timer label support for gauges and histograms
@siimon
Copy link
Owner

siimon commented Sep 2, 2016

Great, thanks! Would you mind to add some tests for this and update the readme?

I won't be able to publish to npm until the middle of next week since I'm away from a computer - hope that's ok!

@notkevin888
Copy link
Contributor Author

Yeah, for sure :) Adding those in now!

* add gauge tests

* add histogram tests

* add summary tests

* add timer + label documentation to readme

* add comments
@notkevin888
Copy link
Contributor Author

Hey, hope you had a great weekend! I know it's still the beginning of the week; just wondering what the timeline looks like for this pr making it to npm :)

@siimon siimon merged commit de29e47 into siimon:master Sep 6, 2016
@siimon
Copy link
Owner

siimon commented Sep 6, 2016

Published with version 3.5.0! Thanks for your help!

@notkevin888
Copy link
Contributor Author

Sweet, thank you!

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.

2 participants