chore: add bare-minimum logging support #2204

JohnVillalovos · 2022-07-30T20:23:13Z

Follow the Python documentation guidelines for "Configuring Logging
for a Library" [1]

Which is adding these two lines:
import logging
logging.getLogger(name).addHandler(logging.NullHandler())

Setup a very basic usage of logging in gitlab/client.py

By using the NullHandler it means that by default any log messages
output will not be displayed. It is up to the client application to do
a logging.basicConfig() call to get log messages to display.

[1] https://docs.python.org/3/howto/logging.html#configuring-logging-for-a-library
Related: #2080

codecov-commenter · 2022-07-30T20:33:48Z

Codecov Report

Merging #2204 (b476afe) into main (7a8a862) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #2204   +/-   ##
=======================================
  Coverage   96.18%   96.18%           
=======================================
  Files          87       88    +1     
  Lines        5683     5692    +9     
=======================================
+ Hits         5466     5475    +9     
  Misses        217      217

Flag	Coverage Δ
api_func_v4	`82.53% <90.00%> (+0.02%)`	⬆️
cli_func_v4	`83.01% <90.00%> (+0.02%)`	⬆️
unit	`87.66% <100.00%> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
gitlab/__init__.py	`100.00% <100.00%> (ø)`
gitlab/_logging.py	`100.00% <100.00%> (ø)`
gitlab/client.py	`98.80% <100.00%> (+<0.01%)`	⬆️

nejch

Thanks @JohnVillalovos, just had a few questions!

nejch · 2022-08-07T22:33:47Z

gitlab/client.py

@@ -16,6 +17,8 @@
 import gitlab.exceptions
 from gitlab import utils

+LOG = logging.getLogger(__name__)


Maybe we can just use logger as suggested in the linked Python docs? Just visually also seems to make sense to me calling methods on it like that:

Suggested change

LOG = logging.getLogger(__name__)

logger = logging.getLogger(__name__)

Since it is a global I prefer the LOG. Also in the past I have used logger but then found myself accidentally typing logging and not noticing what went wrong for awhile :(

Plus it is the way OpenStack does it:
https://github.com/openstack/ironic/blob/0659485d630b8651faa633f98b1802bdf244f186/ironic/drivers/modules/ipmitool.py#L61

As an example it is easy to do:

logging.debug("some message") when you really meant to do logger.debug("some message")

Which can cause problems. As I unfortunately know from my own experience.

Ok, it was just an idea as it is a Logger instance that we manipulate and not a constant as LOG would imply. I guess log would also be somewhere in between.

What do you think of the name LOGGER? For me it is the middle ground.

nejch · 2022-08-07T22:43:18Z

gitlab/_logging.py

+# output unless the client application configures logging. For example by
+# calling `logging.basicConfig()`
+_module_root_logger_name = __name__.split(".", maxsplit=1)[0]
+logging.getLogger(_module_root_logger_name).addHandler(logging.NullHandler())


I mght be misremembering but thought I saw an earlier version of this PR where I liked the simplicity of just doing this in __init__.py a bit more - especially if this is extra module is being done just for testing. requests and a lot of others do that too just in __init__.py. Maybe we can look at other ways of testing if needed (or we can add more testing later)?

Added a docstring to explain why doing it here.

To me it just shifts the issue to imports if something happens to be imported e.g. import _clients would go before this. So I thought there might be a simple way for this, as I don't really see it done in popular python libraries. I know some use get_logger() helpers to lazily load loggers, maybe that's the idea here and I'm not getting it 😁

I probably don't exactly understand the risks here for our own codebase or how you stumbled across this issue, so it would just be completely subjective from my side, maybe @max-wittig or @lmilbaum would be better to take a look here :P

@nejch Not sure I am following the discussion. Will take a deeper look at it in few hours.

I've done some reading to figure out what the underscore means in the file name and its impact on time those lines of code are executed. Would you mind helping me figure this out?

I think it's to indicate a private module, and it is imported here to ensure the code is evaluated before any other logging code. But I'll let John explain this :)

gitlab/client.py

nejch

Just got back to this @JohnVillalovos, sorry for the roundabout for the small change. A few more questions and I don't understand fully the issue I think, maybe others have an opinion.

nejch · 2022-11-01T21:48:33Z

tests/unit/test__logging.py

+@pytest.fixture
+def LOG():
+    return logging.getLogger(_logging._module_root_logger_name)
+
+
+def test_module_root_logger_name():
+    assert _logging._module_root_logger_name == "gitlab"
+
+
+def test_module_name(LOG):
+    assert LOG.name == "gitlab"


Just a thought, but how about just doing a simple logging.getLogger("gitlab") in our code as that would make all the string manipulation and tests unnecessary.

If the package name changes, users would have much bigger problems than this, as they would not be able to import gitlab.

nejch · 2022-11-01T21:55:10Z

gitlab/_logging.py

+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU Lesser General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU Lesser General Public License for more details.
+#
+# You should have received a copy of the GNU Lesser General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.


IIRC this PR was started before we centralized the licensing into one file only, so maybe can be removed? Or if you feel strongly about license per file maybe just an SPDX header to keep it concise and machine-readable.

I'll get rid of that 👍

gitlab/client.py

nejch · 2022-11-01T22:16:42Z

gitlab/_logging.py

+# output unless the client application configures logging. For example by
+# calling `logging.basicConfig()`
+_module_root_logger_name = __name__.split(".", maxsplit=1)[0]
+logging.getLogger(_module_root_logger_name).addHandler(logging.NullHandler())


To me it just shifts the issue to imports if something happens to be imported e.g. import _clients would go before this. So I thought there might be a simple way for this, as I don't really see it done in popular python libraries. I know some use get_logger() helpers to lazily load loggers, maybe that's the idea here and I'm not getting it 😁

I probably don't exactly understand the risks here for our own codebase or how you stumbled across this issue, so it would just be completely subjective from my side, maybe @max-wittig or @lmilbaum would be better to take a look here :P

lmilbaum · 2022-11-03T06:27:01Z

I think it worth adding this feature to the features list mentioned in the README

lmilbaum · 2022-11-03T07:04:39Z

Integration tests would probably help us figure out what actually happens when the library is being used by an application. More than that, it will provide us the confidence that what happens is exactly what we expect it to do. WDYT?

Follow the Python documentation guidelines for "Configuring Logging for a Library" [1] Which is basically adding these two lines: import logging logging.getLogger(__name__).addHandler(logging.NullHandler()) Setup a very basic usage of logging in `gitlab/client.py` By using the NullHandler it means that by default any log messages output will not be displayed. It is up to the client application to do a `logging.basicConfig()` call to get log messages to display. [1] https://docs.python.org/3/howto/logging.html#configuring-logging-for-a-library Related: #2080

JohnVillalovos requested a review from nejch July 30, 2022 20:25

JohnVillalovos force-pushed the jlvillal/logging branch 5 times, most recently from 6b68469 to cf064f9 Compare August 6, 2022 05:16

nejch reviewed Aug 7, 2022

View reviewed changes

JohnVillalovos force-pushed the jlvillal/logging branch from cf064f9 to 1509425 Compare August 25, 2022 16:49

JohnVillalovos force-pushed the jlvillal/logging branch from 1509425 to c6eb4c2 Compare September 10, 2022 20:10

JohnVillalovos force-pushed the jlvillal/logging branch from c6eb4c2 to 990098a Compare October 4, 2022 04:29

nejch reviewed Nov 1, 2022

View reviewed changes

nejch force-pushed the jlvillal/logging branch from 990098a to b476afe Compare March 12, 2023 19:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: add bare-minimum logging support #2204

chore: add bare-minimum logging support #2204

JohnVillalovos commented Jul 30, 2022 •

edited

codecov-commenter commented Jul 30, 2022 •

edited by codecov bot

nejch left a comment

nejch Aug 7, 2022

JohnVillalovos Aug 25, 2022

JohnVillalovos Aug 25, 2022 •

edited

nejch Oct 8, 2022

lmilbaum Nov 3, 2022

nejch Aug 7, 2022

JohnVillalovos Aug 25, 2022

nejch Nov 1, 2022

lmilbaum Nov 2, 2022

lmilbaum Nov 3, 2022

nejch Nov 3, 2022

nejch left a comment

nejch Nov 1, 2022

nejch Nov 1, 2022

JohnVillalovos Nov 1, 2022

nejch Nov 1, 2022

lmilbaum commented Nov 3, 2022

lmilbaum commented Nov 3, 2022

	LOG = logging.getLogger(__name__)
	logger = logging.getLogger(__name__)

chore: add bare-minimum logging support #2204

Are you sure you want to change the base?

chore: add bare-minimum logging support #2204

Conversation

JohnVillalovos commented Jul 30, 2022 • edited

codecov-commenter commented Jul 30, 2022 • edited by codecov bot

Codecov Report

nejch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JohnVillalovos Aug 25, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nejch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmilbaum commented Nov 3, 2022

lmilbaum commented Nov 3, 2022

JohnVillalovos commented Jul 30, 2022 •

edited

codecov-commenter commented Jul 30, 2022 •

edited by codecov bot

JohnVillalovos Aug 25, 2022 •

edited