Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: Add
ClpKeyValuePairStreamHandler
to support logging dictionary type log events into CLP key-value pair IR format. #46base: main
Are you sure you want to change the base?
feat: Add
ClpKeyValuePairStreamHandler
to support logging dictionary type log events into CLP key-value pair IR format. #46Changes from 10 commits
51636a1
e03b6d7
5a51940
919a15a
a1ed017
5a53fe5
048531d
ec36f20
d414d65
06608a7
096c3ce
e854fa1
b310993
b039278
715961b
5d8cd33
f270b39
1bcf7d7
2f03629
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
unix_timestamp_ms
?ms
is ambiguous since it could mean Ms or ms due to capitalization, so I would rather usemillisecs
for the least ambiguity; but I guess that would be a larger change across our codebase.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.
Changed the structure to:
Dropped
ZONE_
prefix as suggested in the previous discussion.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.
Is this a difference since
StreamHandler
accepts aLogRecord
andLogRecord
can have themsg
be an arbitrary object?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.
Yeah, I realized this would cause some confusion. We should probably say this is what differs from the current CLP logging handlers.
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.
NTS: This needs refactoring.
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.
I don't understand the "avoid requiring...`" part.
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.
This is directly copied from the existing docstrings here:
clp-loglib-py/src/clp_logging/handlers.py
Line 130 in 7c458f2
Do u want me to rewrite it for this new handler?
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.
Uh, not necessarily. I'm just confused about the docstring is supposed to mean? Should we ask David?
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.
I think what the original comment means is to avoid the default
emit
implementation to call output stream-levelwrite
directly: https://github.com/python/cpython/blob/c1f352bf0813803bb795b796c16040a5cd4115f2/Lib/logging/__init__.py#L1138