Skip to content

Conversation

@ddrthall
Copy link
Contributor

@ddrthall ddrthall commented Nov 18, 2024

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:

  • A non-default host, port or socket_path provided to the Dogstatsd constructor should take precedence over DD_DOGSTATS_URL.
  • unix:// prefixed DOGSTATD_URLS should result in a socket_path and a None host/port pair
  • udp:// prefixed DOGSTATSD_URLS should result in a valid host/port pair, with appropriate type safety around casting the port component from a string to an integer.
  • \\.\pipe\ prefixed DOGSTATD_URLS should provide an informative debug message and fallback to other connection identifications.
  • An unrecognized prefix for DOGSTATD_URL should fallback to other connection identifications

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)

  • Feature or bug fix MUST have appropriate tests (unit, integration, etc...)
  • PR title must be written as a CHANGELOG entry (see why)
  • Files changes must correspond to the primary purpose of the PR as described in the title (small unrelated changes should have their own PR)
  • PR must have one changelog/ label attached. If applicable it should have the backward-incompatible label attached.
  • PR should not have do-not-merge/ label attached.
  • If Applicable, issue must have kind/ and severity/ labels attached at least.

@ddrthall ddrthall added the changelog/Added Added features results into a minor version bump label Nov 18, 2024
@github-actions github-actions bot added the documentation Documentation related changes label Nov 18, 2024
@ddrthall ddrthall force-pushed the ryan.hall/add_dogstatsd_url_support branch 2 times, most recently from 7a4ec25 to 37f0173 Compare November 18, 2024 21:16
@ddrthall ddrthall added the do-not-merge/HOLD Do not merge this PR label Nov 18, 2024
@ddrthall ddrthall marked this pull request as ready for review November 18, 2024 21:19
@ddrthall ddrthall requested review from a team as code owners November 18, 2024 21:19
@ddrthall ddrthall requested a review from a team November 18, 2024 21:19
@ddrthall ddrthall force-pushed the ryan.hall/add_dogstatsd_url_support branch from 37f0173 to 71daae4 Compare December 4, 2024 18:00
@github-actions
Copy link

github-actions bot commented Jan 4, 2025

This issue has been automatically marked as stale because it has not had activity in the last 30 days.
Note that the issue will not be automatically closed, but this notification will remind us to investigate why there's been inactivity.

@github-actions github-actions bot added the stale Stale - Bot reminder label Jan 4, 2025
@ddrthall ddrthall force-pushed the ryan.hall/add_dogstatsd_url_support branch from 71daae4 to 9bdac11 Compare March 26, 2025 15:11
@ddrthall ddrthall requested review from a team as code owners March 26, 2025 15:11
@ddrthall ddrthall force-pushed the ryan.hall/add_dogstatsd_url_support branch from 9bdac11 to ac18e9b Compare May 1, 2025 17:11
@ddrthall ddrthall removed the do-not-merge/HOLD Do not merge this PR label May 23, 2025
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.
@carlosroman carlosroman force-pushed the ryan.hall/add_dogstatsd_url_support branch from ac18e9b to ed1bc9b Compare July 7, 2025 13:46

if (
host == DEFAULT_HOST
and port == DEFAULT_PORT
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense if an explict request for defaults Dogstatsd(host='localhost', port=8125) takes precedence over environment variables?

Comment on lines +646 to +650
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."
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense be more specific in the error message? Probably worth a higher log level too.

Suggested change
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)

Comment on lines +664 to +665
"Port number provided in DD_DOGSTATSD_PORT env var is not an integer: \
%s, using %s as port number",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"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",

Comment on lines +635 to +636
except ValueError:
log.debug("Invalid port number provided, reverting to default port")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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
Copy link
Contributor

Choose a reason for hiding this comment

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

(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")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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)

Comment on lines +620 to +626

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."
)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Suggested change
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
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"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
Copy link
Contributor

Choose a reason for hiding this comment

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

Caller assigns this into socket_path. Should this return the full url or just the path?

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

Labels

changelog/Added Added features results into a minor version bump documentation Documentation related changes stale Stale - Bot reminder

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants