ferruzzi commented on code in PR #43340:
URL: https://github.com/apache/airflow/pull/43340#discussion_r1821166636
##########
tests/core/test_otel_logger.py:
##########
@@ -358,108 +358,134 @@ def test_timer_start_and_stop_manually_send_true(self,
mock_time, metrics_consis
)
def test_incr_counter(self):
- # Arrange
- logger = self.stats
metric_name = "test_metric"
count = 5
tags = {"env": "prod"}
- # Act
- logger.incr(metric_name, count=count, tags=tags)
+ self.stats.incr(metric_name, count=count, tags=tags)
- # Assert
- counter = logger.metrics_map.get_counter(
- full_name(prefix=logger.prefix, name=metric_name), attributes=tags
+ counter = self.stats.metrics_map.get_counter(
+ full_name(prefix=self.stats.prefix, name=metric_name),
attributes=tags
)
counter.add.assert_called_with(count, attributes=tags)
def test_decr_counter(self):
- # Arrange
- logger = self.stats
metric_name = "test_metric"
count = 3
tags = {"env": "prod"}
- # Act
- logger.decr(metric_name, count=count, tags=tags)
+ self.stats.decr(metric_name, count=count, tags=tags)
- # Assert
- counter = logger.metrics_map.get_counter(
- full_name(prefix=logger.prefix, name=metric_name), attributes=tags
+ counter = self.stats.metrics_map.get_counter(
+ full_name(prefix=self.stats.prefix, name=metric_name),
attributes=tags
)
counter.add.assert_called_with(-count, attributes=tags)
@pytest.mark.parametrize("expected_duration", [2.5, 1.0, 3.0])
def test_timing(self, expected_duration):
- # Arrange
- logger = self.stats
metric_name = "test_metric"
tags = {"env": "prod"}
# Mocking time.perf_counter to simulate timing
with mock.patch.object(time, "perf_counter", side_effect=[0.0,
expected_duration]):
- with logger.timer(metric_name, tags=tags) as timer:
+ with self.stats.timer(metric_name, tags=tags) as timer:
pass
- # Assert
+ acceptable_margin = 0.1
assert isinstance(timer.duration, float)
- assert timer.duration >= expected_duration - 0.1
- assert timer.duration <= expected_duration + 0.1
+ assert timer.duration >= expected_duration - acceptable_margin
+ assert timer.duration <= expected_duration + acceptable_margin
assert self.meter.get_meter().create_observable_gauge.call_count == 1
self.meter.get_meter().create_observable_gauge.assert_called_once_with(
Review Comment:
You can actually drop the `call_count == 1` assertion here since
`assert_called_once` also asserts exactly once.
[[docs]](https://docs.python.org/3/library/unittest.mock.html#unittest.mock.Mock.assert_called_once_with)
##########
tests/core/test_otel_logger.py:
##########
@@ -358,108 +358,134 @@ def test_timer_start_and_stop_manually_send_true(self,
mock_time, metrics_consis
)
def test_incr_counter(self):
- # Arrange
- logger = self.stats
metric_name = "test_metric"
count = 5
tags = {"env": "prod"}
- # Act
- logger.incr(metric_name, count=count, tags=tags)
+ self.stats.incr(metric_name, count=count, tags=tags)
- # Assert
- counter = logger.metrics_map.get_counter(
- full_name(prefix=logger.prefix, name=metric_name), attributes=tags
+ counter = self.stats.metrics_map.get_counter(
+ full_name(prefix=self.stats.prefix, name=metric_name),
attributes=tags
)
counter.add.assert_called_with(count, attributes=tags)
def test_decr_counter(self):
- # Arrange
- logger = self.stats
metric_name = "test_metric"
count = 3
tags = {"env": "prod"}
- # Act
- logger.decr(metric_name, count=count, tags=tags)
+ self.stats.decr(metric_name, count=count, tags=tags)
- # Assert
- counter = logger.metrics_map.get_counter(
- full_name(prefix=logger.prefix, name=metric_name), attributes=tags
+ counter = self.stats.metrics_map.get_counter(
+ full_name(prefix=self.stats.prefix, name=metric_name),
attributes=tags
)
counter.add.assert_called_with(-count, attributes=tags)
@pytest.mark.parametrize("expected_duration", [2.5, 1.0, 3.0])
def test_timing(self, expected_duration):
- # Arrange
- logger = self.stats
metric_name = "test_metric"
tags = {"env": "prod"}
# Mocking time.perf_counter to simulate timing
with mock.patch.object(time, "perf_counter", side_effect=[0.0,
expected_duration]):
- with logger.timer(metric_name, tags=tags) as timer:
+ with self.stats.timer(metric_name, tags=tags) as timer:
pass
- # Assert
+ acceptable_margin = 0.1
assert isinstance(timer.duration, float)
- assert timer.duration >= expected_duration - 0.1
- assert timer.duration <= expected_duration + 0.1
+ assert timer.duration >= expected_duration - acceptable_margin
+ assert timer.duration <= expected_duration + acceptable_margin
assert self.meter.get_meter().create_observable_gauge.call_count == 1
self.meter.get_meter().create_observable_gauge.assert_called_once_with(
- name=full_name(prefix=logger.prefix, name=metric_name),
callbacks=ANY
+ name=full_name(prefix=self.stats.prefix, name=metric_name),
callbacks=ANY
)
def test_gauge_set(self):
- # Arrange
- logger = self.stats
metric_name = "test_metric"
value = 42
tags = {"env": "prod"}
- # Act
- logger.gauge(metric_name, value, tags=tags)
+ self.stats.gauge(metric_name, value, tags=tags)
- # Assert
- current_value =
logger.metrics_map.poke_gauge(full_name(prefix=logger.prefix,
name=metric_name), tags)
+ current_value = self.stats.metrics_map.poke_gauge(
+ full_name(prefix=self.stats.prefix, name=metric_name), tags
+ )
assert current_value == value
def test_gauge_increment(self):
- # Arrange
- logger = self.stats
metric_name = "test_metric"
count = 10
tags = {"env": "prod"}
- # Act
- logger.gauge(metric_name, count, delta=True, tags=tags) # Use
delta=True to increment the gauge
+ self.stats.gauge(metric_name, count, delta=True, tags=tags) # Use
delta=True to increment the gauge
- # Assert
# Retrieve the current value using poke_gauge
- current_value =
logger.metrics_map.poke_gauge(full_name(prefix=logger.prefix,
name=metric_name), tags)
+ current_value = self.stats.metrics_map.poke_gauge(
+ full_name(prefix=self.stats.prefix, name=metric_name), tags
+ )
assert current_value == count
def test_gauge_decrement(self):
- # Arrange
- logger = self.stats
metric_name = "test_metric"
count = 5
tags = {"env": "prod"}
- # Act: Increment the gauge by 'count'
- logger.gauge(metric_name, count, delta=True, tags=tags)
+ self.stats.gauge(metric_name, count, delta=True, tags=tags)
- # Assert: Check if the current value after incrementing is equal to
'count'
- current_value_after_increment = logger.metrics_map.poke_gauge(
- full_name(prefix=logger.prefix, name=metric_name), tags
+ current_value_after_increment = self.stats.metrics_map.poke_gauge(
+ full_name(prefix=self.stats.prefix, name=metric_name), tags
)
assert current_value_after_increment == count # Ensure the gauge
value reflects the increment
- # Act for decrement
- logger.gauge(metric_name, -count, delta=True, tags=tags) # Decrement
the gauge by 'count'
+ self.stats.gauge(metric_name, -count, delta=True, tags=tags) #
Decrement the gauge by 'count'
- # Assert: Ensure the gauge value reflects the decrement to zero
- current_value_after_decrement = logger.metrics_map.poke_gauge(
- full_name(prefix=logger.prefix, name=metric_name), tags
+ current_value_after_decrement = self.stats.metrics_map.poke_gauge(
+ full_name(prefix=self.stats.prefix, name=metric_name), tags
)
assert current_value_after_decrement == 0
+
Review Comment:
Non-blocking nitpick/style suggestion, feel free not to implement this idea;
You could combine all of the following new tests into one parametrized test
which passes in the name being tested, the tags (or perhaps a bool indicating
whether to use a set of tags which are defined in the test itself), and the
expected exception message. If you'd like to see what that might look like,
check out
[this](https://github.com/apache/airflow/blob/main/providers/tests/amazon/aws/operators/test_appflow.py#L212)
test
##########
tests/core/test_otel_logger.py:
##########
@@ -358,108 +358,134 @@ def test_timer_start_and_stop_manually_send_true(self,
mock_time, metrics_consis
)
def test_incr_counter(self):
- # Arrange
- logger = self.stats
metric_name = "test_metric"
count = 5
tags = {"env": "prod"}
- # Act
- logger.incr(metric_name, count=count, tags=tags)
+ self.stats.incr(metric_name, count=count, tags=tags)
- # Assert
- counter = logger.metrics_map.get_counter(
- full_name(prefix=logger.prefix, name=metric_name), attributes=tags
+ counter = self.stats.metrics_map.get_counter(
+ full_name(prefix=self.stats.prefix, name=metric_name),
attributes=tags
)
counter.add.assert_called_with(count, attributes=tags)
def test_decr_counter(self):
- # Arrange
- logger = self.stats
metric_name = "test_metric"
count = 3
tags = {"env": "prod"}
- # Act
- logger.decr(metric_name, count=count, tags=tags)
+ self.stats.decr(metric_name, count=count, tags=tags)
- # Assert
- counter = logger.metrics_map.get_counter(
- full_name(prefix=logger.prefix, name=metric_name), attributes=tags
+ counter = self.stats.metrics_map.get_counter(
+ full_name(prefix=self.stats.prefix, name=metric_name),
attributes=tags
)
counter.add.assert_called_with(-count, attributes=tags)
@pytest.mark.parametrize("expected_duration", [2.5, 1.0, 3.0])
def test_timing(self, expected_duration):
- # Arrange
- logger = self.stats
metric_name = "test_metric"
tags = {"env": "prod"}
# Mocking time.perf_counter to simulate timing
with mock.patch.object(time, "perf_counter", side_effect=[0.0,
expected_duration]):
- with logger.timer(metric_name, tags=tags) as timer:
+ with self.stats.timer(metric_name, tags=tags) as timer:
pass
- # Assert
+ acceptable_margin = 0.1
assert isinstance(timer.duration, float)
- assert timer.duration >= expected_duration - 0.1
- assert timer.duration <= expected_duration + 0.1
+ assert timer.duration >= expected_duration - acceptable_margin
+ assert timer.duration <= expected_duration + acceptable_margin
assert self.meter.get_meter().create_observable_gauge.call_count == 1
self.meter.get_meter().create_observable_gauge.assert_called_once_with(
- name=full_name(prefix=logger.prefix, name=metric_name),
callbacks=ANY
+ name=full_name(prefix=self.stats.prefix, name=metric_name),
callbacks=ANY
)
def test_gauge_set(self):
- # Arrange
- logger = self.stats
metric_name = "test_metric"
value = 42
tags = {"env": "prod"}
- # Act
- logger.gauge(metric_name, value, tags=tags)
+ self.stats.gauge(metric_name, value, tags=tags)
- # Assert
- current_value =
logger.metrics_map.poke_gauge(full_name(prefix=logger.prefix,
name=metric_name), tags)
+ current_value = self.stats.metrics_map.poke_gauge(
+ full_name(prefix=self.stats.prefix, name=metric_name), tags
+ )
assert current_value == value
def test_gauge_increment(self):
- # Arrange
- logger = self.stats
metric_name = "test_metric"
count = 10
tags = {"env": "prod"}
- # Act
- logger.gauge(metric_name, count, delta=True, tags=tags) # Use
delta=True to increment the gauge
+ self.stats.gauge(metric_name, count, delta=True, tags=tags) # Use
delta=True to increment the gauge
- # Assert
# Retrieve the current value using poke_gauge
- current_value =
logger.metrics_map.poke_gauge(full_name(prefix=logger.prefix,
name=metric_name), tags)
+ current_value = self.stats.metrics_map.poke_gauge(
+ full_name(prefix=self.stats.prefix, name=metric_name), tags
+ )
assert current_value == count
def test_gauge_decrement(self):
- # Arrange
- logger = self.stats
metric_name = "test_metric"
count = 5
tags = {"env": "prod"}
- # Act: Increment the gauge by 'count'
- logger.gauge(metric_name, count, delta=True, tags=tags)
+ self.stats.gauge(metric_name, count, delta=True, tags=tags)
- # Assert: Check if the current value after incrementing is equal to
'count'
- current_value_after_increment = logger.metrics_map.poke_gauge(
- full_name(prefix=logger.prefix, name=metric_name), tags
+ current_value_after_increment = self.stats.metrics_map.poke_gauge(
+ full_name(prefix=self.stats.prefix, name=metric_name), tags
)
assert current_value_after_increment == count # Ensure the gauge
value reflects the increment
- # Act for decrement
- logger.gauge(metric_name, -count, delta=True, tags=tags) # Decrement
the gauge by 'count'
+ self.stats.gauge(metric_name, -count, delta=True, tags=tags) #
Decrement the gauge by 'count'
- # Assert: Ensure the gauge value reflects the decrement to zero
- current_value_after_decrement = logger.metrics_map.poke_gauge(
- full_name(prefix=logger.prefix, name=metric_name), tags
+ current_value_after_decrement = self.stats.metrics_map.poke_gauge(
+ full_name(prefix=self.stats.prefix, name=metric_name), tags
)
assert current_value_after_decrement == 0
+
Review Comment:
Nitpick, non-blocking: Consider combining the following test cases into one
parameterized test which accepts `(test_name, test_tags, expected_message)`,
For an example of what that could look like, check out [this
test](https://github.com/apache/airflow/blob/main/providers/tests/amazon/aws/operators/test_appflow.py#L212)
##########
airflow/metrics/otel_logger.py:
##########
@@ -241,8 +239,8 @@ def decr(
raise ValueError("count must be a positive value.")
full_metric_name = self.get_name(metric_name, tags)
- if self.metrics_validator.test(full_metric_name) and
name_is_otel_safe(self.prefix, full_metric_name):
- counter =
self.metrics_map.get_counter(full_name(prefix=self.prefix,
name=full_metric_name))
Review Comment:
So much cleaner :heart_eyes: I love it when existing code can be
streamlined.
##########
airflow/metrics/otel_logger.py:
##########
@@ -212,10 +212,8 @@ def incr(
full_metric_name = self.get_name(metric_name, tags)
- if self.metrics_validator.test(full_metric_name) and
name_is_otel_safe(self.prefix, full_metric_name):
- counter = self.metrics_map.get_counter(
- full_name(prefix=self.prefix, name=full_metric_name),
attributes=tags
- )
+ if self.metrics_validator.test(full_metric_name):
Review Comment:
I see what you tried to do but this ends up being a breaking change. By
moving the `full_name()` call into the `get_name()`, we are now passing the
full name (with prefix) into the validator.test() call which will break all
user's Allow/Block lists.
If you choose to add the `full_name()` call in there then you must strip
`prefix` and `delimiter` off before passing it into `validator.test()`, OR add
some logic to test() which does that before checking the name/pattern match.
Those both feel like acceptable options, but maybe feel like more work than
leaving the `full_name()` calls where they are.
Which, in hindsight, brings up the naming of the variable
`full_metric_name`. If it includes the prefix, that sounds good, but without
the prefix then maybe that needs to be renamed. Naming is one of the hardest
parts of the job. Perhaps `base_name` since it gets the prefix added later?
##########
airflow/metrics/statsd_logger.py:
##########
@@ -51,26 +51,22 @@ def prepare_metric_name_with_tags(fn: T) -> T:
@wraps(fn)
def wrapper(
- self, metric_name: str | None = None, tags: dict[str, str | int] |
None = None
+ self, metric_name: str | None = None, tags: dict[str, str] | None =
None
Review Comment:
If you are no longer accepting `int` as a value type here, then I don't
think you need to do the casting on L60? I'm, not sure why you had added
`int` as a valid type and then removed it thought, so one or the other should
be fixed.
If `tags` is going to be a `dict[str, str]` then there is no need to cast it
to a `dict[str, str]` on L60. If it is going to be declared as `dict[str, str
| int]` then we should keep the casing.
--
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]
