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

Graphite DurationUnit has confusing semantics #74

Closed
pteichman opened this issue Sep 10, 2014 · 2 comments · Fixed by #84
Closed

Graphite DurationUnit has confusing semantics #74

pteichman opened this issue Sep 10, 2014 · 2 comments · Fixed by #84

Comments

@pteichman
Copy link
Contributor

I'm attempting to report timers to Graphite using millisecond units. With DurationUnit: time.Nanosecond (the default), they're off by a factor of a million. With DurationUnit: time.Millisecond, they're off by a trillion.

Is this a problem of documentation (there aren't any DurationUnit examples) or should all the timer metrics be t.Foo()/du rather than du*t.Foo()? I'm happy to make the patch in either case.

@pteichman
Copy link
Contributor Author

I worked around this by compressing time in my calls to Timer.Update, e.g. DurationUnit: time.Nanosecond and:

-               stats.MsgTimer.Update(time.Since(start))
+               stats.MsgTimer.Update(time.Since(start) / time.Millisecond)

Am I misunderstanding the point of DurationUnit? I'm finding it hard to imagine a scenario when you'd need to log nanoseconds as a larger magnitude, which is the only option with the current code.

@rcrowley
Copy link
Owner

rcrowley commented Oct 5, 2014

Looks like a bug, thanks.

pteichman pushed a commit to pteichman/go-metrics that referenced this issue Oct 14, 2014
This fixes Timer reporting with DurationUnits other than
the default time.Nanosecond.

Fixes rcrowley#74
rcrowley added a commit that referenced this issue Nov 8, 2014
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 a pull request may close this issue.

2 participants