Add DD_DOGSTATSD_URL support for unix and udp urls#870
Add DD_DOGSTATSD_URL support for unix and udp urls#870
Conversation
7a4ec25 to
37f0173
Compare
37f0173 to
71daae4
Compare
|
This issue has been automatically marked as stale because it has not had activity in the last 30 days. |
71daae4 to
9bdac11
Compare
9bdac11 to
ac18e9b
Compare
Adds support for the unix:// and udp:// variants of DD_DOGSTATSD_URL. Will only be applied if the host and port are their default values and no socket_path has been provided.
ac18e9b to
ed1bc9b
Compare
|
|
||
| if ( | ||
| host == DEFAULT_HOST | ||
| and port == DEFAULT_PORT |
There was a problem hiding this comment.
Would it make sense if an explict request for defaults Dogstatsd(host='localhost', port=8125) takes precedence over environment variables?
| log.debug( | ||
| "Unable to parse DD_DOGSTATSD_URL, did you remember to prefix the url " | ||
| "with 'unix://' or 'udp://'? Falling back to alternate " | ||
| "connection identifiers." | ||
| ) |
There was a problem hiding this comment.
Would it make sense be more specific in the error message? Probably worth a higher log level too.
| log.debug( | |
| "Unable to parse DD_DOGSTATSD_URL, did you remember to prefix the url " | |
| "with 'unix://' or 'udp://'? Falling back to alternate " | |
| "connection identifiers." | |
| ) | |
| log.warning("D_DOGSTATSD_URL value %r had unsupported scheme %r, must be one of 'unix://', 'udp://'", parsed.scheme) |
| "Port number provided in DD_DOGSTATSD_PORT env var is not an integer: \ | ||
| %s, using %s as port number", |
There was a problem hiding this comment.
| "Port number provided in DD_DOGSTATSD_PORT env var is not an integer: \ | |
| %s, using %s as port number", | |
| "DD_DOGSTATSD_PORT value %r is not an integer, falling back to %d", |
| except ValueError: | ||
| log.debug("Invalid port number provided, reverting to default port") |
There was a problem hiding this comment.
| except ValueError: | |
| log.debug("Invalid port number provided, reverting to default port") | |
| except ValueError as e: | |
| log.warning("DD_DOGSTATSD_URL value %r contained invalid port number, falling back to %d: %s", dogstatsd_url, DEFAULT_PORT, e) |
| elif parsed.scheme == "udp": | ||
| try: | ||
| p_port = parsed.port | ||
| # Python 2 doesn't automatically perform bounds checking on the port |
There was a problem hiding this comment.
(Not sure which versions we support, but this was fixed in 2014)
| p_port = parsed.port | ||
| # Python 2 doesn't automatically perform bounds checking on the port | ||
| if p_port is None or p_port < 0 or p_port > 65535: | ||
| log.debug("Invalid port number provided, reverting to default port") |
There was a problem hiding this comment.
| log.debug("Invalid port number provided, reverting to default port") | |
| log.debug("DD_DOGSTATSD_URL value %r had no or invalid port number, reverting to default %s", dogstatsd_url, DEFAULT_PORT) |
|
|
||
| elif dogstatsd_url.startswith(WINDOWS_NAMEDPIPE_SCHEME): | ||
| log.debug( | ||
| "DD_DOGSTATSD_URL is configured to utilize a windows named pipe, " | ||
| "which is not currently supported by datadogpy. Falling back to " | ||
| "alternate connection identifiers." | ||
| ) |
There was a problem hiding this comment.
In the spirit of our documentation guidelines, let the generic "unsupported scheme" message handle this case.
And AIUI this was the only place where WINDOWS_NAMEDPIPE_SCHEME was used, so that can be removed too.
| elif dogstatsd_url.startswith(WINDOWS_NAMEDPIPE_SCHEME): | |
| log.debug( | |
| "DD_DOGSTATSD_URL is configured to utilize a windows named pipe, " | |
| "which is not currently supported by datadogpy. Falling back to " | |
| "alternate connection identifiers." | |
| ) |
| if parsed.scheme == "unix": | ||
| log.debug( | ||
| "Found a DD_DOGSTATSD_URL matching the uds syntax, " | ||
| "setting socket path %s.", dogstatsd_url |
There was a problem hiding this comment.
| "setting socket path %s.", dogstatsd_url | |
| "setting socket path to %r.", dogstatsd_url |
| "Found a DD_DOGSTATSD_URL matching the uds syntax, " | ||
| "setting socket path %s.", dogstatsd_url | ||
| ) | ||
| return host, port, dogstatsd_url |
There was a problem hiding this comment.
Caller assigns this into socket_path. Should this return the full url or just the path?
Adds support for the unix:// and udp:// variants of DD_DOGSTATSD_URL.
Description of the Change
During dogstatsd creation, utilize the DD_DOGSTATSD_URL environment variable if the provided host and port are their default values and no socket_path has been provided. unix:// and udp:// paths are supported, but \\.\pipe\ paths (windows named pipes) are not yet implemented.
Alternate Designs
Possible Drawbacks
Verification Process
Form a Dogstatsd object with different values of DD_DOGSTATSD_URL. The following cases are important to verify:
During fallback, DD_AGENT_HOST and DD_DOGSTATSD_PORT should be utilized if present. Otherwise, udp is selected with DEFAULT_HOST:DEFAULT_PORT.
Additional Notes
This PR is reliant on the functionality in !869, with a comparison available here
Release Notes
Review checklist (to be filled by reviewers)
changelog/label attached. If applicable it should have thebackward-incompatiblelabel attached.do-not-merge/label attached.kind/andseverity/labels attached at least.