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

Time unit mismatch in statsd metrics Observe() #608

Closed
esenac opened this issue Sep 14, 2017 · 4 comments
Closed

Time unit mismatch in statsd metrics Observe() #608

esenac opened this issue Sep 14, 2017 · 4 comments

Comments

@esenac
Copy link
Contributor

esenac commented Sep 14, 2017

Hi @peterbourgon!
We are using statsd for metrics, and it seems we found a mismatch in time units between metrics/statsd.Timingand metrics.Timer.
Timer's ObserveDuration method calculate the elapsed time in seconds and passes it to the underlying Histogram's Observe method, that in case of statsd.Timing requires the parameter in milliseconds.
It would be nice to have the chance to specify the time unit measure for ObserveDuration in Timer constructor or at least to be compliant with the underlying method.

@peterbourgon
Copy link
Member

peterbourgon commented Sep 14, 2017

metrics.Timer was not built to be compatible with statsd.Timing! But I would be happy to accept a PR that changed the unit on a metrics.Timer. So as not to needlessly break the API, I guess it should be implemented as a new method on the Timer struct, e.g. Unit(time.Duration), with an implied default of time.Second.

@esenac
Copy link
Contributor Author

esenac commented Sep 15, 2017

Opened PR #610!

@nicolaiskogheim
Copy link

This should be closed, right? :)

@esenac
Copy link
Contributor Author

esenac commented Sep 29, 2017

@nicolaiskogheim yep, thanks.

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