uranusjr commented on code in PR #70517:
URL: https://github.com/apache/airflow/pull/70517#discussion_r3732869035
##########
shared/observability/src/airflow_shared/observability/metrics/statsd_logger.py:
##########
@@ -31,11 +31,54 @@
)
if TYPE_CHECKING:
- from statsd import StatsClient
+ from configparser import ConfigParser
+
+ from statsd import StatsClient, UnixSocketStatsClient
from .protocols import DeltaType
from .validators import ListValidator
+# Fallbacks for the plain StatsD client, which (unlike datadog's DogStatsd)
cannot resolve a
+# None host/port. The DataDog backend leaves these unset so the client reads
its own env vars.
+DEFAULT_STATSD_HOST = "localhost"
+DEFAULT_STATSD_PORT = 8125
+
+
+def resolve_statsd_connection(conf: ConfigParser) -> tuple[str | None, int |
None, str | None]:
+ """
+ Resolve the (host, port, socket_path) a StatsD/DataDog logger should use
from config.
+
+ Only explicitly-configured values are returned; anything unset comes back
as ``None``.
+ ``statsd_host`` / ``statsd_port`` are nullable config keys, so
``conf.has_option`` tells
+ an explicit value apart from "unset". Returning ``None`` for everything
(when nothing is
+ configured) lets the DataDog client fall back to its own environment
variables
+ (``DD_AGENT_HOST`` / ``DD_DOGSTATSD_URL`` / ``DD_DOGSTATSD_PORT``).
+
+ ``statsd_socket_path`` (a Unix Domain Socket) is not fatal to combine with
``statsd_host`` /
+ ``statsd_port`` — the socket takes precedence — but it is almost certainly
a mistake, so a
+ warning is logged. No defaults are applied here; each backend applies its
own if needed.
+ """
+ socket_path = conf.get("metrics", "statsd_socket_path", fallback=None)
+ host = (
+ conf.get("metrics", "statsd_host", fallback=None)
+ if conf.has_option("metrics", "statsd_host")
+ else None
+ )
+ port = (
+ conf.getint("metrics", "statsd_port", fallback=None)
+ if conf.has_option("metrics", "statsd_port")
+ else None
Review Comment:
This would crash with the default (not explicitly configured) since *None*
still makes `has_option` return True, but that wouldn’t be parsable by
`getint`. (This would not be an issue if the suggestions in
https://github.com/apache/airflow/pull/70517/changes#r3721048824 are applied.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
