GoogleCloudPlatform / cloud-profiler-python

Stackdriver Profiler Python agent is a tool that continuously gathers CPU usage information from Python applications
Apache License 2.0
28 stars 23 forks source link

Logging implementation could be better #29

Closed subodh101 closed 4 years ago

subodh101 commented 4 years ago

What? https://github.com/GoogleCloudPlatform/cloud-profiler-python/blob/master/googlecloudprofiler/__init__.py#L26 Defining the logging in the init() changes the configuration of the logger.

It makes the code looks pretty bad as before importing googlecloudprofiler. I have to initialize and add handler to it to the logging.

import logging

handler = logging.StreamHandler(stdout)
root_logger = logging.getLogger()
root_logger.addHandler(handler)

from googlecloudprofiler import start
...

Why? First thing is that it took me a while to figure out that because of googlecloudprofiler the structlog is not showing the correct logs as it is not considering the format that I am defining later with logging. I have used other python modules but never found such an implementation in the init.py file. It is overshadowing the new logger implementation.

How? Logger configuration can be defined somewhere and not in init.py such that just before using the googlecloudprofiler[NOT IMPORTING] the user has setup StreamHandler otherwise logging should be configured by your default method.

subodh101 commented 4 years ago

I got it fixed by removing the handlers from the root logger.

root_logger.handlers = []  # Remove all handlers from third party

After that added my own handlers.

jqll commented 4 years ago

Thanks for reporting and sorry for the late response. We should probably use a NullHandler as suggested in https://docs.python.org/3/howto/logging.html#configuring-logging-for-a-library. But it's only available after Python 3.1. Moving logging.basicConfig() in start() might also be an option. But the same problem will happen in rare cases where the user configures the loggers after calling start(). I probably won't be able to work on this immediately. But I think we should fix it and I filed an internal issue to track.