ashb commented on code in PR #43340:
URL: https://github.com/apache/airflow/pull/43340#discussion_r1840039162
##########
airflow/metrics/datadog_logger.py:
##########
@@ -51,7 +55,33 @@
)
-class SafeDogStatsdLogger:
+def prepare_stat_with_tags(fn: T) -> T:
+ """Prepare tags and stat."""
+
+ @wraps(fn)
+ def wrapper(self, stat: str | None = None, *args, tags: dict[str, str] |
None = None, **kwargs):
+ stat = stat or ""
Review Comment:
Why do we need this `stat or ""`? Shouldn't this decorator pass through
whatever it's given?
##########
airflow/metrics/datadog_logger.py:
##########
@@ -66,109 +96,95 @@ def __init__(
self.metrics_tags = metrics_tags
self.metric_tags_validator = metric_tags_validator
+ @prepare_stat_with_tags
@validate_stat
def incr(
self,
- stat: str,
+ metric_name: str,
count: int = 1,
rate: float = 1,
*,
- tags: dict[str, str] | None = None,
+ tags: list[str] | None = None,
Review Comment:
I see what's going on here (the decorator changes the dict into the list)
but I wonder if there's an easier way of dealing with this... 🤔
##########
airflow/metrics/datadog_logger.py:
##########
@@ -51,7 +55,33 @@
)
-class SafeDogStatsdLogger:
+def prepare_stat_with_tags(fn: T) -> T:
+ """Prepare tags and stat."""
+
+ @wraps(fn)
+ def wrapper(self, stat: str | None = None, *args, tags: dict[str, str] |
None = None, **kwargs):
+ stat = stat or ""
+
+ if tags and self.metrics_tags:
+ valid_tags: dict[str, str] = {}
+ for k, v in tags.items():
+ if self.metric_tags_validator.test(k):
+ if all(c not in [",", "="] for c in f"{v}{k}"):
Review Comment:
Why do we care about `=` being in the key or value? Isn't the separator for
datadog tags `:` so this should be
```suggestion
if ":" not in f"{v}{k}":
```
##########
airflow/metrics/statsd_logger.py:
##########
@@ -44,27 +46,32 @@
log = logging.getLogger(__name__)
-def prepare_stat_with_tags(fn: T) -> T:
- """Add tags to stat with influxdb standard format if influxdb_tags_enabled
is True."""
+def prepare_metric_name_with_tags(fn: T) -> T:
+ """Add tags to metric_name with InfluxDB standard format if
influxdb_tags_enabled is True."""
@wraps(fn)
def wrapper(
- self, stat: str | None = None, *args, tags: dict[str, str] | None =
None, **kwargs
+ self, metric_name: str | None = None, tags: dict[str, str] | None =
None
Review Comment:
This also possibly breaks things as `stats.incr("metric_a", 2). Please bring
`*args` back.
##########
airflow/metrics/datadog_logger.py:
##########
@@ -51,7 +55,33 @@
)
-class SafeDogStatsdLogger:
+def prepare_stat_with_tags(fn: T) -> T:
+ """Prepare tags and stat."""
+
+ @wraps(fn)
+ def wrapper(self, stat: str | None = None, *args, tags: dict[str, str] |
None = None, **kwargs):
+ stat = stat or ""
Review Comment:
Nit: the `stat` has been renamed to `metric_name` elsewhere, so lets do it
too.
```suggestion
def wrapper(self, metric_name: str | None = None, *args, tags: dict[str,
str] | None = None, **kwargs):
metric_name = metric_name or ""
```
##########
airflow/metrics/datadog_logger.py:
##########
@@ -51,7 +55,33 @@
)
-class SafeDogStatsdLogger:
+def prepare_stat_with_tags(fn: T) -> T:
+ """Prepare tags and stat."""
+
+ @wraps(fn)
+ def wrapper(self, stat: str | None = None, *args, tags: dict[str, str] |
None = None, **kwargs):
+ stat = stat or ""
+
+ if tags and self.metrics_tags:
+ valid_tags: dict[str, str] = {}
+ for k, v in tags.items():
+ if self.metric_tags_validator.test(k):
+ if all(c not in [",", "="] for c in f"{v}{k}"):
Review Comment:
Shouldn't this be the job of `self.metric_tags_validator` though?
##########
airflow/metrics/statsd_logger.py:
##########
@@ -44,27 +46,32 @@
log = logging.getLogger(__name__)
-def prepare_stat_with_tags(fn: T) -> T:
- """Add tags to stat with influxdb standard format if influxdb_tags_enabled
is True."""
+def prepare_metric_name_with_tags(fn: T) -> T:
+ """Add tags to metric_name with InfluxDB standard format if
influxdb_tags_enabled is True."""
@wraps(fn)
def wrapper(
- self, stat: str | None = None, *args, tags: dict[str, str] | None =
None, **kwargs
+ self, metric_name: str | None = None, tags: dict[str, str] | None =
None
) -> Callable[[str], str]:
- if self.influxdb_tags_enabled:
- if stat is not None and tags is not None:
- for k, v in tags.items():
- if self.metric_tags_validator.test(k):
- if all(c not in [",", "="] for c in f"{v}{k}"):
- stat += f",{k}={v}"
- else:
- log.error("Dropping invalid tag: %s=%s.", k, v)
- return fn(self, stat, *args, tags=tags, **kwargs)
+ metric_name = metric_name or ""
Review Comment:
Ditto here? Why aren't we leaving None as None?
##########
tests/core/test_stats.py:
##########
@@ -485,7 +485,7 @@ def test_increment_counter(self):
def test_increment_counter_with_tags(self):
self.stats.incr(
"test_stats_run.delay",
- tags={"key0": 0, "key1": "val1", "key2": "val2"},
+ tags={"key0": "0", "key1": "val1", "key2": "val2"},
Review Comment:
```suggestion
tags={"key0": 0, "key1": "val1", "key2": "val2"},
```
If it worked before this test should probably still pass without needing
this change.
##########
airflow/metrics/statsd_logger.py:
##########
@@ -44,27 +46,32 @@
log = logging.getLogger(__name__)
-def prepare_stat_with_tags(fn: T) -> T:
- """Add tags to stat with influxdb standard format if influxdb_tags_enabled
is True."""
+def prepare_metric_name_with_tags(fn: T) -> T:
+ """Add tags to metric_name with InfluxDB standard format if
influxdb_tags_enabled is True."""
@wraps(fn)
def wrapper(
- self, stat: str | None = None, *args, tags: dict[str, str] | None =
None, **kwargs
+ self, metric_name: str | None = None, tags: dict[str, str] | None =
None
Review Comment:
Nit: you changed tags from keyword only to positional with this change.
```suggestion
self, metric_name: str | None = None, *, tags: dict[str, str] | None
= None
```
--
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]
