gemini-code-assist[bot] commented on code in PR #36020:
URL: https://github.com/apache/beam/pull/36020#discussion_r2312071615
##########
sdks/go/pkg/beam/runners/prism/internal/execute.go:
##########
@@ -360,7 +360,11 @@ func executePipeline(ctx context.Context, wks
map[string]*worker.W, j *jobservic
case rb, ok := <-bundles:
if !ok {
err := eg.Wait()
- j.Logger.Debug("pipeline done!",
slog.String("job", j.String()), slog.Any("error", err), slog.Any("topo", topo))
+ var topoAttrs []slog.Attr
+ for _, s := range topo {
+ topoAttrs = append(topoAttrs,
slog.Any(s.ID, s))
+ }
+ j.Logger.Debug("pipeline done!",
slog.String("job", j.String()), slog.Any("error", err), slog.Any("topo",
topoAttrs))
Review Comment:
Using `slog.Any("topo", topoAttrs)` will not produce the desired structured
log for the topology. `topoAttrs` is a slice of `slog.Attr`, and `slog.Any`
will attempt to log it as a single value, which is not what is intended and
will likely result in incorrect JSON. To log a group of attributes, you should
use `slog.Group`.
```suggestion
j.Logger.Debug("pipeline done!",
slog.String("job", j.String()), slog.Any("error", err), slog.Group("topo",
topoAttrs...))
```
##########
sdks/go/pkg/beam/runners/prism/internal/engine/elementmanager.go:
##########
@@ -89,6 +89,15 @@ type PColInfo struct {
KeyDec func(io.Reader) []byte
}
+func (info PColInfo) LogValue() slog.Value {
+ return slog.GroupValue(
+ slog.String("GlobalID", info.GlobalID),
+ slog.String("WindowCoder", info.WindowCoder.String()),
+ slog.Any("WDec", info.WDec),
+ slog.Any("WEnc", info.WEnc),
+ )
Review Comment:
The `WDec` and `WEnc` fields are function types. Attempting to log them with
`slog.Any` will result in a JSON marshaling error, similar to the issue this
pull request aims to fix. Function values cannot be serialized to JSON. These
fields should be omitted from the log output.
```go
return slog.GroupValue(
slog.String("GlobalID", info.GlobalID),
slog.String("WindowCoder", info.WindowCoder.String()),
)
```
##########
sdks/go/pkg/beam/runners/prism/internal/stage.go:
##########
@@ -108,6 +108,19 @@ func clampTick(dur time.Duration) time.Duration {
}
}
+func (s *stage) LogValue() slog.Value {
+ var outAttrs []slog.Attr
+ for k, v := range s.OutputsToCoders {
+ outAttrs = append(outAttrs, slog.Any(k, v))
+ }
+ return slog.GroupValue(
+ slog.String("ID", s.ID),
+ slog.Any("transforms", s.transforms),
+ slog.Any("inputInfo", s.inputInfo),
+ slog.Any("outputInfo", outAttrs),
Review Comment:
Using `slog.Any("outputInfo", outAttrs)` will not produce the desired
structured log for the `outputInfo` field. `outAttrs` is a slice of
`slog.Attr`, and `slog.Any` will attempt to log it as a single value, which is
not what is intended and will likely result in incorrect JSON. To log a group
of attributes, you should use `slog.Group`.
```suggestion
slog.Group("outputInfo", outAttrs...),
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
