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:
[email protected]
