potiuk commented on code in PR #74848:
URL: https://github.com/apache/airflow/pull/74848#discussion_r4308139180
##########
shared/observability/src/airflow_shared/observability/metrics/datadog_logger.py:
##########
@@ -39,6 +39,27 @@
log = logging.getLogger(__name__)
+class _DogStatsdTimer(Timer):
+ """
+ An implementation of stats.Timer() which sends its measured duration to
DogStatsd.
+
+ ``DogStatsd.timed()`` reports seconds unless the client sets ``use_ms``,
and rounds to whole
+ milliseconds when it does, so the duration measured by ``Timer`` is sent
instead.
+ """
+
+ def __init__(self, dogstatsd: DogStatsd, stat: str, tags: list[str],
**kwargs) -> None:
+ super().__init__()
+ self.dogstatsd = dogstatsd
+ self.stat = stat
+ self.tags = tags
+ self.kwargs = kwargs
+
+ def stop(self, send: bool = True) -> None:
+ super().stop(send)
+ if send and self.duration is not None:
+ self.dogstatsd.timing(metric=self.stat, value=self.duration,
tags=self.tags, **self.kwargs)
Review Comment:
Extra `**kwargs` used to go to `DogStatsd.timed()` and now go to
`DogStatsd.timing()`, which has a different signature. `Stats.timer("x",
use_ms=True)` worked before, and now raises `TypeError` when the timer stops,
from the `with` block's `__exit__`, which can hide the timed code's own
exception. Taking only what `timing()` accepts makes an unsupported argument
fail when the timer is created instead (the `**kwargs` in `timer()` below would
then need the same treatment):
```suggestion
def __init__(
self, dogstatsd: DogStatsd, stat: str, tags: list[str], sample_rate:
float | None = None
) -> None:
super().__init__()
self.dogstatsd = dogstatsd
self.stat = stat
self.tags = tags
self.sample_rate = sample_rate
def stop(self, send: bool = True) -> None:
super().stop(send)
if send and self.duration is not None:
self.dogstatsd.timing(
metric=self.stat, value=self.duration, tags=self.tags,
sample_rate=self.sample_rate
)
```
##########
shared/observability/tests/observability/metrics/test_stats.py:
##########
@@ -253,19 +253,20 @@ def
test_does_send_stats_using_dogstatsd_when_statsd_and_dogstatsd_both_on(self)
metric="empty_key", sample_rate=1, tags=[], value=1
)
- @mock.patch.object(time, "perf_counter", side_effect=[0.0, 100.0])
- def test_timer(self, time_mock):
+ @mock.patch.object(time, "perf_counter", side_effect=[0.0, 0.0004])
+ def test_timer_sends_duration_in_milliseconds(self, time_mock):
with self.dogstatsd.timer("empty_timer") as timer:
pass
- self.dogstatsd_client.timed.assert_called_once_with("empty_timer",
tags=[])
- expected_duration = 1000.0 * 100.0
- assert expected_duration == timer.duration
- assert time_mock.call_count == 2
+ assert timer.duration == pytest.approx(0.4)
+ self.dogstatsd_client.timing.assert_called_once_with(
+ metric="empty_timer", value=timer.duration, tags=[]
+ )
Review Comment:
`value=timer.duration` compares the code under test with itself; asserting
the expected milliseconds makes the test check what is actually sent:
```suggestion
self.dogstatsd_client.timing.assert_called_once_with(
metric="empty_timer", value=pytest.approx(0.4), tags=[]
)
```
--
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]
