Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gh-68443: Replace debug level-related logic in http client with logging #8633

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

CuriousLearner
Copy link
Member

@CuriousLearner CuriousLearner commented Aug 2, 2018

@CuriousLearner
Copy link
Member Author

CuriousLearner commented Aug 7, 2018

@vsajip Hello, I see that there are various linting errors in test_logging.py which were fixed in 5f63c20. Does it make sense to separate them out in another Pull Request?

* master: (104 commits)
  Fast path for exact floats in math.hypot() and math.dist() (pythonGH-8949)
  Remove AIX workaround test_subprocess (pythonGH-8939)
  bpo-34503: Fix refleak in PyErr_SetObject() (pythonGH-8934)
  closes bpo-34504: Remove the useless NULL check in PySequence_Check(). (pythonGH-8935)
  closes bpo-34501: PyType_FromSpecWithBases: Check spec->name before dereferencing it. (pythonGH-8930)
  closes bpo-34502: Remove a note about utf8_mode from sys.exit() docs. (pythonGH-8928)
  Remove unneeded PyErr_Clear() in _winapi_SetNamedPipeHandleState_impl() (pythonGH-8281)
  Fix markup in stdtypes documentation (pythonGH-8905)
  bpo-34395: Don't free allocated memory on realloc fail in load_mark() in _pickle.c. (pythonGH-8788)
  Fix upsizing of marks stack in pickle module. (pythonGH-8860)
  bpo-34171: Prevent creating Lib/trace.cover when run the trace module. (pythonGH-8841)
  closes bpo-34493: Objects/genobject.c: Add missing NULL check to compute_cr_origin() (pythonGH-8911)
  Fixed typo with asynccontextmanager code example (pythonGH-8845)
  bpo-34426: fix typo (__lltrace__ -> __ltrace__) (pythonGH-8822)
  bpo-13312: Avoid int underflow in time year. (pythonGH-8912)
  bpo-34492: Python/coreconfig.c: Fix _Py_wstrlist_copy() (pythonGH-8910)
  bpo-34448: Improve output of usable wchar_t check (pythonGH-8846)
  closes bpo-34471: _datetime: Add missing NULL check to tzinfo_from_isoformat_results. (pythonGH-8869)
  bpo-6700: Fix inspect.getsourcelines for module level frames/tracebacks (pythonGH-8864)
  Fix typo in the dataclasses's doc (pythonGH-8896)
  ...
@willingc willingc self-requested a review October 7, 2018 09:23
@CuriousLearner
Copy link
Member Author

Hey @bitdancer , @vsajip

Can you please have a look at this? I just noticed this PR wasn't reviewed from almost a year. Please let me know, what is needed here.

Thanks!

@willingc
Copy link
Contributor

willingc commented May 9, 2019

@CuriousLearner I would go ahead and rebase to resolve conflicts.

@CuriousLearner
Copy link
Member Author

@willingc Done!

@@ -265,7 +269,7 @@ def _read_status(self):
if len(line) > _MAXLINE:
raise LineTooLong("status line")
if self.debuglevel > 0:
print("reply:", repr(line))
_log.info("reply: {}".format(repr(line)))
Copy link

Choose a reason for hiding this comment

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

logging first argument support old format string, so _log.info("reply: %s", repr(line)) would work. There is mixed calls of {}".format(foo with %s", foo, wouldn't be better to stick with one?

Copy link
Member

Choose a reason for hiding this comment

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

wouldn't be better to stick with one?

I agree - % should work fine and a lot of logging uses %-formatting rather than .format() due to its age.

Copy link
Contributor

Choose a reason for hiding this comment

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

There is another reason: tools like Sentry use message pattern as a key for logs grouping.
Adding pre-formatted messages to Sentry is very ineffective.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, sorry!

I think I came back to this later and followed the same style as for other Python projects I was involved with.

Fixing this!

@@ -3,6 +3,8 @@
import io
import itertools
import os
import sys
Copy link

Choose a reason for hiding this comment

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

This isn't being used anywhere!?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, good catch. I think it remained here while I was working on this.

logger = logging.getLogger("http")
root_logger = self.root_logger
root_logger.removeHandler(self.root_logger.handlers[0])
logger = logging.getLogger("httplogger")
Copy link
Member

Choose a reason for hiding this comment

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

Why this name change?

Copy link
Member Author

Choose a reason for hiding this comment

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

There is no specific reason for it. I'll just revert back.

* master: (599 commits)
  Docs: Improved phrasing (pythonGH-14069)
  Remove redundant if check from optional argument function in argparse. (pythonGH-8766)
  bpo-37289: Add a test for if with ifexpr in the peephole optimiser to detect regressions (pythonGH-14127)
  Update What's New in Python 3.9 (pythonGH-14253)
  bpo-36511: Improve ARM32 buildbot scripts (pythonGH-14251)
  bpo-37151: remove _PyCFunction_FastCallDict (pythonGH-14269)
  Fix typo, 'widger' -> 'widget', in idlelib/tree.py (pythonGH-14263)
  Fix bpo number in News file. (pythonGH-14260)
  bpo-37342: Fix the incorrect nb_index's type in typeobj documentation (pythonGH-14241)
  Update What's New in Python 3.8 (pythonGH-14239)
  bpo-36710: Use tstate in pylifecycle.c (pythonGH-14249)
  Add missing single quote in io.TextIOWrapper.reconfigure documentation (pythonGH-14246)
  bpo-36511: Add buildbot scripts and fix tests for Windows ARM32 buildbot (pythonGH-13454)
  bpo-37333: Ensure IncludeTkinter has a value (pythonGH-14240)
  bpo-37331: Clarify format of socket handler messages in the documentation. (pythonGH-14234)
  bpo-37258: Not a bug, but added a unit test and updated documentation. (pythonGH-14229)
  bpo-36710: Remove PyImport_Cleanup() function (pythonGH-14221)
  Fix name of '\0'. (pythonGH-14222)
  bpo-36710: Add tstate parameter in import.c (pythonGH-14218)
  Document typing.ForwardRef (pythonGH-14216)
  ...
@csabella csabella requested a review from bitdancer February 25, 2020 12:34
Copy link

This PR is stale because it has been open for 30 days with no activity.

@github-actions github-actions bot added the stale Stale PR or inactive for long period of time. label Dec 15, 2024
@picnixz picnixz changed the title bpo-24255: Replace debug level-related logic in http client with logging gh-68443: Replace debug level-related logic in http client with logging Dec 15, 2024
@github-actions github-actions bot removed the stale Stale PR or inactive for long period of time. label Feb 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants