ferruzzi commented on code in PR #41975:
URL: https://github.com/apache/airflow/pull/41975#discussion_r1742570262
##########
airflow/metrics/validators.py:
##########
@@ -118,7 +106,7 @@ def get_validator() -> ListValidator:
else:
list_type = DEFAULT_VALIDATOR_TYPE
- return validators[validator_type][list_type](metric_lists[list_type])
+ return validators["pattern"][list_type](metric_lists[list_type])
Review Comment:
If the above is accepted then also:
```suggestion
return validators[list_type](metric_lists[list_type])
```
##########
tests/core/test_stats.py:
##########
@@ -345,76 +326,48 @@ def test_regex_matches(self, match_pattern, expect_incr):
statsd_client.assert_not_called()
-class TestPatternOrBasicValidatorConfigOption:
+class TestPatternValidatorConfigOption:
def teardown_method(self):
# Avoid side-effects
importlib.reload(airflow.stats)
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"}
block_list = {("metrics", "metrics_block_list"): "foo,bar"}
@pytest.mark.parametrize(
Review Comment:
Did we miss a case here where both allow and block are provided and we
assert that allow is being used?
##########
tests/core/test_stats.py:
##########
@@ -345,76 +326,48 @@ def test_regex_matches(self, match_pattern, expect_incr):
statsd_client.assert_not_called()
-class TestPatternOrBasicValidatorConfigOption:
+class TestPatternValidatorConfigOption:
def teardown_method(self):
# Avoid side-effects
importlib.reload(airflow.stats)
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"}
block_list = {("metrics", "metrics_block_list"): "foo,bar"}
@pytest.mark.parametrize(
"config, expected",
[
pytest.param(
- {**stats_on, **pattern_on},
+ {**stats_on},
PatternAllowListValidator,
id="pattern_allow_by_default",
),
pytest.param(
- stats_on,
- AllowListValidator,
- id="basic_allow_by_default",
- ),
- pytest.param(
- {**stats_on, **pattern_on, **allow_list},
+ {**stats_on, **allow_list},
PatternAllowListValidator,
id="pattern_allow_list_provided",
),
pytest.param(
- {**stats_on, **pattern_off, **allow_list},
- AllowListValidator,
- id="basic_allow_list_provided",
- ),
- pytest.param(
- {**stats_on, **pattern_on, **block_list},
+ {**stats_on, **block_list},
PatternBlockListValidator,
id="pattern_block_list_provided",
),
- pytest.param(
- {**stats_on, **block_list},
- BlockListValidator,
- id="basic_block_list_provided",
- ),
],
)
- def test_pattern_or_basic_picker(self, config, expected):
+ def test_pattern_picker(self, config, expected):
with conf_vars(config):
importlib.reload(airflow.stats)
- if eval(config.get(("metrics", "metrics_use_pattern_match"),
"False")):
- assert isinstance(airflow.stats.Stats.statsd,
statsd.StatsClient)
- else:
- with pytest.warns(
- RemovedInAirflow3Warning,
- match="The basic metric validator will be deprecated in
the future in favor of pattern-matching. You can try this now by setting
config option metrics_use_pattern_match to True.",
- ):
- assert isinstance(airflow.stats.Stats.statsd,
statsd.StatsClient)
- assert isinstance(airflow.stats.Stats.instance.metrics_validator,
expected)
+ assert isinstance(airflow.stats.Stats.statsd, statsd.StatsClient)
+ assert type(airflow.stats.Stats.instance.metrics_validator) is
expected
@conf_vars({**stats_on, **block_list, ("metrics", "metrics_allow_list"):
"bax,qux"})
Review Comment:
Um akshully, that should be `baz` not `bax` :P That's my typo.
##########
airflow/metrics/validators.py:
##########
@@ -88,25 +88,13 @@ class MetricNameLengthExemptionWarning(Warning):
def get_validator() -> ListValidator:
validators = {
- "basic": {"allow": AllowListValidator, "block": BlockListValidator},
"pattern": {"allow": PatternAllowListValidator, "block":
PatternBlockListValidator},
Review Comment:
```suggestion
"allow": PatternAllowListValidator,
"block": PatternBlockListValidator,
```
I don't see a reason to keep the "pattern" level anymore.
--
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]
