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.

+1

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?

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.

Hey Erin,

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

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.

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.

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

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

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!

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

I will take this patch forward :)

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?

@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

Sure Conrad,

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

I've raised the PR here: https://github.com/python/cpython/pull/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.

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!

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

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 PEP8).  In short, omit those changes from your PR, and don't bother creating a separate one, it would just get rejected ;)

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?

PR waiting review, Stage should be 'patch review'