Metrics Improvement Project
Pull Request Description
Summary:
This PR introduces the get_name method as an abstract method in the base metrics logger class. The get_name method has been implemented in both the StatsD and OTel metrics logger classes to handle metric name construction consistently across backends.
Note: The PR is not fully finished and it does not close #42881. I'm sending it to PR to see if I can proceed and finish it. The get_name method has been integrated into the incr method of the StatsD logger, but I have not applied it to other methods yet. I'm doing this to verify whether it aligns with the proposed functionality before further updates.
Changes:
- Added the
get_nameabstract method to the base metrics logger interface. - Implemented the
get_namemethod in the StatsD logger class to concatenate tags with the metric name. - Implemented the
get_namemethod in the OTel logger class to return the metric name directly. - Updated the
incrmethod in the StatsD class to utilize theget_namemethod for constructing metric names.
Please review the changes and provide feedback!
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
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.
@howardyoo - Can you have a look when you get time, this is something we discussed a while back.
It's a good start.
- We need to use get_name in all the other methods as well (
decr(),gauge(), etc) which you already said. - Please add unit tests to make sure the get_name methods you implemented are returning as expected.
- We still need to run the returned name through self.metrics_validator to see if it is on the allow or block list. You could perhaps add that to the abstract class and call super() in the implementation classes, or you could add it back where it was in the individual implemented methods (incr, decr, etc)
@howardyoo - Can you have a look when you get time, this is something we discussed a while back.
Hey sorry about the delay. I took a look and the implementation should properly work for both statsd side and otel side.
It's a good start.
- We need to use get_name in all the other methods as well (
decr(),gauge(), etc) which you already said.- Please add unit tests to make sure the get_name methods you implemented are returning as expected.
- We still need to run the returned name through self.metrics_validator to see if it is on the allow or block list. You could perhaps add that to the abstract class and call super() in the implementation classes, or you could add it back where it was in the individual implemented methods (incr, decr, etc)
Yes, I believe implementing unit test on get_name would be required.