Skip to content

Replace debuglevel-related logic with logging #68443

Open
Open
Replace debuglevel-related logic with logging#68443
Labels
3.7 (EOL)end of life3.8 (EOL)end of lifeeasystdlibStandard Library Python modules in the Lib/ directorytype-featureA feature request or enhancement

Description

@demianbrecht
demianbrecht
mannequin
opened on May 21, 2015
BPO 24255
Nosy @vsajip, @bitdancer, @berkerpeksag, @demianbrecht, @CuriousLearner, @xgid, @sloonz, @sosey, @aldwinaldwin
PRs
  • gh-68443: Replace debug level-related logic in http client with logging #8633
    		•
    Files
  • http-client-logging.patch
  • http-client-logging-v2.patch
    		•

    Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
closed_at = None
created_at = <Date 2015-05-21.06:25:54.911>
labels = ['3.7', '3.8', 'type-feature', 'library', 'easy']
title = 'Replace debuglevel-related logic with logging'
updated_at = <Date 2019-06-17.08:18:45.993>
user = 'https://github.com/demianbrecht'

    bugs.python.org fields:

    activity = <Date 2019-06-17.08:18:45.993>
actor = 'aldwinaldwin'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Library (Lib)']
creation = <Date 2015-05-21.06:25:54.911>
creator = 'demian.brecht'
dependencies = []
files = ['39651', '47727']
hgrepos = []
issue_num = 24255
keywords = ['patch', 'easy']
message_count = 19.0
messages = ['243734', '243750', '244919', '244960', '312426', '316571', '322897', '322958', '322968', '322977', '322983', '322985', '322986', '322989', '323239', '323249', '323254', '323268', '345816']
nosy_count = 11.0
nosy_names = ['vinay.sajip', 'r.david.murray', 'berker.peksag', 'demian.brecht', 'erynofwales', 'CuriousLearner', 'xgdomingo', 'sloonz', 'msosey', 'Conrad Ho', 'aldwinaldwin']
pr_nums = ['8633']
priority = 'normal'
resolution = None
stage = 'needs patch'
status = 'open'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue24255'
versions = ['Python 3.6', 'Python 3.7', 'Python 3.8']

    Activity

    1. demianbrecht commented on May 21, 2015

      @demianbrecht
      demianbrechtmannequin
      on May 21, 2015
      MannequinAuthor

      Far too many times have I wished that changing the logging output in http.client was controllable through logging configuration rather than code changes modifying a connection's debuglevel.

      It would be nice if the http package was brought up to date and had debuglevel-related code replaced with logging and sane logging was added throughout.

    2. demianbrecht
      added
      stdlibStandard Library Python modules in the Lib/ directory
      type-featureA feature request or enhancement
      on May 21, 2015

    3. bitdancer commented on May 21, 2015

      @bitdancer
      bitdancer
      on May 21, 2015
      Member

      +1

    4. demianbrecht
      added
      easy
      on May 21, 2015

    5. erynofwales commented on Jun 6, 2015

      @erynofwales
      erynofwalesmannequin
      on Jun 6, 2015
      Mannequin

      Hi. This is my first issue, but I'd be willing to have a go at updating this module. Do either of you have specific ideas about what changes you want here?

      My thought was to import logging, create a logger object, and start by replacing all the self.debuglevel stuff with appropriate log statements. Does that make sense? Do each of the classes need separate loggers, or is just one module-level logger enough?

    6. erynofwales commented on Jun 7, 2015

      @erynofwales
      erynofwalesmannequin
      on Jun 7, 2015
      Mannequin

      Here's a patch that replaces all the debuglevel stuff with logging. There's a single module-level logger that handles it all.

      I wasn't sure if it would be okay to break API compatibility, so I kept the debuglevel flag and the set_debuglevel() method even though they don't do anything.

      Most of the information that was being print()ed is still there, but I cleaned it up a little and added a few more messages.

      The INFO level messages are pretty sparse, with DEBUG providing a lot more raw data from the request.

    7. CuriousLearner commented on Feb 20, 2018

      @CuriousLearner
      CuriousLearner
      on Feb 20, 2018
      Member

      Hey Erin,

      Can you please convert your patch to a PR on Github?

    8. sosey commented on May 14, 2018

      @sosey
      soseymannequin
      on May 14, 2018
      Mannequin

      I'm going to work on this one since the original reporter last commented 3 years ago. I took a quick look at how the other modules are handling logging to see if I could make it consistent, and they all do it a bit differently. It might be worthwhile considering normalizing logging across the modules at some point. I'm working with the python 3.8 currently on master.

    9. ConradHo commented on Aug 1, 2018

      @ConradHo
      ConradHomannequin
      on Aug 1, 2018
      Mannequin

      Hi,

      I have referenced the original patch and created an updated patch that uses the logging module + f-strings in place of the print statements in the http.client module. Also updated the relevant tests for print/logging in test_httplib to reflect these changes.

      The HTTPHandlerTest testcase from test_logging was also affected. In the testcase, it gets a logger with name 'http' and adds a logging.HTTPHandler to it. In our patch, we create a http.client logger, which happens to be considered a child of this testcase logger under the logger naming hierarchy.

      This causes http.client logging events to propagate up to the 'http' logger and for the testcase to loop infinitely (ie. the HTTPHandler calls http.client functions internally. These functions log events using the http.client logger, which propagate up to the testcase http logger which calls HttpHandler again).

      I have simply changed the testcase getLogger name to not be 'http' and clash with that http.client module logger.

    10. bitdancer commented on Aug 2, 2018

      @bitdancer
      bitdancer
      on Aug 2, 2018
      Member

      Conrad: thanks for the effort, but using f-strings with logging is counterproductive. The idea behind logging is that the logged strings are not materialized unless something actually wants to output them. Using fstrings means you are doing all the work of formatting the string regardless of whether or not the string is actually going to get written anywhere. The original patch also retains the debug guards that minimize overhead when debugging is not turned on, which it doesn't look like your patch does.

      Regardless, what we need at this stage is a github PR, not a patch :)

    11. erynofwales commented on Aug 2, 2018

      @erynofwales
      erynofwalesmannequin
      on Aug 2, 2018
      Mannequin

      Hi, it sounds like my original patch is the preferred approach. I can put up a GitHub PR for that.

    12. erynofwales commented on Aug 2, 2018

      @erynofwales
      erynofwalesmannequin
      on Aug 2, 2018
      Mannequin

      Actually, I spoke too soon. My current employer isn't too keen on us contributing to open source, so I can't do this. Sorry!

    13. CuriousLearner commented on Aug 2, 2018

      @CuriousLearner
      CuriousLearner
      on Aug 2, 2018
      Member

      That's okay Eryn. We really appreciate all your help.

      I will take this patch forward :)

    14. ConradHo commented on Aug 2, 2018

      @ConradHo
      ConradHomannequin
      on Aug 2, 2018
      Mannequin

      Thanks Eryn!

      @sanyam if you apply the original patch directly that will currently result in some merge failures, and there are test fixes etc that I did on the second patch. Think we should combine them.

      I just made the chgs suggested by David on my own forked repo. Do you want me to submit a PR directly?

    15. ConradHo commented on Aug 2, 2018

      @ConradHo
      ConradHomannequin
      on Aug 2, 2018
      Mannequin

      @eryn in the news blurb thing I'm going to say
      "original patch done by Eryn Wells." Your employer should be okay with that right? :D

    16. CuriousLearner commented on Aug 2, 2018

      @CuriousLearner
      CuriousLearner
      on Aug 2, 2018
      Member

      Sure Conrad,

      Yeah, indeed. The patch didn't apply cleanly, so I"ve done it manually.

      I've raised the PR here: #8633

      I'll check your patch and merge :)

      Thanks for your help too!

      I'm adding Python 3.7 and Python 3.8 for this patch.

    17. CuriousLearner
      added
      3.7 (EOL)end of life
      3.8 (EOL)end of life
      on Aug 2, 2018

    18. ConradHo commented on Aug 7, 2018

      @ConradHo
      ConradHomannequin
      on Aug 7, 2018
      Mannequin

      Hi Sanyam, were you able to fix the CI errors?

      The fixes for the infinite loop that you are seeing in your PR CI run and the changes to test correct logging (vs testing stdout) etc are in my original patch already. I've checked that the test suite passes with my patch.

      Otherwise, should I try to submit a PR / what's the best way to move this forward? It's my first contribution to cpython!

    19. CuriousLearner commented on Aug 7, 2018

      @CuriousLearner
      CuriousLearner
      on Aug 7, 2018
      Member

      Hey Conrad,

      I've merged your fixes as well. They are in the PR now. Also added your name in the NEWS entry :)

      @vinay, I noticed that there are linting errors in test_logging.py. Does it make sense to issue a separate Pull Request to fix those? Currently, I've fixed them in the current Pull Request. Please let me know :)

    20. bitdancer commented on Aug 7, 2018

      @bitdancer
      bitdancer
      on Aug 7, 2018
      Member

      We generally do not fix "linting errors" unless they reveal logic errors or we touch the lines of code for some other reason. We also follow the existing style of a module rather than any particular style guide (the stdlib modules are often older than all of the style guides...even PEP-8). In short, omit those changes from your PR, and don't bother creating a separate one, it would just get rejected ;)

    21. CuriousLearner commented on Aug 8, 2018

      @CuriousLearner
      CuriousLearner
      on Aug 8, 2018
      Member

      Yeah, that is understandable. I've reverted major chunk of it. Just in the http client file, I've added blank lines where ever necessary (which I think won't change the blame information for any of the code :))

      Would that be okay?

      Also, I did a change in the test to redirect the output from test.support to stdout in order to test the changes, but seems like the CI failed because the environment of test was changed. Is there a better method to accomplish it?

    22. aldwinaldwin commented on Jun 17, 2019

      @aldwinaldwin
      aldwinaldwinmannequin
      on Jun 17, 2019
      Mannequin

      PR waiting review, Stage should be 'patch review'

    23. ezio-melotti
      transferred this issue fromon Apr 10, 2022
    24. serhiy-storchaka
      added this to Logging issues 🪵on May 20, 2022
    25. vsajip
      removed this from Logging issues 🪵on Aug 27, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Metadata

    Metadata

    Assignees

    No one assigned

      Labels

      3.7 (EOL)end of life3.8 (EOL)end of lifeeasystdlibStandard Library Python modules in the Lib/ directorytype-featureA feature request or enhancement

      Projects

      No projects

        Milestone

        No milestone

        Relationships

        None yet

        Development

        No branches or pull requests

        Issue actions