- Notifications
You must be signed in to change notification settings - Fork1.1k
Logging improvements#1004
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
Logging improvements#1004
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Anyone? |
Josverl commentedJul 6, 2025
Did you see my review comments? |
Hi@Josverl , I do not see any comment made by anyone else but me in this ticket. I would love to review any comment, can you please refer me where? Thanks, |
| classLogRecord: | ||
| defset(self,name,level,message): | ||
| def__init__(self,name,level,message,extra=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
If the intend ofextra is to provide the same functionality as CPythonsLogRecord( ..., args, ...)
,then it makes sense to name it the same. That makes it simpler to learn.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Hi,
There is a inconsistency betweenLogRecord[1] andLogger.makeRecord[2].
It is "unknown" howextra reaches theLogRecord once constructed.
I can makeextra as property which is also not fully compatible.
I can havesetExtra().
I do not think this is that critical how we pass theextra into the record, as the interface is clearly not compatible, for example, we do not havemakeRecord or event population between loggers.
If you have a better notation, please let me know.
Thanks!
[1]https://docs.python.org/3/library/logging.html#logging.LogRecord
[2]https://docs.python.org/3/library/logging.html#logging.Logger.makeRecord
| self.ct=time.time() | ||
| self.msecs=int((self.ct-int(self.ct))*1000) | ||
| self.asctime=None | ||
| ifextraisnotNone: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
can be simplified toif extra:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Correct, but if you look closely in this source the pattern is to test for explicit None without having empty additional path. If it is important I will replace.
Josverl commentedOct 23, 2025
Hi Alon, |
Thanks! I will create an example. |
Extra context is usable to enrich log record with concrete contextadditions.Signed-off-by: Alon Bar-Lev <alon.barlev@gmail.com>
The keyword parameters are populated to common log and exc_info should becommon to all methods anyway.This change the default to be exc_info=False for all cases similar to thestandard python.Signed-off-by: Alon Bar-Lev <alon.barlev@gmail.com>
logging: Support extra context for LogRecord.
Extra context is usable to enrich log record with concrete context
additions.
logging: move exc_info to common log.
The keyword parameters are populated to common log and exc_info should be
common to all methods anyway.
This change the default to be exc_info=False for all cases similar to the
standard python.