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

[dogstatsd] Allow to dynamically use default route as a statsd host #134

Merged
merged 1 commit into from
May 26, 2016

Conversation

remh
Copy link

@remh remh commented May 23, 2016

When you run in a containerized environment, the dogstatsd server will
likely expose its port on the host network. If you have an application
that runs within a container, it needs to connect to it using the
default route.

@remh remh force-pushed the remh/allow_to_find_default_route branch 4 times, most recently from 429c363 to 218e6a1 Compare May 23, 2016 01:03
When you run in a containerized environment, the dogstatsd server will
likely expose its port on the host network. If you have an application
that runs within a container, it needs to connect to it using the
default route.
@remh remh force-pushed the remh/allow_to_find_default_route branch 2 times, most recently from 71f423a to 60eea4b Compare May 23, 2016 16:55
@yannmh yannmh added this to the 0.12.0 milestone May 23, 2016
@@ -79,6 +100,9 @@ def get_socket(self):
avoid bad thread race conditions.
"""
if not self.socket:
if self.use_default_route:
self.host = self._get_default_route()

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be a good candidate for a Python property too.

    @property.setter
    def use_default_route(self, value):
        self._use_default_route = value

       # Invalid the socket 
       self.socket = None

        if self._use_default_route:
            self.host = self._get_default_route()

with the following benefits:

  1. We don't need to repeat self.host = self._get_default_route() twice.
  2. The host and socket attributes would be systematically updated on use_default_route changes.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't do it for the host or port though. Should we ?

@yannmh
Copy link
Member

yannmh commented May 23, 2016

Looks good, I simply added a nitpick. Feel free to consider it or not, and merge the PR :)

@aeneaswiener
Copy link

aeneaswiener commented Jun 1, 2016

On Mac OS X this doesn't degrade gracefully:

>>> from datadog import initialize, statsd
>>> initialize(statsd_use_default_route=True)
>>> statsd.event("Test event", "Test body")
No handlers could be found for logger "dogstatsd"
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/env/lib/python2.7/site-packages/datadog/dogstatsd/base.py", line 363, in event
    self._send(string)
  File "/Users/env/lib/python2.7/site-packages/datadog/dogstatsd/base.py", line 299, in _send_to_server
    (self.socket or self.get_socket()).send(packet.encode(self.encoding))
  File "/Users/env/lib/python2.7/site-packages/datadog/dogstatsd/base.py", line 107, in get_socket
    sock.connect((self.host, self.port))
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/socket.py", line 228, in meth
    return getattr(self._sock,name)(*args)
TypeError: coercing to Unicode: need string or buffer, NoneType found

Is this expected / Is there a fix for this?

@yannmh
Copy link
Member

yannmh commented Jun 1, 2016

That's definitely not expected.

Thanks for reporting @aeneaswiener. We are working on a fix.

@remh
Copy link
Author

remh commented Jun 1, 2016

@aeneaswiener thanks for the feedback.

The main use case for this option is when you run your application in a (docker) container and you have your statsd server listening on the host. So for this use case, OS X support is not necessary.

Can you tell us a bit more about your use case ?

@aeneaswiener
Copy link

aeneaswiener commented Jun 1, 2016

@remh We run our Python applications locally during development, which includes running a test suite before building and pushing docker images. Of course we could introduce an if statement when initialising dogstatsd to initialise differently depending on the environment, but it would be nicer if the statsd instance would just fail silently if there is no route to dogstatsd.

@remh
Copy link
Author

remh commented Jun 1, 2016

@aeneaswiener not sure that failing silently is the best thing to do here. This is a bad practice that could hide underlying issues.

We could raise a better exception though if we can't find the default router, something like

raise IOError("Unable to find default route. This option is only supported on Linux")

Would that work ?

@aeneaswiener
Copy link

aeneaswiener commented Jun 2, 2016

That would work, yes. Though I am not sure if IO error is the best one to raise as it leaks an implementation detail of the API. In any case I think the behaviour (exception) should be the same as when an unreachable statsd_host parameter is passed to the datadog.initialize method.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants