-
Notifications
You must be signed in to change notification settings - Fork 306
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
don't send None to statsd daemon as metric value #119
Conversation
Thanks @dcrosta for fixing this. I am adding your contribution to our 0.11.0 milestone. |
Cool. Is there a projected date for 0.11.0? |
@dcrosta, we are planning a release in about two weeks 🚢 . Additionally, I just noticed that some recent merges are conflicting with your branch: would you mind doing a quick rebase of your changes on top of the current state of the |
@yannmh ok, it's rebased -- waiting on Travis now. |
def test_histogram_doesnt_send_None(self): | ||
self.statsd.histogram('metric', None) | ||
assert self.recv() is None | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dcrosta what would you think about concatenating those tests to a single one ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I typically do one assertion per test -- then if the test fails, you know from the test name alone what the problem is. If you guys use a different style, I can definitely consolidate them, let me know what your preference is.
Out of curiosity @dcrosta, could you share with us the use case that led to send |
We have a value we pass to |
Thanks for your reactivity @dcrosta. Let's merge your changes 🚢 ! (oops seems like an other rebase is needed) |
OK, rebased again. |
Thanks again ! |
[dogstatsd] discard `None` metric values
If you do, you'll get errors like this in
/var/log/messages
: