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

don't send None to statsd daemon as metric value #119

Merged
merged 2 commits into from
Mar 10, 2016

Conversation

dcrosta
Copy link
Contributor

@dcrosta dcrosta commented Mar 2, 2016

If you do, you'll get errors like this in /var/log/messages:

Metric value must be a number: metric.name, None

@yannmh yannmh added this to the 0.11.0 milestone Mar 2, 2016
@yannmh
Copy link
Member

yannmh commented Mar 2, 2016

Thanks @dcrosta for fixing this. I am adding your contribution to our 0.11.0 milestone.

@yannmh yannmh self-assigned this Mar 2, 2016
@dcrosta
Copy link
Contributor Author

dcrosta commented Mar 2, 2016

Cool. Is there a projected date for 0.11.0?

@yannmh
Copy link
Member

yannmh commented Mar 4, 2016

@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 master branch please ?

@dcrosta
Copy link
Contributor Author

dcrosta commented Mar 4, 2016

@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

Copy link
Member

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 ?

Copy link
Contributor Author

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.

@yannmh
Copy link
Member

yannmh commented Mar 9, 2016

Out of curiosity @dcrosta, could you share with us the use case that led to send None values via statsd ?

@dcrosta
Copy link
Contributor Author

dcrosta commented Mar 9, 2016

We have a value we pass to gauge() which is sometimes None -- our code doesn't (currently) check whether it's None before calling gauge(). Obviously we could fix this, but I thought it might make more sense to share this with others who might have the problem.

@yannmh
Copy link
Member

yannmh commented Mar 9, 2016

Thanks for your reactivity @dcrosta.

Let's merge your changes 🚢 ! (oops seems like an other rebase is needed)

@dcrosta
Copy link
Contributor Author

dcrosta commented Mar 9, 2016

OK, rebased again.

@yannmh
Copy link
Member

yannmh commented Mar 10, 2016

Thanks again !

yannmh added a commit that referenced this pull request Mar 10, 2016
[dogstatsd] discard `None` metric values
@yannmh yannmh merged commit f695789 into DataDog:master Mar 10, 2016
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