gh-91513: add taskname to logging.LogRecord attributes#92646
gh-91513: add taskname to logging.LogRecord attributes#92646carlbordum wants to merge 1 commit intopython:mainfrom
taskname to logging.LogRecord attributes#92646Conversation
carlbordum
force-pushed
the
91513-logrecord-taskname-attr
branch
from
4a9cae1 to
27b9704
Compare
carlbordum
changed the title
taskname to logging.LogRecord attributestaskname to logging.LogRecord attributes
carlbordum
force-pushed
the
91513-logrecord-taskname-attr
branch
from
27b9704 to
6e7dc2c
Compare
| self.process = None | ||
| if logAsyncioTasks: | ||
| aio = sys.modules.get('asyncio') | ||
| self.taskname = aio.current_task().get_name() |
There was a problem hiding this comment.
- If
asynciohasn't been imported,aioisNoneand this will fail withAttributeError. - If we aren't running any asyncio task right now,
current_task()will raiseRuntimeError.
In other words, this fails with or without the asyncio import:
#import asyncio
import logging
logging.error("blah blah")I think we should also make sure that self.taskname is always set to something, and just set it to None if logAsyncTasks is set to False or no asyncio task is running. See how it sets self.processName = None above.
vsajip
left a comment
vsajip
left a comment
There was a problem hiding this comment.
This PR needs tests (which would have exposed the point that @Akuli makes) and it would also make sense to use taskName in preference to taskname. I know it's not PEP-8 compliant and all, but logging predated PEP-8 and it makes more sense to keep things consistent at a local level.
It would make sense to do something like:
if not logAsyncioTasks:
self.taskName = None
else:
aio = sys.modules.get('asyncio')
if aio is None:
self.taskName = None
else:
self.taskName = aio.current_task().get_name()or, more concisely,
self.taskName = None
if logAsyncioTasks:
aio = sys.modules.get('asyncio')
if aio:
self.taskName = aio.current_task().get_name() 
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
|
There's been no response to my request for changes, and now there's another PR - #93193 - to cover the same functionality. I propose to close this PR in favour of the other. |
Sorry for making such an incomplete PR. I'll be more thorough next time. |
|
That's OK, don't worry about it. What matters is that the functionality is now in there. Thanks for trying to improve |
4 participants
No description provided.