Skip to content

Conversation

DaleSeo
Copy link
Contributor

@DaleSeo DaleSeo commented Feb 28, 2021

What kind of change does this PR introduce?

Fixes #43. This PR stops using the root logger that makes it difficult for consumers to configure logging for our library and removes the one-off logging configuration from the library which is totally unnecessary.

What is the current behavior?

It's difficult for consumers of the library to configure logging for the library specifically.

Let's assume that a consumer has the following logging setup as they want their INFO messages are logged.

import logging
import logging.config
import asyncio
from python_graphql_client import GraphqlClient

LOGGING = {
    "version": 1,
    "disable_existing_loggers": False,
    "formatters": {
        "simple": {"format": "[%(levelname)s] %(module)s: %(message)s"},
    },
    "handlers": {
        "console": {"class": "logging.StreamHandler", "formatter": "simple"},
    },
    "root": {
        "handlers": ["console"],
        "level": "INFO",
    },
}
logging.config.dictConfig(LOGGING)

logging.info("Logging in the consumer code...")

client = GraphqlClient(endpoint="ws://localhost:4000/subscriptions")
query = """
    subscription onMessageAdded {
        messageAdded
    }
"""
asyncio.run(client.subscribe(query=query, handle=print))

However, the consumer ends up getting INFO messages of our library logged as well as their application logs.

$ python dale_test.py
[INFO] root: Logging in the consumer code...
[INFO] python_graphql_client.graphql_client: the server accepted the connection
{'data': {'messageAdded': 'In ea ratione.'}}
{'data': {'messageAdded': 'Odio consequuntur sit a voluptatem dolorem sint.'}}

This is because our library uses the root logger with a one-off logging setup that configures the root logger to log all INFO messages.

From the consumer's perspective, this can be annoying because there is no way for them to differentiate their logs from our library logs.

What is the new behavior?

A simple solution is to stop using the root logger and remove the one-off logging setup from the library. Instead, if we use a logger scoped to the package namespace, consumers will be able to configure logging for the library differently from the rest of their application.

import logging
import logging.config
import asyncio
from python_graphql_client import GraphqlClient

LOGGING = {
    "version": 1,
    "disable_existing_loggers": False,
    "formatters": {
        "simple": {"format": "[%(levelname)s] %(name)s: %(message)s"},
    },
    "handlers": {
        "console": {"class": "logging.StreamHandler", "formatter": "simple"},
    },
    "root": {
        "handlers": ["console"],
        "level": "INFO",
    },
    "loggers": {
        "python_graphql_client": {
            "handlers": ["console"],
            "level": "WARNING",
            "propagate": False,
        }
    },
}
logging.config.dictConfig(LOGGING)

logging.info("Logging in the consumer code...")

client = GraphqlClient(endpoint="ws://localhost:4000/subscriptions")
query = """
    subscription onMessageAdded {
        messageAdded
    }
"""
asyncio.run(client.subscribe(query=query, handle=print))
$ python dale_test.py
[INFO] root: Logging in the consumer code...
{'data': {'messageAdded': 'Qui quia illo facere magnam cum ab consequuntur.'}}
{'data': {'messageAdded': 'Est a optio.'}}

Does this PR introduce a breaking change?

This PR will only affect consumers who use subscription operations through this library. They might experience INFO logs of our library disappearing depending on their logging setup. I don't believe this a breaking change since it doesn't change any API or behaviors of the library.

@DaleSeo DaleSeo self-assigned this Feb 28, 2021
@DaleSeo DaleSeo requested review from steve148 and xkludge February 28, 2021 23:07
@DaleSeo DaleSeo added the enhancement New feature or request label Feb 28, 2021
@DaleSeo DaleSeo changed the title Removes Global Logging Configuration Removes One-off Logging Configuration Mar 1, 2021
@DaleSeo DaleSeo changed the title Removes One-off Logging Configuration Stoping Using Root Logger with One-off Logging Configuration Mar 1, 2021
@DaleSeo DaleSeo changed the title Stoping Using Root Logger with One-off Logging Configuration Stoping Using Root Logger with One-off Logging Setup Mar 1, 2021
Copy link
Contributor

@xkludge xkludge left a comment

Choose a reason for hiding this comment

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

Lgtm

@DaleSeo DaleSeo merged commit 8213abc into prodigyeducation:master Mar 4, 2021
@DaleSeo DaleSeo deleted the remove-global-logging branch March 4, 2021 20:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disable client logs
3 participants