[YAML] Better logging. #30005
[YAML] Better logging. #30005
Conversation
* Prints elements in a more universal format (json rather than BeamSchema_xxxx). * Also adds (basic!) options for log level and a message identifier. This is feedback from the bug bash.
|
R: @Polber |
|
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control |
| log_levels = { | ||
| 'ERROR': logging.error, | ||
| 'INFO': logging.info, | ||
| 'DEBUG': logging.debug, | ||
| } |
There was a problem hiding this comment.
Why not go ahead and include all the levels (WARNING, FATAL, OFF, etc.)? Same above
There was a problem hiding this comment.
Mostly here the intent was to be able to log above, at, or below what is typically logged in service infrastructure (on the workers). FATAL is a bit odd, 'cause we're not going to be killing the process on every message (one could try to make every message fatal, but that's getting into different territory), and different languages have WARN vs. WARNING. OFF is mostly useful for a log filter, not a choice for what level to log at.
Mostly I'm trying to keep things simple and address the feedback that BeamSchema_xxxxxxxxxxxxxxxxxxx was not very useful and we want to be able to do this from Java as well.
| @staticmethod | ||
| def log_for_testing(): | ||
| def log_for_testing( | ||
| level: Optional[str] = 'INFO', prefix: Optional[str] = ''): |
There was a problem hiding this comment.
prefix could default to name of transform (i.e. LogForTesting) or maybe even always put that as part of the log string and append this prefix (defaulting to empty string) to that (i.e. INFO:root:LogForTesting or INFO:root:LogForTesting:prefix) to explicitly show where the log is coming from.
Similar for the Java version, although I'm not sure what is included in the log string by default in that case (perhaps the source class is already included)
There was a problem hiding this comment.
It's not easy to get at the name of the transform here, runners often prepend the name of the transform in their logs.
There was a problem hiding this comment.
I just mean putting the literal string 'LogForTestnig' since that is the name of the transform, but not a blocker, just a thought. LGTM for now
Co-authored-by: Jeff Kinard <jeff@thekinards.com>
2 participants
This is feedback from the bug bash.
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, commentfixes #<ISSUE NUMBER>instead.CHANGES.mdwith noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.