-
Notifications
You must be signed in to change notification settings - Fork 4.4k
[Prism] Fix two kinds of errors when using json log format. #36020
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
Conversation
/gemini review |
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.
Code Review
This pull request aims to fix two logging-related errors when using the JSON log format. The changes correctly address the original issues by modifying how watermarks are logged and by improving the logging logic for container logs and expansion services.
However, the new slog.LogValuer
implementations for PColInfo
and stage
introduce new logging issues. Specifically, PColInfo.LogValue
attempts to log function fields, which will cause a JSON marshaling error. Additionally, both stage.LogValue
and the topology logging in executePipeline
incorrectly use slog.Any
to log a slice of attributes, which should be handled with slog.Group
. I've provided specific comments and suggestions to fix these new issues.
The Python-side changes to support this are well-implemented and improve the robustness and structure of the logging system.
sdks/go/pkg/beam/runners/prism/internal/engine/elementmanager.go
Outdated
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #36020 +/- ##
============================================
- Coverage 56.77% 56.76% -0.01%
Complexity 3383 3383
============================================
Files 1220 1220
Lines 184968 184998 +30
Branches 3508 3508
============================================
+ Hits 105015 105016 +1
- Misses 76625 76658 +33
+ Partials 3328 3324 -4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Assigning reviewers: R: @jrmccluskey for label python. Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
A follow-up PR of #36008
Fixed two types of errors can be seen when using json log format.
After the fix, it prints out the correct fields in topo ([]*stage) and uses epoch millisecond for watermarks.
There are also some minor improvements on the logging logic. For example, logs from expansion service are now recorded with logger name "ExpansionService".