lostluck commented on code in PR #36538: URL: https://github.com/apache/beam/pull/36538#discussion_r2436315362
########## sdks/go/pkg/beam/internal/logconfig/logconfig.go: ########## @@ -0,0 +1,85 @@ +// Licensed to the Apache Software Foundation (ASF) under one or more +// contributor license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright ownership. +// The ASF licenses this file to You under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance with +// the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// Package errors contains functionality for set up structural logger level and kind. +package logconfig + +import ( + "log" + "log/slog" + "os" + "strings" + "time" + + "github.com/golang-cz/devslog" +) + +var ( + LogLevel = "info" // The logging level for slog. Valid values are `debug`, `info`, `warn` or `error`. Default is `info`. + LogKind = "text" // The logging format for slog. Valid values are `dev', 'json', or 'text'. Default is `text`. Review Comment: As a rule package level variables like this are an anti pattern, since they open up to race conditions with multiple assignments happening. But we end up with some safety here due to encapsulating them in an internal package, so we control when these are called. My bias is to make them atomic, unexported and access them through functions instead, so they can't change outside of those calls below. ########## sdks/go/pkg/beam/runners/prism/internal/worker/worker.go: ########## @@ -245,7 +245,7 @@ func (wk *W) Logging(stream fnpb.BeamFnLogging_LoggingServer) error { attrs = append(attrs, slog.Group("customData", grp...)) } - if beamlog.LogLevel == "debug" { + if logconfig.LogLevel == "debug" { Review Comment: Separate question: is this mechanism here so Go SDK "in process" logging is consolidated with the "separate binary" logging? ########## sdks/go/pkg/beam/runners/prism/internal/worker/worker.go: ########## @@ -245,7 +245,7 @@ func (wk *W) Logging(stream fnpb.BeamFnLogging_LoggingServer) error { attrs = append(attrs, slog.Group("customData", grp...)) } - if beamlog.LogLevel == "debug" { + if logconfig.LogLevel == "debug" { Review Comment: Instead of a global field, consider making this a per pipeline setting that is on the worker W type instead of on a package variable. Slightly longer path, but no concurrency risk, and reduces global (all pipeline) settings in prism. (Binary options like that should set defaults, but should generally be overridable per pipeline. -- 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]
