classification
Title: Replace debuglevel-related logic with logging
Type: enhancement Stage: needs patch
Components: Library (Lib) Versions: Python 3.8, Python 3.7, Python 3.6
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: Conrad Ho, CuriousLearner, berker.peksag, demian.brecht, erynofwales, msosey, r.david.murray, sloonz, vinay.sajip, xgdomingo
Priority: normal Keywords: easy, patch

Created on 2015-05-21 06:25 by demian.brecht, last changed 2018-08-08 07:52 by CuriousLearner.

Files
File name Uploaded Description Edit
http-client-logging.patch erynofwales, 2015-06-07 17:17 review
http-client-logging-v2.patch Conrad Ho, 2018-08-01 23:16
Pull Requests
URL Status Linked Edit
PR 8633 open CuriousLearner, 2018-08-02 18:25
Messages (18)
msg243734 - (view) Author: Demian Brecht (demian.brecht) * (Python triager) Date: 2015-05-21 06:25
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.
msg243750 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2015-05-21 13:57
+1
msg244919 - (view) Author: Eryn Wells (erynofwales) * Date: 2015-06-06 16:28
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?
msg244960 - (view) Author: Eryn Wells (erynofwales) * Date: 2015-06-07 17:17
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.
msg312426 - (view) Author: Sanyam Khurana (CuriousLearner) * (Python triager) Date: 2018-02-20 18:03
Hey Erin,

Can you please convert your patch to a PR on Github?
msg316571 - (view) Author: Megan Sosey (msosey) * Date: 2018-05-14 20:06
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.
msg322897 - (view) Author: Conrad Ho (Conrad Ho) Date: 2018-08-01 23:16
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.
msg322958 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2018-08-02 14:28
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 :)
msg322968 - (view) Author: Eryn Wells (erynofwales) * Date: 2018-08-02 15:12
Hi, it sounds like my original patch is the preferred approach. I can put up a GitHub PR for that.
msg322977 - (view) Author: Eryn Wells (erynofwales) * Date: 2018-08-02 16:16
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!
msg322983 - (view) Author: Sanyam Khurana (CuriousLearner) * (Python triager) Date: 2018-08-02 17:46
That's okay Eryn. We really appreciate all your help.

I will take this patch forward :)
msg322985 - (view) Author: Conrad Ho (Conrad Ho) Date: 2018-08-02 18:08
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?
msg322986 - (view) Author: Conrad Ho (Conrad Ho) Date: 2018-08-02 18:15
@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
msg322989 - (view) Author: Sanyam Khurana (CuriousLearner) * (Python triager) Date: 2018-08-02 19:04
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.
msg323239 - (view) Author: Conrad Ho (Conrad Ho) Date: 2018-08-07 10:50
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!
msg323249 - (view) Author: Sanyam Khurana (CuriousLearner) * (Python triager) Date: 2018-08-07 19:07
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 :)
msg323254 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2018-08-07 22:42
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 ;)
msg323268 - (view) Author: Sanyam Khurana (CuriousLearner) * (Python triager) Date: 2018-08-08 07:52
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?
History
Date User Action Args
2018-08-08 07:52:04CuriousLearnersetmessages: + msg323268
2018-08-07 22:42:55r.david.murraysetmessages: + msg323254
2018-08-07 19:07:56CuriousLearnersetnosy: + vinay.sajip
messages: + msg323249
2018-08-07 10:50:15Conrad Hosetmessages: + msg323239
2018-08-02 19:04:22CuriousLearnersetstage: patch review -> needs patch
messages: + msg322989
versions: + Python 3.7, Python 3.8
2018-08-02 18:25:13CuriousLearnersetstage: needs patch -> patch review
pull_requests: + pull_request8138
2018-08-02 18:15:11Conrad Hosetmessages: + msg322986
2018-08-02 18:08:18Conrad Hosetmessages: + msg322985
2018-08-02 17:46:01CuriousLearnersetmessages: + msg322983
2018-08-02 16:16:06erynofwalessetmessages: + msg322977
2018-08-02 15:12:59erynofwalessetmessages: + msg322968
2018-08-02 14:28:26r.david.murraysetmessages: + msg322958
2018-08-01 23:16:22Conrad Hosetfiles: + http-client-logging-v2.patch
nosy: + Conrad Ho
messages: + msg322897

2018-05-14 20:06:44msoseysetnosy: + msosey
messages: + msg316571
2018-02-20 18:03:29CuriousLearnersetnosy: + CuriousLearner
messages: + msg312426
2018-02-11 02:40:36xgdomingosetnosy: + xgdomingo
2018-02-09 14:19:06sloonzsetnosy: + sloonz
2015-12-11 02:31:23r.david.murraylinkissue25835 superseder
2015-10-03 04:07:42berker.peksaglinkissue19917 superseder
2015-06-07 17:17:28erynofwalessetfiles: + http-client-logging.patch
keywords: + patch
messages: + msg244960
2015-06-06 16:54:36berker.peksagsetnosy: + berker.peksag
2015-06-06 16:28:11erynofwalessetnosy: + erynofwales
messages: + msg244919
2015-05-21 16:41:57demian.brechtsetkeywords: + easy
2015-05-21 13:57:57r.david.murraysetnosy: + r.david.murray
messages: + msg243750
2015-05-21 06:25:54demian.brechtcreate