shubham22 commented on code in PR #36250:
URL: https://github.com/apache/airflow/pull/36250#discussion_r1450969095
##########
airflow/config_templates/config.yml:
##########
@@ -944,6 +944,14 @@ metrics:
description: |
StatsD (https://github.com/etsy/statsd) integration settings.
options:
+ metrics_use_pattern_match:
+ description: |
+ If true, metrics_allow_list and metrics_block_list will use regex
pattern matching
Review Comment:
I would suggest changing description of "metrics_allow_list" and/or
"metrics_block_list" to mention about behaviour change when this is "True"
##########
airflow/config_templates/config.yml:
##########
@@ -944,6 +944,14 @@ metrics:
description: |
StatsD (https://github.com/etsy/statsd) integration settings.
options:
+ metrics_use_pattern_match:
+ description: |
+ If true, metrics_allow_list and metrics_block_list will use regex
pattern matching
+ anywhere within the metric name instead of only prefix matching at the
start of the name.
+ version_added: 2.7.4
Review Comment:
change the version added, once the milestone is updated. If possible, let's
do it in 2.8.1
##########
tests/core/test_stats.py:
##########
@@ -265,40 +271,128 @@ def
test_does_not_send_stats_using_statsd_when_statsd_and_dogstatsd_both_on(self
importlib.reload(airflow.stats)
-class TestStatsWithAllowList:
- def setup_method(self):
- self.statsd_client = Mock(spec=statsd.StatsClient)
- self.stats = SafeStatsdLogger(self.statsd_client,
AllowListValidator("stats_one, stats_two"))
+class TestStatsAllowAndBlockLists:
+ @pytest.mark.parametrize(
+ "validator, stat_name, expect_incr",
+ [
+ (PatternAllowListValidator, "stats_one", True),
+ (PatternAllowListValidator, "stats_two.bla", True),
+ (PatternAllowListValidator, "stats_three.foo", True),
+ (PatternAllowListValidator, "stats_foo_three", True),
+ (PatternAllowListValidator, "stats_three", False),
+ (AllowListValidator, "stats_one", True),
+ (AllowListValidator, "stats_two.bla", True),
+ (AllowListValidator, "stats_three.foo", False),
+ (AllowListValidator, "stats_foo_three", False),
+ (AllowListValidator, "stats_three", False),
+ (PatternBlockListValidator, "stats_one", False),
+ (PatternBlockListValidator, "stats_two.bla", False),
+ (PatternBlockListValidator, "stats_three.foo", False),
+ (PatternBlockListValidator, "stats_foo_three", False),
+ (PatternBlockListValidator, "stats_foo", False),
+ (PatternBlockListValidator, "stats_three", True),
+ (BlockListValidator, "stats_one", False),
+ (BlockListValidator, "stats_two.bla", False),
+ (BlockListValidator, "stats_three.foo", True),
+ (BlockListValidator, "stats_foo_three", True),
+ (BlockListValidator, "stats_three", True),
+ ],
+ )
+ def test_allow_and_block_list(self, validator, stat_name, expect_incr):
+ statsd_client = Mock(spec=statsd.StatsClient)
+ stats = SafeStatsdLogger(statsd_client, validator("stats_one,
stats_two, foo"))
+
+ stats.incr(stat_name)
+
+ if expect_incr:
+ statsd_client.incr.assert_called_once_with(stat_name, 1, 1)
+ else:
+ statsd_client.assert_not_called()
+
+ @pytest.mark.parametrize(
+ "match_pattern, expect_incr",
+ [
+ ("^stat", True),
+ ("a.{4}o", True),
+ ("^banana", False),
+ ],
+ )
+ def test_regex_matches(self, match_pattern, expect_incr):
+ stat_name = "stats_foo_one"
+ validator = PatternAllowListValidator
- def test_increment_counter_with_allowed_key(self):
- self.stats.incr("stats_one")
- self.statsd_client.incr.assert_called_once_with("stats_one", 1, 1)
+ statsd_client = Mock(spec=statsd.StatsClient)
+ stats = SafeStatsdLogger(statsd_client, validator(match_pattern))
- def test_increment_counter_with_allowed_prefix(self):
- self.stats.incr("stats_two.bla")
- self.statsd_client.incr.assert_called_once_with("stats_two.bla", 1, 1)
+ stats.incr(stat_name)
- def test_not_increment_counter_if_not_allowed(self):
- self.stats.incr("stats_three")
- self.statsd_client.assert_not_called()
+ if expect_incr:
+ statsd_client.incr.assert_called_once_with(stat_name, 1, 1)
+ else:
+ statsd_client.assert_not_called()
-class TestStatsWithBlockList:
- def setup_method(self):
- self.statsd_client = Mock(spec=statsd.StatsClient)
- self.stats = SafeStatsdLogger(self.statsd_client,
BlockListValidator("stats_one, stats_two"))
+class TestPatternOrBasicValidatorConfigOption:
+ def teardown_method(self):
+ # Avoid side-effects
+ importlib.reload(airflow.stats)
- def test_increment_counter_with_allowed_key(self):
- self.stats.incr("stats_one")
- self.statsd_client.assert_not_called()
+ stats_on = {("metrics", "statsd_on"): "True"}
+ pattern_on = {("metrics", "metrics_use_pattern_match"): "True"}
+ pattern_off = {("metrics", "metrics_use_pattern_match"): "False"}
+ allow_list = {("metrics", "metrics_allow_list"): "foo,bar"}
Review Comment:
to have more robust test, shouldn't you actually use regex when pattern
match is on? This test will pass irrespective of pattern match configuration
value.
--
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]
