Skip to content

chore: add bare-minimum logging support #2204

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions gitlab/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import warnings
from typing import Any

import gitlab._logging
import gitlab.config # noqa: F401
from gitlab import utils as _utils
from gitlab._version import ( # noqa: F401
Expand Down
31 changes: 31 additions & 0 deletions gitlab/_logging.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
# -*- coding: utf-8 -*-
"""
The initialization of the logging module is intentionaly done here in the
_logging.py module so that it will be initialized before any other of our
python-gitlab modules are loaded. So if they do logging upon being loaded it
will allow them to do it safely.
"""

# 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/>.
Comment on lines +9 to +20
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll get rid of that 👍


import logging
from typing import List

__all__: List[str] = []

# Using the `NullHandler` means that any log messages generated will not be
# 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())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a docstring to explain why doing it here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

5 changes: 4 additions & 1 deletion gitlab/client.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
"""Wrapper for the GitLab API."""

import logging
import os
import re
import time
Expand All @@ -14,6 +15,8 @@
import gitlab.exceptions
from gitlab import _backends, utils

LOG = logging.getLogger(__name__)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@JohnVillalovos JohnVillalovos Aug 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


REDIRECT_MSG = (
"python-gitlab detected a {status_code} ({reason!r}) redirection. You must update "
"your GitLab URL to the correct URL to avoid issues. The redirection was from: "
Expand Down Expand Up @@ -540,7 +543,6 @@ def _set_auth_info(self) -> None:

@staticmethod
def enable_debug() -> None:
import logging
from http.client import HTTPConnection # noqa

HTTPConnection.debuglevel = 1
Expand All @@ -549,6 +551,7 @@ def enable_debug() -> None:
requests_log = logging.getLogger("requests.packages.urllib3")
requests_log.setLevel(logging.DEBUG)
requests_log.propagate = True
LOG.debug("Enabled debug mode for python-gitlab")

def _get_session_opts(self) -> Dict[str, Any]:
return {
Expand Down
24 changes: 24 additions & 0 deletions tests/unit/test__logging.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
import logging

import pytest

from gitlab import _logging


@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"
Comment on lines +8 to +18
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.



def test_logger_null_handler(LOG):
assert len(LOG.handlers) == 1
handler = LOG.handlers[0]
assert isinstance(handler, logging.NullHandler)