-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Conversation
alonbl
commented
Jul 5, 2025
Anyone?
Josverl
commented
Jul 6, 2025
Did you see my review comments?
alonbl
commented
Jul 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,
Alon
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 of extra is to provide the same functionality as CPythons LogRecord( ..., 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 between LogRecord[1] and Logger.makeRecord[2].
It is "unknown" how extra reaches the LogRecord once constructed.
I can make extra as property which is also not fully compatible.
I can have setExtra().
I do not think this is that critical how we pass the extra into the record, as the interface is clearly not compatible, for example, we do not have makeRecord 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
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 to if 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.
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.
Sorry for the late reply.
With your clarification the if extra == None implementation makes sense.
Josverl
commented
Oct 23, 2025
Hi Alon,
I resubmitted my review and this time it shows up.
In addition, I think it would be helpful to add an example of how to add such context.
alonbl
commented
Oct 23, 2025
Hi Alon, I resubmitted my review and this time it shows up. In addition, I think it would be helpful to add an example of how to add such context.
Thanks! I will create an example.
Extra context is usable to enrich log record with concrete context additions. Signed-off-by: Alon Bar-Lev <alon.barlev@gmail.com>
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. Signed-off-by: Alon Bar-Lev <alon.barlev@gmail.com>
7a2b33a to
e3992b8
Compare
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.