gemini-code-assist[bot] commented on code in PR #36484:
URL: https://github.com/apache/beam/pull/36484#discussion_r2427525409
##########
sdks/go/pkg/beam/runners/universal/runnerlib/job.go:
##########
@@ -138,7 +139,16 @@ func WaitForCompletion(ctx context.Context, client
jobpb.JobServiceClient, jobID
case msg.GetMessageResponse() != nil:
resp := msg.GetMessageResponse()
- text := fmt.Sprintf("%v (%v): %v", resp.GetTime(),
resp.GetMessageId(), resp.GetMessageText())
+ var b strings.Builder
+ if resp.GetTime() != "" {
+ fmt.Fprintf(&b, "(time=%v)", resp.GetTime())
+ }
+ if resp.GetMessageId() != "" {
+ fmt.Fprintf(&b, "(id=%v)", resp.GetMessageId())
+ }
+ b.WriteString(resp.GetMessageText())
+ text := b.String()
+
Review Comment:
Using `strings.Builder` is efficient, but the resulting log format
`(time=...)(id=...)message` can be a bit hard to read, especially with no space
between the parts. For improved readability and a more standard log appearance,
consider building the message from parts with spaces in between. A slice-based
approach can be more readable here, and the performance difference is likely
negligible in this context.
```suggestion
var parts []string
if resp.GetTime() != "" {
parts = append(parts, fmt.Sprintf("time=%v",
resp.GetTime()))
}
if resp.GetMessageId() != "" {
parts = append(parts, fmt.Sprintf("id=%v",
resp.GetMessageId()))
}
if resp.GetMessageText() != "" {
parts = append(parts, resp.GetMessageText())
}
text := strings.Join(parts, " ")
```
##########
sdks/go/pkg/beam/log/log.go:
##########
@@ -190,3 +201,51 @@ func Exitln(ctx context.Context, v ...any) {
Output(ctx, SevFatal, 1, fmt.Sprintln(v...))
os.Exit(1)
}
+
+func SetupLoggingWithDefault() {
+ var logLevel = new(slog.LevelVar)
+ var logHandler slog.Handler
+ loggerOutput := os.Stderr
+ handlerOpts := &slog.HandlerOptions{
+ Level: logLevel,
+ }
+ switch strings.ToLower(LogLevel) {
+ case "debug":
+ logLevel.Set(slog.LevelDebug)
+ handlerOpts.AddSource = true
+ case "info":
+ logLevel.Set(slog.LevelInfo)
+ case "warn":
+ logLevel.Set(slog.LevelWarn)
+ case "error":
+ logLevel.Set(slog.LevelError)
+ default:
+ log.Fatalf("Invalid value for log_level: %v, must be 'debug',
'info', 'warn', or 'error'", LogLevel)
+ }
+ switch strings.ToLower(LogKind) {
+ case "dev":
+ logHandler =
+ devslog.NewHandler(loggerOutput, &devslog.Options{
+ TimeFormat: "[" + time.RFC3339Nano +
"]",
+ StringerFormatter: true,
+ HandlerOptions: handlerOpts,
+ StringIndentation: false,
+ NewLineAfterLog: true,
+ MaxErrorStackTrace: 3,
+ })
Review Comment:
The configuration for the `dev` log kind uses `devslog`, which produces a
human-readable format (e.g., `[<timestamp>] INFO ...`). However, the 'After'
log example in the pull request description shows a `key=value` format, which
corresponds to the `text` log kind (`slog.TextHandler`). Since the default
`log_kind` for Prism is `dev`, this discrepancy could be confusing. To align
the default behavior with the example, you might consider changing the default
`log_kind` flag in `sdks/go/cmd/prism/prism.go` to `text`, or updating the PR
description to show an example of the `devslog` output.
--
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]
