[GitHub] [airflow] craigrosie commented on a change in pull request #7227: [AIRFLOW-6530] Allow Custom Statsd Client

[GitBox <gi...@apache.org>]

craigrosie commented on a change in pull request #7227: [AIRFLOW-6530] Allow Custom Statsd Client
URL: https://github.com/apache/airflow/pull/7227#discussion_r391939968
 
 

 ##########
 File path: airflow/stats.py
 ##########
 @@ -200,10 +200,35 @@ def __init__(self, *args, **kwargs):
                     self.__class__.instance = DummyStatsLogger()
             except (socket.gaierror, ImportError) as e:
                 log.warning("Could not configure StatsClient: %s, using DummyStatsLogger instead.", e)
+                self.__class__.instance = DummyStatsLogger()
 
 Review comment:
   I think I found a bug when adding tests for the new functionality. The log message above the line I added states that the class used for the statsd client should fall back to `DummyStatsLogger` if the defined statsd client class cannot be imported, or cannot connect to a statsd instance, but this never happens, and `self.__class__.instance` remains set to the default value of `None`
   
   For example, if I try to load an invalid custom statsd class using the functionality that @envious added, triggering an `ImportError`, when I then try and call `Stats.incr()` (which should still work because the underlying class has fallen back to `DummyStatsLogger`), I get errors like:
   ```python
   cls = <class 'airflow.stats.Stats'>, name = 'incr'
   
       def __getattr__(cls, name):
   >       return getattr(cls.instance, name)
   E       AttributeError: 'NoneType' object has no attribute 'incr'
   
   airflow/stats.py:188: AttributeError
   ```

----------------------------------------------------------------
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


With regards,
Apache Git Services