hubcio commented on code in PR #3379: URL: https://github.com/apache/iggy/pull/3379#discussion_r3333578867
########## foreign/go/contracts/logger.go: ########## @@ -0,0 +1,39 @@ +// 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 iggcon + +import ( + "io" + "log/slog" + "os" +) + +func NewLogger(level slog.Level, writer io.Writer) *slog.Logger { + handler := slog.NewTextHandler(writer, &slog.HandlerOptions{ + Level: level, + }) + return slog.New(handler) +} + +func NewStderrLogger(level slog.Level) *slog.Logger { + return NewLogger(level, os.Stderr) +} + +func NopLogger() *slog.Logger { + return slog.New(slog.NewTextHandler(io.Discard, nil)) Review Comment: `NopLogger()` isn't actually silent. `slog.NewTextHandler(io.Discard, nil)` passes nil options, so the handler's min level defaults to info - info/warn/error still get fully formatted and then written to `io.Discard`, only debug is skipped. measured on go 1.25 that's ~430-530 ns/op (and 1 alloc once an `error` value is passed, which is exactly the heartbeat warn) vs ~3ns for a true noop. the `WithLogger` doc says output is "silently discarded" and the PR claims zero overhead, which only holds for debug. fix is one line: `return slog.New(slog.DiscardHandler)`. it reports `Enabled() == false` at every level so nothing is formatted, 0 alloc across the board, and it's already in the stdlib (go.mod and toolchain are on 1.25). all the current call sites are cold (heartbeat, login, redirect) so there's no runtime regression today, but the claim and the contract should match the code. ########## foreign/go/contracts/logger.go: ########## @@ -0,0 +1,39 @@ +// 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 iggcon + +import ( + "io" + "log/slog" + "os" +) + +func NewLogger(level slog.Level, writer io.Writer) *slog.Logger { + handler := slog.NewTextHandler(writer, &slog.HandlerOptions{ + Level: level, + }) + return slog.New(handler) +} + +func NewStderrLogger(level slog.Level) *slog.Logger { Review Comment: `NewStderrLogger` has no non-test callers - the only thing that calls it is its own test (`TestNewStderrLogger_RespectsLevel`), and `NewLogger(level, os.Stderr)` already covers what it does. golangci's `unused` won't flag it since it's exported, so it just sits as dead public surface. worth dropping it (and its test). ########## foreign/go/internal/util/leader_aware.go: ########## @@ -31,25 +31,36 @@ import ( // CheckAndRedirectToLeader queries the client for cluster metadata and returns // an address to redirect to (empty string means no redirection needed). -func CheckAndRedirectToLeader(ctx context.Context, c iggcon.Client, currentAddress string, transport iggcon.Protocol) (string, error) { - log.Println("Checking cluster metadata for leader detection") +func CheckAndRedirectToLeader(ctx context.Context, c iggcon.Client, currentAddress string, transport iggcon.Protocol, logger *slog.Logger) (string, error) { + logger.Debug("Checking cluster metadata for leader detection") Review Comment: `logger` is dereferenced here unconditionally as the first statement, and again in `processClusterMetadata`. adding the param turned this exported helper into one with an undocumented non-nil precondition. it's safe through the real path today since both `NewIggyClient` and `NewIggyTcpClient` default a nil logger to `NopLogger()`, but a struct-literal `IggyTcpClient` (e.g. `newTestClient` in the tcp tests) leaves `logger` nil and would panic the moment the leader path runs. cheapest guard is to default nil to `NopLogger()` at the top of `CheckAndRedirectToLeader`, or document the precondition. ########## foreign/go/contracts/logger.go: ########## @@ -0,0 +1,39 @@ +// 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 iggcon + +import ( + "io" + "log/slog" + "os" +) + +func NewLogger(level slog.Level, writer io.Writer) *slog.Logger { Review Comment: two small things on the exported helpers: `NewLogger(level, writer)` flips the usual slog arg order (`slog.NewTextHandler(writer, opts)` is writer-first), so `NewLogger(writer, level)` would read more naturally. also none of `NewLogger`/`NewStderrLogger`/`NopLogger` have doc comments - not a CI failure with the current lint config, but they're exported. ########## foreign/go/client/iggy_client.go: ########## @@ -72,15 +82,24 @@ func NewIggyClient(options ...Option) (iggcon.Client, error) { opt(&opts) } + logger := opts.logger + if logger == nil { + logger = iggcon.NopLogger() + } + + // Prepend the logger option so transport inherits it. + tcpOpts := append([]tcp.Option{tcp.WithLogger(logger)}, opts.tcpOptions...) Review Comment: this prepends `tcp.WithLogger(logger)` then appends the user's `opts.tcpOptions`, so if someone passes both `WithLogger(a)` and `WithTcp(tcp.WithLogger(b))`, the transport ends up logging through `b` (last write wins) while `ic.logger` at line 102 keeps `a` - and the heartbeat at line 132 logs through `a`. so the client and its transport can silently use two different loggers. it's structural, not a precedence thing: the transport's `logger` field is unexported so the client can't read it back, and reordering prepend/append won't fix the split. either route the heartbeat through one governing logger so a single logger covers everything, or document that `WithTcp(tcp.WithLogger)` doesn't govern the client-level heartbeat and `WithLogger` is the unified entry. don't just swap the append order - that would clobber an explicit transport logger. ########## foreign/go/client/iggy_client.go: ########## @@ -110,7 +129,7 @@ func (ic *IggyClient) Connect(ctx context.Context) error { case <-ticker.C: pingCtx, pingCancel := context.WithTimeout(lifetimeCtx, ic.heartbeatInterval/2) if err := ic.Ping(pingCtx); err != nil { - log.Printf("[WARN] heartbeat failed: %v", err) + ic.logger.Warn("heartbeat failed", "err", err) Review Comment: key here is `"err"` but `leader_aware.go` uses `"error"` for the same kind of field. pick one (slog idiom is `"error"`) so the structured keys stay consistent across the SDK. -- 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]
