From ecd90634248ce495dd1dca41c77254ce7877f6db Mon Sep 17 00:00:00 2001 From: Richard Belleville Date: Wed, 7 Nov 2018 11:28:52 -0800 Subject: [PATCH] Use single NullHandler for whole library I was trying to get a feel for what the rest of the python ecosystem does with its logging, so I looked into the top few libraries on pypi: urllib3 maintains a logger for not quite every module, but for each one that does heavy lifting. The logger name is `__name__`, and no handlers are registered for any module-level loggers, including NullHandler. Their documentation spells out how to configure logging for the library. They explicitly configure a library root-level logger called `urllib3` to which they attach a `NullHandler`. This addresses the "no handlers could be found" problem. Their tests explicitly configure handlers, just like ours do. scrapy is more hands-on. It provides a configuration module for its logging and a whole document on how to handle logging with scrapy. It looks like log.py's whole reason for existence is making sure that a handler is attached to to the scrapy handler at startup. I think the extra complexity here is because scrapy also offers a CLI, so there has to be some way to configure logging without resorting to writing python, so I felt we didn't need to resort to this added complexity. --- Based on all of the libraries I've looked at, I think our current approach is reasonable. The one change I would make is to explicitly configure a `grpc` logger and to only attach a `NullHandler` to it instead of putting the burden on the author of each new module to configure it there. With this change, I have - Configured a logger in each module that cares about logging - Removed all NullHandlers attached to module-level loggers - Explicitly configured a `grpc` logger with a `NullHandler` attached Resolves: #16572 Related: #17064 --- src/python/grpcio/grpc/__init__.py | 4 +++- src/python/grpcio/grpc/_channel.py | 1 - src/python/grpcio/grpc/_common.py | 1 - src/python/grpcio/grpc/_cython/_cygrpc/grpc_string.pyx.pxi | 1 - src/python/grpcio/grpc/_cython/_cygrpc/server.pyx.pxi | 1 - src/python/grpcio/grpc/_plugin_wrapping.py | 1 - .../grpcio/grpc/framework/foundation/callable_util.py | 1 - .../grpcio/grpc/framework/foundation/logging_pool.py | 1 - src/python/grpcio/grpc/framework/foundation/stream_util.py | 1 - src/python/grpcio_tests/tests/unit/_logging_test.py | 7 +++++++ 10 files changed, 10 insertions(+), 9 deletions(-) diff --git a/src/python/grpcio/grpc/__init__.py b/src/python/grpcio/grpc/__init__.py index df98dd10ad1..8c15e540825 100644 --- a/src/python/grpcio/grpc/__init__.py +++ b/src/python/grpcio/grpc/__init__.py @@ -15,12 +15,14 @@ import abc import enum +import logging import sys - import six from grpc._cython import cygrpc as _cygrpc +logging.getLogger(__name__).addHandler(logging.NullHandler()) + ############################## Future Interface ############################### diff --git a/src/python/grpcio/grpc/_channel.py b/src/python/grpcio/grpc/_channel.py index 3ff76587484..ab154d85121 100644 --- a/src/python/grpcio/grpc/_channel.py +++ b/src/python/grpcio/grpc/_channel.py @@ -25,7 +25,6 @@ from grpc._cython import cygrpc from grpc.framework.foundation import callable_util _LOGGER = logging.getLogger(__name__) -_LOGGER.addHandler(logging.NullHandler()) _USER_AGENT = 'grpc-python/{}'.format(_grpcio_metadata.__version__) diff --git a/src/python/grpcio/grpc/_common.py b/src/python/grpcio/grpc/_common.py index 42f3a4e6147..f69127e38ef 100644 --- a/src/python/grpcio/grpc/_common.py +++ b/src/python/grpcio/grpc/_common.py @@ -21,7 +21,6 @@ import grpc from grpc._cython import cygrpc _LOGGER = logging.getLogger(__name__) -_LOGGER.addHandler(logging.NullHandler()) CYGRPC_CONNECTIVITY_STATE_TO_CHANNEL_CONNECTIVITY = { cygrpc.ConnectivityState.idle: diff --git a/src/python/grpcio/grpc/_cython/_cygrpc/grpc_string.pyx.pxi b/src/python/grpcio/grpc/_cython/_cygrpc/grpc_string.pyx.pxi index fa356d913e2..00a1b23a67b 100644 --- a/src/python/grpcio/grpc/_cython/_cygrpc/grpc_string.pyx.pxi +++ b/src/python/grpcio/grpc/_cython/_cygrpc/grpc_string.pyx.pxi @@ -15,7 +15,6 @@ import logging _LOGGER = logging.getLogger(__name__) -_LOGGER.addHandler(logging.NullHandler()) # This function will ascii encode unicode string inputs if neccesary. # In Python3, unicode strings are the default str type. diff --git a/src/python/grpcio/grpc/_cython/_cygrpc/server.pyx.pxi b/src/python/grpcio/grpc/_cython/_cygrpc/server.pyx.pxi index f9d1e863ca9..ce701724fd3 100644 --- a/src/python/grpcio/grpc/_cython/_cygrpc/server.pyx.pxi +++ b/src/python/grpcio/grpc/_cython/_cygrpc/server.pyx.pxi @@ -19,7 +19,6 @@ import time import grpc _LOGGER = logging.getLogger(__name__) -_LOGGER.addHandler(logging.NullHandler()) cdef class Server: diff --git a/src/python/grpcio/grpc/_plugin_wrapping.py b/src/python/grpcio/grpc/_plugin_wrapping.py index 53af2ff9135..916ee080b60 100644 --- a/src/python/grpcio/grpc/_plugin_wrapping.py +++ b/src/python/grpcio/grpc/_plugin_wrapping.py @@ -21,7 +21,6 @@ from grpc import _common from grpc._cython import cygrpc _LOGGER = logging.getLogger(__name__) -_LOGGER.addHandler(logging.NullHandler()) class _AuthMetadataContext( diff --git a/src/python/grpcio/grpc/framework/foundation/callable_util.py b/src/python/grpcio/grpc/framework/foundation/callable_util.py index 36066e19df5..24daf3406f8 100644 --- a/src/python/grpcio/grpc/framework/foundation/callable_util.py +++ b/src/python/grpcio/grpc/framework/foundation/callable_util.py @@ -22,7 +22,6 @@ import logging import six _LOGGER = logging.getLogger(__name__) -_LOGGER.addHandler(logging.NullHandler()) class Outcome(six.with_metaclass(abc.ABCMeta)): diff --git a/src/python/grpcio/grpc/framework/foundation/logging_pool.py b/src/python/grpcio/grpc/framework/foundation/logging_pool.py index dfb8dbdc30a..216e3990db0 100644 --- a/src/python/grpcio/grpc/framework/foundation/logging_pool.py +++ b/src/python/grpcio/grpc/framework/foundation/logging_pool.py @@ -18,7 +18,6 @@ import logging from concurrent import futures _LOGGER = logging.getLogger(__name__) -_LOGGER.addHandler(logging.NullHandler()) def _wrap(behavior): diff --git a/src/python/grpcio/grpc/framework/foundation/stream_util.py b/src/python/grpcio/grpc/framework/foundation/stream_util.py index e03130cced7..1faaf29bd7e 100644 --- a/src/python/grpcio/grpc/framework/foundation/stream_util.py +++ b/src/python/grpcio/grpc/framework/foundation/stream_util.py @@ -20,7 +20,6 @@ from grpc.framework.foundation import stream _NO_VALUE = object() _LOGGER = logging.getLogger(__name__) -_LOGGER.addHandler(logging.NullHandler()) class TransformingConsumer(stream.Consumer): diff --git a/src/python/grpcio_tests/tests/unit/_logging_test.py b/src/python/grpcio_tests/tests/unit/_logging_test.py index 80b1f1b3c12..631b9de9db5 100644 --- a/src/python/grpcio_tests/tests/unit/_logging_test.py +++ b/src/python/grpcio_tests/tests/unit/_logging_test.py @@ -68,6 +68,13 @@ class LoggingTest(unittest.TestCase): self.assertEqual(1, len(logging.getLogger().handlers)) self.assertIs(logging.getLogger().handlers[0].stream, intended_stream) + @isolated_logging + def test_grpc_logger(self): + self.assertIn("grpc", logging.Logger.manager.loggerDict) + root_logger = logging.getLogger("grpc") + self.assertEqual(1, len(root_logger.handlers)) + self.assertIsInstance(root_logger.handlers[0], logging.NullHandler) + if __name__ == '__main__': unittest.main(verbosity=2)