You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "ferruzzi (via GitHub)" <gi...@apache.org> on 2023/12/15 23:45:40 UTC
[PR] Add a fuzzy match for metric allow and block list [airflow]
ferruzzi opened a new pull request, #36250:
URL: https://github.com/apache/airflow/pull/36250
Current metric allow and block lists allow the user to set which metric names to emit, but they only match on `startswith()`. This PR adds two new Validators - FuzzyAllow and FuzzyBlock, which match using regex - along with a config option to chooze between the "basic" or "fuzzy" match and unit testing.
While I was in there I also noticed that the OTel metrics never implemented Blocklist, it only had an AllowList. This could maybe have been a separate PR, but I fixed that bug while I was working on it since that would have required adjustments either way.
The new `get_validator()` method is a modified version of the decision logic previously used in the statsd logger, expanded to include the Fuzzy options, and then also applied to the otel logger.
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of an existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
<!-- Please keep an empty line above the dashes. -->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in a newsfragment file, named `{pr_number}.significant.rst` or `{issue_number}.significant.rst`, in [newsfragments](https://github.com/apache/airflow/tree/main/newsfragments).
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add a fuzzy/regex pattern-matching for metric allow and block list [airflow]
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala commented on code in PR #36250:
URL: https://github.com/apache/airflow/pull/36250#discussion_r1451021416
##########
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:
Since it's a new feature, we need to wait for the next minor release:
```suggestion
version_added: 2.9.0
```
##########
airflow/metrics/validators.py:
##########
@@ -85,6 +85,42 @@ class MetricNameLengthExemptionWarning(Warning):
BACK_COMPAT_METRIC_NAMES: set[Pattern[str]] = {re2.compile(name) for name in BACK_COMPAT_METRIC_NAME_PATTERNS}
OTEL_NAME_MAX_LENGTH = 63
+DEFAULT_VALIDATOR_TYPE = "allow"
+
+
+def get_validator() -> ListValidator:
+ validators = {
+ "basic": {"allow": AllowListValidator, "block": BlockListValidator},
+ "pattern": {"allow": PatternAllowListValidator, "block": PatternBlockListValidator},
+ }
+ metric_lists = {
+ "allow": (metric_allow_list := conf.get("metrics", "metrics_allow_list", fallback=None)),
+ "block": (metric_block_list := conf.get("metrics", "metrics_block_list", fallback=None)),
+ }
+
+ use_pattern = conf.getboolean("metrics", "metrics_use_pattern_match", fallback=False)
+ validator_type = "pattern" if use_pattern else "basic"
+
+ if not use_pattern:
+ warnings.warn(
+ "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.",
+ AirflowProviderDeprecationWarning,
Review Comment:
This is a part of Airflow core, you cannot use `AirflowProviderDeprecationWarning` warning:
```suggestion
RemovedInAirflow3Warning,
```
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add a fuzzy/regex pattern-matching for metric allow and block list [airflow]
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas commented on code in PR #36250:
URL: https://github.com/apache/airflow/pull/36250#discussion_r1450750079
##########
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),
+ ("s.{6}o", True),
+ ("^banana", False),
Review Comment:
Slight nit: all these patterns match from the beginning of the string either by forcing it with ^ or just by chance (with the middle one). Since we want to ensure this new feature can match in the middle, do you mind adding a regex that does so? I think you could actually just make the 6 a 5 and it would match in the middle.
##########
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 in the metric
+ name and not just start of the name.
Review Comment:
A suggestion to make this even more concrete and spelled out. Seems obvious to us working on it, but it's a slightly tricky one if you aren't familiar.
```suggestion
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.
```
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add a fuzzy match for metric allow and block list [airflow]
Posted by "shubham22 (via GitHub)" <gi...@apache.org>.
shubham22 commented on code in PR #36250:
URL: https://github.com/apache/airflow/pull/36250#discussion_r1428629011
##########
airflow/config_templates/config.yml:
##########
@@ -944,6 +944,14 @@ metrics:
description: |
StatsD (https://github.com/etsy/statsd) integration settings.
options:
+ metrics_use_fuzzy_match:
Review Comment:
I would also suggest renaming this to `metrics_enable_pattern_match` or `metrics_use_pattern_match`, in case we really decide to go with this.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add a fuzzy match for metric allow and block list [airflow]
Posted by "shubham22 (via GitHub)" <gi...@apache.org>.
shubham22 commented on code in PR #36250:
URL: https://github.com/apache/airflow/pull/36250#discussion_r1439775839
##########
airflow/config_templates/config.yml:
##########
@@ -944,6 +944,14 @@ metrics:
description: |
StatsD (https://github.com/etsy/statsd) integration settings.
options:
+ metrics_use_fuzzy_match:
Review Comment:
> then likely a bunch of confusion when the default changes after that.
This is the primary reason I think moving over to two new configs that replace old configs as better. This way when the old config is deprecated, users who adopted the new configs wouldn't have to make any change at all.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add a fuzzy match for metric allow and block list [airflow]
Posted by "ferruzzi (via GitHub)" <gi...@apache.org>.
ferruzzi commented on code in PR #36250:
URL: https://github.com/apache/airflow/pull/36250#discussion_r1449533923
##########
airflow/config_templates/config.yml:
##########
@@ -944,6 +944,14 @@ metrics:
description: |
StatsD (https://github.com/etsy/statsd) integration settings.
options:
+ metrics_use_fuzzy_match:
Review Comment:
Based on discussion here, I'm going to add a deprecation warning and rename "fuzzy matching" to "pattern matching".
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add a fuzzy/regex pattern-matching for metric allow and block list [airflow]
Posted by "ferruzzi (via GitHub)" <gi...@apache.org>.
ferruzzi commented on code in PR #36250:
URL: https://github.com/apache/airflow/pull/36250#discussion_r1450981651
##########
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:
Done, thanks
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add a fuzzy match for metric allow and block list [airflow]
Posted by "ferruzzi (via GitHub)" <gi...@apache.org>.
ferruzzi commented on PR #36250:
URL: https://github.com/apache/airflow/pull/36250#issuecomment-1887938252
Alright, based on feedback above, I am going to add a warning (once on launch) if `metrics_use_fuzzy_match` is false stating that the basic matching will be deprecated in the future and you can try the new pattern matching by setting that config option to True.
I also thought of two more unit tests which confirm the allow list can handle regex (both a pass and a fail case) and not just simple substring matching.
Based on the above feedback, I'm also renaming from "fuzzy matching" to "pattern matching".
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add a fuzzy/regex pattern-matching for metric allow and block list [airflow]
Posted by "ferruzzi (via GitHub)" <gi...@apache.org>.
ferruzzi commented on code in PR #36250:
URL: https://github.com/apache/airflow/pull/36250#discussion_r1451026339
##########
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:
Revised
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add a fuzzy match for metric allow and block list [airflow]
Posted by "ferruzzi (via GitHub)" <gi...@apache.org>.
ferruzzi commented on code in PR #36250:
URL: https://github.com/apache/airflow/pull/36250#discussion_r1430777847
##########
airflow/config_templates/config.yml:
##########
@@ -944,6 +944,14 @@ metrics:
description: |
StatsD (https://github.com/etsy/statsd) integration settings.
options:
+ metrics_use_fuzzy_match:
Review Comment:
That feels like a breaking change to me, no? What previously only matched the start of the metric name string will now match any substring in the name.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add a fuzzy match for metric allow and block list [airflow]
Posted by "ferruzzi (via GitHub)" <gi...@apache.org>.
ferruzzi commented on PR #36250:
URL: https://github.com/apache/airflow/pull/36250#issuecomment-1888134215
Slight git goof, sorry for the rebase and force push.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add a fuzzy match for metric allow and block list [airflow]
Posted by "ferruzzi (via GitHub)" <gi...@apache.org>.
ferruzzi commented on code in PR #36250:
URL: https://github.com/apache/airflow/pull/36250#discussion_r1431749360
##########
airflow/config_templates/config.yml:
##########
@@ -944,6 +944,14 @@ metrics:
description: |
StatsD (https://github.com/etsy/statsd) integration settings.
options:
+ metrics_use_fuzzy_match:
Review Comment:
@shubham22 - I'm not against the idea of deprecating but there needs to be some kind of extra config option in the interim. If we are going to deprecate then our best option (as I see it) is to keep `metric_allow_list` and `metric_block_list`, add a flag to pick "old style or new style", and add a deprecation message if that is set to "old style". Which is what I've done, minus the message. I can add the message if we don't want to have the option to keep the old way, but we're going to be stuck with this extra config option until (3.0??) and then likely a bunch of confusion when the default changes after that.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add a fuzzy match for metric allow and block list [airflow]
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala commented on code in PR #36250:
URL: https://github.com/apache/airflow/pull/36250#discussion_r1428787680
##########
airflow/config_templates/config.yml:
##########
@@ -944,6 +944,14 @@ metrics:
description: |
StatsD (https://github.com/etsy/statsd) integration settings.
options:
+ metrics_use_fuzzy_match:
Review Comment:
I like the idea of pattern match, we even don't need to rename the variable, we can just update the configuration description and explain that since 2.9.0, Airflow supports regex in `metrics_allow_list` and `metrics_block_list`.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add a fuzzy match for metric allow and block list [airflow]
Posted by "ferruzzi (via GitHub)" <gi...@apache.org>.
ferruzzi commented on code in PR #36250:
URL: https://github.com/apache/airflow/pull/36250#discussion_r1445258589
##########
airflow/config_templates/config.yml:
##########
@@ -944,6 +944,14 @@ metrics:
description: |
StatsD (https://github.com/etsy/statsd) integration settings.
options:
+ metrics_use_fuzzy_match:
Review Comment:
It looks like there are a number of "less-bad" solutions but no real good one. Does anyone feel particularly strongly about any of the options here?
If nobody has a strong opinion, I'm going to add "if metrics_use_fuzzy_match is false, display a deprecation notice" and leave it at that for now.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add a fuzzy match for metric allow and block list [airflow]
Posted by "shubham22 (via GitHub)" <gi...@apache.org>.
shubham22 commented on code in PR #36250:
URL: https://github.com/apache/airflow/pull/36250#discussion_r1428627753
##########
airflow/config_templates/config.yml:
##########
@@ -944,6 +944,14 @@ metrics:
description: |
StatsD (https://github.com/etsy/statsd) integration settings.
options:
+ metrics_use_fuzzy_match:
Review Comment:
I am personally against adding a new config for `metrics_use_fuzzy_match`. With this approach, we will keep sprawling the number of configurations every time we need to make a hard decision. I am of the opinion that we should have just 2 configs - `metrics_allow_list` and `metrics_block_list` - and they should be changed to support the pattern matching. That means, we should deprecate existing configs now and introduce new ones which are more powerful and support pattern matching, in order to keep it backward compatible. Yes, this means we need to go through deprecation until v3.0, but I think that is better for the product.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add a fuzzy/regex pattern-matching for metric allow and block list [airflow]
Posted by "shubham22 (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add a fuzzy/regex pattern-matching for metric allow and block list [airflow]
Posted by "ferruzzi (via GitHub)" <gi...@apache.org>.
ferruzzi commented on code in PR #36250:
URL: https://github.com/apache/airflow/pull/36250#discussion_r1450974993
##########
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:
There is a whole separate group of tests using regex patterns. This shows that you can just enter a string and it'll match anywhere in the name, and also optionally you can use regex patterns for the more complex stuff if you wish.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add a fuzzy/regex pattern-matching for metric allow and block list [airflow]
Posted by "ferruzzi (via GitHub)" <gi...@apache.org>.
ferruzzi commented on code in PR #36250:
URL: https://github.com/apache/airflow/pull/36250#discussion_r1450974993
##########
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:
Added a whole separate test using regex patterns. This shows that you can just enter a string and it'll match anywhere in the name, and also optionally you can use regex patterns for the more complex stuff if you wish.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add a fuzzy/regex pattern-matching for metric allow and block list [airflow]
Posted by "ferruzzi (via GitHub)" <gi...@apache.org>.
ferruzzi commented on code in PR #36250:
URL: https://github.com/apache/airflow/pull/36250#discussion_r1450994922
##########
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'll workshop a couple of ideas. Thanks for the suggestion
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add a fuzzy match for metric allow and block list [airflow]
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas commented on code in PR #36250:
URL: https://github.com/apache/airflow/pull/36250#discussion_r1445439794
##########
airflow/config_templates/config.yml:
##########
@@ -944,6 +944,14 @@ metrics:
description: |
StatsD (https://github.com/etsy/statsd) integration settings.
options:
+ metrics_use_fuzzy_match:
Review Comment:
I don't feel _too_ strongly, not willing to die on any hills here.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add a fuzzy match for metric allow and block list [airflow]
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas commented on code in PR #36250:
URL: https://github.com/apache/airflow/pull/36250#discussion_r1445117071
##########
airflow/config_templates/config.yml:
##########
@@ -944,6 +944,14 @@ metrics:
description: |
StatsD (https://github.com/etsy/statsd) integration settings.
options:
+ metrics_use_fuzzy_match:
Review Comment:
>This is the primary reason I think moving over to two new configs that replace old configs as better. This way when the old config is deprecated, users who adopted the new configs wouldn't have to make any change at all.
But now isn't this adding even more new configs than Dennis's original proposal?
Ultimately, I agree that we should change the definition of the current configs and add nothing new (zero new configs added) but we also do need _some_ way to give people fair warning. I think we should try do that with adding as few, if any, configs as possible. So I'm in slight favour of Dennis's proposal since it only adds one new config, which will be dropped after the deprecation process/switch over. Another approach could be a version check in the code that says something like "the behaviour of these configs will change by release <foo>, please be prepared to update your configuration at that time" (or a better message since I just spit balled that right now...) and uses the old beahviour if the code is before the release where we switch, and later releases it uses the new behaviour.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add a fuzzy match for metric allow and block list [airflow]
Posted by "ferruzzi (via GitHub)" <gi...@apache.org>.
ferruzzi commented on code in PR #36250:
URL: https://github.com/apache/airflow/pull/36250#discussion_r1430777847
##########
airflow/config_templates/config.yml:
##########
@@ -944,6 +944,14 @@ metrics:
description: |
StatsD (https://github.com/etsy/statsd) integration settings.
options:
+ metrics_use_fuzzy_match:
Review Comment:
@hussein-awala This feels like a breaking change to me, no? What previously only matched the start of the metric name string will now match any substring in the name.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add a fuzzy/regex pattern-matching for metric allow and block list [airflow]
Posted by "ferruzzi (via GitHub)" <gi...@apache.org>.
ferruzzi commented on code in PR #36250:
URL: https://github.com/apache/airflow/pull/36250#discussion_r1450759695
##########
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),
+ ("s.{6}o", True),
+ ("^banana", False),
Review Comment:
Gah, you are absolutely right. I intended to test that in the middle one and goofed. I'll adjust it to `a.{4}o` which verifies pattern matching (and not just substring) and matches not-the-beginning
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add a fuzzy/regex pattern-matching for metric allow and block list [airflow]
Posted by "ferruzzi (via GitHub)" <gi...@apache.org>.
ferruzzi commented on code in PR #36250:
URL: https://github.com/apache/airflow/pull/36250#discussion_r1450759695
##########
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),
+ ("s.{6}o", True),
+ ("^banana", False),
Review Comment:
Gah, you are absolutely right. I intended to test that in the middle one and goofed. I'll adjust it.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add a fuzzy/regex pattern-matching for metric allow and block list [airflow]
Posted by "ferruzzi (via GitHub)" <gi...@apache.org>.
ferruzzi merged PR #36250:
URL: https://github.com/apache/airflow/pull/36250
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org