You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2020/11/07 12:05:47 UTC
[GitHub] [airflow] mik-laj opened a new pull request #12158: StatsD metrics have tags + generate reference docs
mik-laj opened a new pull request #12158:
URL: https://github.com/apache/airflow/pull/12158
<!--
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 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/
-->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+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 [UPDATING.md](https://github.com/apache/airflow/blob/master/UPDATING.md).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #12158: StatsD metrics have tags + generate reference docs
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #12158:
URL: https://github.com/apache/airflow/pull/12158#discussion_r519178143
##########
File path: airflow/models/dagbag.py
##########
@@ -471,7 +471,7 @@ def collect_dags(
# file_stat.file similar format: /subdir/dag_name.py
# TODO: Remove for Airflow 2.0
filename = file_stat.file.split('/')[-1].replace('.py', '')
- Stats.timing(f'dag.loading-duration.{filename}', file_stat.duration)
+ Stats.timing('dag.loading-duration.{filename}', file_stat.duration, labels={"filename": filename})
Review comment:
The only problem is that not every client supports tags, and some clients support tags in a different way than the official Datadog. For this reason, I preferred not to add more keys, but leave these decisions to the statsd client.
If the client supports the tags, it will keep metrics in the best form for itself.
We currently have two clients:
- Classic statsd client (without tags support), which in this case will get a metric - `dag.loading-duration.my-awesome-file.py` without tags
- Datadog statsd (with tags support), which in this in this case will get a metric - `dag.loading-duration.filename` with tag `filename:my-awesome-file.py`
So you can aggregate this data if you need it.
This key name is not ideal when the client supports tags, but we don't need to generate the metrics key twice. On the other hand, writing the available parameters in the name of the metrics is not a stupid idea. This can help us find the tags more easily.
It is also worth adding that Stackdriver stores labels differently. Instead of tags, it uses a dictionary that has a normal key and 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #12158: StatsD metrics have tags + generate reference docs
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #12158:
URL: https://github.com/apache/airflow/pull/12158#discussion_r529428778
##########
File path: airflow/stats.py
##########
@@ -191,93 +443,121 @@ def test(self, stat):
return True # default is all metrics allowed
+def _format_safe_stats_logger_args(func):
+ @functools.wraps(func)
+ def func_wrap(_self, stat, *args, labels=None, **kwargs):
+ if labels:
+ # Remove {} from stat key e.g. {class_name} => class_name
+ stat = stat.format(**labels)
+ return validate_stat(func)(_self, stat, *args, **kwargs)
+
+ return func_wrap
+
+
class SafeStatsdLogger:
"""Statsd Logger"""
def __init__(self, statsd_client, allow_list_validator=AllowListValidator()):
self.statsd = statsd_client
self.allow_list_validator = allow_list_validator
- @validate_stat
- def incr(self, stat, count=1, rate=1):
+ @_format_safe_stats_logger_args
+ def incr(self, stat, count=1, rate=1, labels=None):
"""Increment stat"""
+ del labels
Review comment:
What's this for?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #12158: StatsD metrics have tags + generate reference docs
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #12158:
URL: https://github.com/apache/airflow/pull/12158#discussion_r529514696
##########
File path: airflow/stats.py
##########
@@ -191,93 +443,121 @@ def test(self, stat):
return True # default is all metrics allowed
+def _format_safe_stats_logger_args(func):
+ @functools.wraps(func)
+ def func_wrap(_self, stat, *args, labels=None, **kwargs):
+ if labels:
+ # Remove {} from stat key e.g. {class_name} => class_name
+ stat = stat.format(**labels)
+ return validate_stat(func)(_self, stat, *args, **kwargs)
+
+ return func_wrap
+
+
class SafeStatsdLogger:
"""Statsd Logger"""
def __init__(self, statsd_client, allow_list_validator=AllowListValidator()):
self.statsd = statsd_client
self.allow_list_validator = allow_list_validator
- @validate_stat
- def incr(self, stat, count=1, rate=1):
+ @_format_safe_stats_logger_args
+ def incr(self, stat, count=1, rate=1, labels=None):
"""Increment stat"""
+ del labels
Review comment:
Is this actually needed? Because of the _format_safe_stats_logger_args decorator, this is never called with any value:
```
def func_wrap(_self, stat, *args, labels=None, **kwargs):
if labels:
# Remove {} from stat key e.g. {class_name} => class_name
stat = stat.format(**labels)
return validate_stat(func)(_self, stat, *args, **kwargs)
```
`labels` is an explicit parameter, so won't be in the kwargs passed to this function (`func` in that snippet)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #12158: StatsD metrics have tags + generate reference docs
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #12158:
URL: https://github.com/apache/airflow/pull/12158#discussion_r519174035
##########
File path: airflow/models/dagbag.py
##########
@@ -471,7 +471,7 @@ def collect_dags(
# file_stat.file similar format: /subdir/dag_name.py
# TODO: Remove for Airflow 2.0
filename = file_stat.file.split('/')[-1].replace('.py', '')
- Stats.timing(f'dag.loading-duration.{filename}', file_stat.duration)
+ Stats.timing('dag.loading-duration.{filename}', file_stat.duration, labels={"filename": filename})
Review comment:
Should we also add (on top of existing ones) new metrics that have just "short" name and the variables are only passed as labels?
For example in this case:
```
Stats.timing('dag.loading-duration', file_stat.duration, labels={"filename": filename})
```
I think those will be much easier to aggregate in some tools.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #12158: StatsD metrics have tags + generate reference docs
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #12158:
URL: https://github.com/apache/airflow/pull/12158#issuecomment-787558774
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Acehaidrey commented on pull request #12158: StatsD metrics have tags + generate reference docs
Posted by GitBox <gi...@apache.org>.
Acehaidrey commented on pull request #12158:
URL: https://github.com/apache/airflow/pull/12158#issuecomment-739709963
I honestly think this is great. I'm not sure how to capture this as we use opentsdb and have an internal client for it, so I had to make changes to all of these stat calls to match the same signature and use tags instead of the `a.b.c`. This will help make this easier to manage. It seems graphite uses that specific notation to do aggregation, whereas other clients use tags for that same result.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #12158: StatsD metrics have tags + generate reference docs
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #12158:
URL: https://github.com/apache/airflow/pull/12158#discussion_r529430812
##########
File path: airflow/stats.py
##########
@@ -191,93 +443,121 @@ def test(self, stat):
return True # default is all metrics allowed
+def _format_safe_stats_logger_args(func):
+ @functools.wraps(func)
+ def func_wrap(_self, stat, *args, labels=None, **kwargs):
+ if labels:
+ # Remove {} from stat key e.g. {class_name} => class_name
+ stat = stat.format(**labels)
+ return validate_stat(func)(_self, stat, *args, **kwargs)
+
+ return func_wrap
+
+
class SafeStatsdLogger:
"""Statsd Logger"""
def __init__(self, statsd_client, allow_list_validator=AllowListValidator()):
self.statsd = statsd_client
self.allow_list_validator = allow_list_validator
- @validate_stat
- def incr(self, stat, count=1, rate=1):
+ @_format_safe_stats_logger_args
Review comment:
I'd rather we didn't have to do string processing every time we emitted a metric, given this is "static"/precomputable.
How much work would it be to have this done once in get_statsd_logger/get_dogstatsd_logger?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] williamBartos commented on pull request #12158: StatsD metrics have tags + generate reference docs
Posted by GitBox <gi...@apache.org>.
williamBartos commented on pull request #12158:
URL: https://github.com/apache/airflow/pull/12158#issuecomment-807482853
Are there plans to still merge 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] closed pull request #12158: StatsD metrics have tags + generate reference docs
Posted by GitBox <gi...@apache.org>.
github-actions[bot] closed pull request #12158:
URL: https://github.com/apache/airflow/pull/12158
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #12158: StatsD metrics have tags + generate reference docs
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #12158:
URL: https://github.com/apache/airflow/pull/12158#discussion_r529441147
##########
File path: airflow/stats.py
##########
@@ -191,93 +443,121 @@ def test(self, stat):
return True # default is all metrics allowed
+def _format_safe_stats_logger_args(func):
+ @functools.wraps(func)
+ def func_wrap(_self, stat, *args, labels=None, **kwargs):
+ if labels:
+ # Remove {} from stat key e.g. {class_name} => class_name
+ stat = stat.format(**labels)
+ return validate_stat(func)(_self, stat, *args, **kwargs)
+
+ return func_wrap
+
+
class SafeStatsdLogger:
"""Statsd Logger"""
def __init__(self, statsd_client, allow_list_validator=AllowListValidator()):
self.statsd = statsd_client
self.allow_list_validator = allow_list_validator
- @validate_stat
- def incr(self, stat, count=1, rate=1):
+ @_format_safe_stats_logger_args
Review comment:
Edit: I thought that we were using the METRIC_LIST to emit stats (hence the pre-computing would be possible) but we aren't, so this can't be done.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #12158: StatsD metrics have tags + generate reference docs
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #12158:
URL: https://github.com/apache/airflow/pull/12158#discussion_r529432092
##########
File path: airflow/stats.py
##########
@@ -191,93 +443,121 @@ def test(self, stat):
return True # default is all metrics allowed
+def _format_safe_stats_logger_args(func):
+ @functools.wraps(func)
+ def func_wrap(_self, stat, *args, labels=None, **kwargs):
+ if labels:
+ # Remove {} from stat key e.g. {class_name} => class_name
Review comment:
```suggestion
# Remove {} from stat key e.g. {class_name} => SchedulerJob
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #12158: StatsD metrics have tags + generate reference docs
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #12158:
URL: https://github.com/apache/airflow/pull/12158#issuecomment-731713005
some confflicts - but it's good for me.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] arbass22 commented on a change in pull request #12158: StatsD metrics have tags + generate reference docs
Posted by GitBox <gi...@apache.org>.
arbass22 commented on a change in pull request #12158:
URL: https://github.com/apache/airflow/pull/12158#discussion_r586697071
##########
File path: airflow/models/taskinstance.py
##########
@@ -1255,11 +1258,13 @@ def signal_handler(signum, frame): # pylint: disable=unused-argument
end_time = time.time()
duration = timedelta(seconds=end_time - start_time)
+ dag_id = task_copy.dag_id
+ task_id = task_copy.task_id
Stats.timing(
- f'dag.{task_copy.dag_id}.{task_copy.task_id}.duration',
- duration,
+ 'dag.{dag_id}.{task_id}.duration', duration, labels={"dag_id": dag_id, "task_id": task_id}
)
- Stats.incr(f'operator_successes_{self.task.task_type}', 1, 1)
+ task_type = self.task.task_type
+ Stats.incr('operator_successes_{task_type}', 1, 1, labels={"task_type": task_type})
Stats.incr('ti_successes')
Review comment:
Would it be possible to add the dag/task tags to ti_successes and ti_failures as well? I think this would useful for aggregation so we can ignore or watch specific dags for failures
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #12158: StatsD metrics have tags + generate reference docs
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #12158:
URL: https://github.com/apache/airflow/pull/12158#discussion_r529432596
##########
File path: airflow/stats.py
##########
@@ -191,93 +443,121 @@ def test(self, stat):
return True # default is all metrics allowed
+def _format_safe_stats_logger_args(func):
+ @functools.wraps(func)
+ def func_wrap(_self, stat, *args, labels=None, **kwargs):
+ if labels:
+ # Remove {} from stat key e.g. {class_name} => class_name
+ stat = stat.format(**labels)
+ return validate_stat(func)(_self, stat, *args, **kwargs)
+
+ return func_wrap
+
+
class SafeStatsdLogger:
"""Statsd Logger"""
def __init__(self, statsd_client, allow_list_validator=AllowListValidator()):
self.statsd = statsd_client
self.allow_list_validator = allow_list_validator
- @validate_stat
- def incr(self, stat, count=1, rate=1):
+ @_format_safe_stats_logger_args
Review comment:
Oh, it is for dogstatsd, but not for statsd.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #12158: StatsD metrics have tags + generate reference docs
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #12158:
URL: https://github.com/apache/airflow/pull/12158#discussion_r529438705
##########
File path: airflow/stats.py
##########
@@ -30,6 +31,245 @@
log = logging.getLogger(__name__)
+class MetricType(str, enum.Enum):
+ """Metrics types."""
+
+ COUNTER = "COUNTER"
+ GAUGE = "GAUGE"
+ TIMER = "TIMER"
+
+
+class Metric(NamedTuple):
+ """StatsD metrics definition"""
+
+ metric_type: str
+ key: str
+ description: str
+
+
+METRICS_LIST: List[Metric] = [
Review comment:
I'm not sure the purpose of this List, nor the Metric class -- it appears this is never used for emitting metrics at runtime, so I'm not sure why we need this?
What was your thinking here please?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #12158: StatsD metrics have tags + generate reference docs
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #12158:
URL: https://github.com/apache/airflow/pull/12158#discussion_r529499699
##########
File path: airflow/stats.py
##########
@@ -191,93 +443,121 @@ def test(self, stat):
return True # default is all metrics allowed
+def _format_safe_stats_logger_args(func):
+ @functools.wraps(func)
+ def func_wrap(_self, stat, *args, labels=None, **kwargs):
+ if labels:
+ # Remove {} from stat key e.g. {class_name} => class_name
+ stat = stat.format(**labels)
+ return validate_stat(func)(_self, stat, *args, **kwargs)
+
+ return func_wrap
+
+
class SafeStatsdLogger:
"""Statsd Logger"""
def __init__(self, statsd_client, allow_list_validator=AllowListValidator()):
self.statsd = statsd_client
self.allow_list_validator = allow_list_validator
- @validate_stat
- def incr(self, stat, count=1, rate=1):
+ @_format_safe_stats_logger_args
+ def incr(self, stat, count=1, rate=1, labels=None):
"""Increment stat"""
+ del labels
Review comment:
This prevents an error about an unused variable and at the same time we do not have a poorly descriptive variable `_`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on pull request #12158: StatsD metrics have tags + generate reference docs
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #12158:
URL: https://github.com/apache/airflow/pull/12158#issuecomment-726026676
@Acehaidrey do you have any thoughts on this change?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kosteev commented on a change in pull request #12158: StatsD metrics have tags + generate reference docs
Posted by GitBox <gi...@apache.org>.
kosteev commented on a change in pull request #12158:
URL: https://github.com/apache/airflow/pull/12158#discussion_r521332630
##########
File path: airflow/models/dagbag.py
##########
@@ -471,7 +471,7 @@ def collect_dags(
# file_stat.file similar format: /subdir/dag_name.py
# TODO: Remove for Airflow 2.0
filename = file_stat.file.split('/')[-1].replace('.py', '')
- Stats.timing(f'dag.loading-duration.{filename}', file_stat.duration)
+ Stats.timing('dag.loading-duration.{filename}', file_stat.duration, labels={"filename": filename})
Review comment:
Hi guys.
Usually, best practice is to have generic metric names and use tags for any variable parameters, that it will make it possible to do all possible breakdowns, slice and dice etc. in the monitoring tool (if it supports it of course).
With that said, even if it is a weird name I actually do not see problem to have metric name like `dag.loading-duration.filename` with `tags=['filename={filename}']`, if this is going to simplify implementation/support/usage.
However, there is one caveat. What if some time later we are going to extend metric with additional tags, e.g. for metric `operator_failures_{task_type}` we would like to add one more tag like `dag_id`. Maybe not a very good example, but I am pretty sure for some metrics we could put more information that will be helpful to build breakdown by.
Then we actually will have to change name of the metric, that will be a breaking change, cause all existing setup alerts/dashboards will stop to work (or we will have to support two metrics from now `operator_failures_{task_type}` + `operator_failures_{task_type}_{dag_id}`, which is not also ideal).
So, as for me, actually omitting placeholders in names like `operator_failures`/`dag.loading-duration`/... and using tags should be the best for clients which support tags and want to leverage them. Unless we are sure that we will not add more tags to existing metrics, which can be not the true.
Then it seems like having maybe two type of metrics, one without tags, other with tags without placeholders in the names might be a good solution.
But maybe this is not an issue. What do you think guys about the fact that metrics can be extended with additional tags in the future.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #12158: StatsD metrics have tags + generate reference docs
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #12158:
URL: https://github.com/apache/airflow/pull/12158#discussion_r519174269
##########
File path: airflow/models/dagbag.py
##########
@@ -471,7 +471,7 @@ def collect_dags(
# file_stat.file similar format: /subdir/dag_name.py
# TODO: Remove for Airflow 2.0
filename = file_stat.file.split('/')[-1].replace('.py', '')
- Stats.timing(f'dag.loading-duration.{filename}', file_stat.duration)
+ Stats.timing('dag.loading-duration.{filename}', file_stat.duration, labels={"filename": filename})
Review comment:
Or maybe we should leave the old stats as they are and only add labels in the new ones ? WDYT? We could even add some prefix/indication that those stats are the "labeled" ones and distinguish them in the stats documentation ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #12158: StatsD metrics have tags + generate reference docs
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #12158:
URL: https://github.com/apache/airflow/pull/12158#discussion_r519174269
##########
File path: airflow/models/dagbag.py
##########
@@ -471,7 +471,7 @@ def collect_dags(
# file_stat.file similar format: /subdir/dag_name.py
# TODO: Remove for Airflow 2.0
filename = file_stat.file.split('/')[-1].replace('.py', '')
- Stats.timing(f'dag.loading-duration.{filename}', file_stat.duration)
+ Stats.timing('dag.loading-duration.{filename}', file_stat.duration, labels={"filename": filename})
Review comment:
Or maybe we should leave the old stats as they are and only add labels in the new ones ? WDYT?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #12158: StatsD metrics have tags + generate reference docs
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #12158:
URL: https://github.com/apache/airflow/pull/12158#discussion_r519179322
##########
File path: airflow/models/dagbag.py
##########
@@ -471,7 +471,7 @@ def collect_dags(
# file_stat.file similar format: /subdir/dag_name.py
# TODO: Remove for Airflow 2.0
filename = file_stat.file.split('/')[-1].replace('.py', '')
- Stats.timing(f'dag.loading-duration.{filename}', file_stat.duration)
+ Stats.timing('dag.loading-duration.{filename}', file_stat.duration, labels={"filename": filename})
Review comment:
I have added documentation that describes this behavior.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org