[statsd] option to time in ms #78
Conversation
|
LGTM! ⏰ |
| self.statsd.timing(self.metric, time() - self.start, | ||
| self.tags, self.sample_rate) | ||
| elapsed = time() - self.start | ||
| use_ms = self.statsd.use_ms or self.use_ms |
There was a problem hiding this comment.
I'm unsure this really makes sense, someone could declare a statsd object with use_ms as True, then explicitly make a _TimedContextManagerDecorator with use_ms as False and this would resort to using milliseconds...is that expected?
Does this object even need this property or can it just pull it off its statsd object?
There was a problem hiding this comment.
Thanks @clokep, this is, indeed, unexpected.
I think having this property is worth: it makes convenient to switch specific timers from s to ms without altering the overall statsd configuration.
There was a problem hiding this comment.
I realized after writing this that if the decorator had it, then it had to be passed into here. I think the logic on this statement probably needs to be rethough (likely: if self.use_ms is not explicitly passed in, then default to self.statsd.use_ms).
`use_ms` option to report timed values in milliseconds instead of
seconds (default False).
examples:
```
from statsd import statsd
statsd.use_ms = True
statsd.timed('this.will.use.ms')
def foo():
# pass
from statsd import statsd
statsd.timed('this.will.use.seconds')
def foo():
# pass
statsd.timed('this.will.use.ms', use_ms=True):
def foo():
# pass
```
Fix #10, #67 (thanks @g--)
yannmh
force-pushed
the
yann/timers-in-ms
branch
from
617f7c9 to
7de341c
Compare
|
Thanks @clokep. I adressed the points you mentionned and rebased my work. @olivielpeau can you take a second look at it when you have a chance ? |
|
Indeed it's better now! LGTM |
use_msoption to report timed values in milliseconds instead ofseconds (default False).
examples:
Fix #10, and inspired by #10, #67
Thanks @g-- !