feat(go): implement golang sdk logger by slash-init · Pull Request #3379 · apache/iggy

slash-init · 2026-05-30T02:51:18Z

Which issue does this PR address?

Rationale

The SDK currently logs to stdout using log.Printf and users have no way to control
it. They can't turn it off, change the log level, or redirect it. This PR adds a
configurable logger so users can decide whether to enable logging, what level of
detail they want, and where the logs should go.

What changed?

The SDK was printing log messages directly to stdout with no user control.

This PR adds a Logger interface that users can pass to the client via WithLogger().
By default, logging is silent (zero overhead). When enabled, users can choose their
log level (Debug, Info, Warn, Error) and output destination (stderr, file, custom).
All internal log.Printf calls have been replaced with logger method calls.

Local Execution

Passed

Ran go vet ./... - no issues
Ran gofmt - all files formatted correctly
Ran golangci-lint - no errors or warnings
Ran go build ./... - builds successfully
Ran go test ./contracts ./client/tcp ./internal/util - all tests pass

AI Usage

Tool: Claude Sonnet 4.6

Scope: Generated the logger interface, DefaultLogger and NoopLogger implementations. All generated code was reviewed, integrated and tested manually.

Verification: All code compiles and tests pass locally.

…out SDK

github-actions · 2026-05-30T02:51:28Z

Thanks for the PR. It is labeled S-waiting-on-review and queued for review.

Slash commands (own line, regular comment) move it around the queue:

/ready - back to S-waiting-on-review after addressing feedback
/request-review @user-or-team - request a reviewer

See CONTRIBUTING.md for details.

chengxilo · 2026-05-31T02:43:26Z

I was doing code review but realized that you are still working on it since github tell me to refresh the PR. However, I do found some problem. I'm sorry when I wrote the misleading proposal since it's just a small silly example.

I did some small research on it and realized that there is something new about go log system and it would probably help: https://go.dev/blog/slog. I think it can be used directly to replace the iggcon.Logger interface. we just need to use *slog.Logger as the logger interface, perhaps.

slash-init · 2026-05-31T02:45:11Z

I was doing code review but realized that you are still working on it since github tell me to refresh the PR. However, I do found some problem. I'm sorry when I wrote the misleading proposal since it's just a small silly example.

I did some small research on it and realized that there is something new about go log system and it would probably help: https://go.dev/blog/slog. I think it can be used directly to replace the iggcon.Logger interface. we just need to use *slog.Logger as the logger interface, perhaps.

Thanks for the clarification. I introduced the interface based on the original proposal, but I agree that using *slog.Logger directly would keep things simpler and align with the standard library.
I'll update the implementation to use slog directly unless you have a different approach in mind.

chengxilo · 2026-05-31T02:50:33Z

By the way, test is missing. So if you don't mind please provide unit test for this PR.

codecov · 2026-06-01T09:02:33Z

Codecov Report

❌ Patch coverage is 52.45902% with 29 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.64%. Comparing base (a8dd3a9) to head (9b5a03a).

Files with missing lines	Patch %	Lines
foreign/go/internal/util/leader_aware.go	36.11%	22 Missing and 1 partial ⚠️
foreign/go/client/iggy_client.go	60.00%	4 Missing ⚠️
foreign/go/client/tcp/tcp_core.go	71.42%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #3379      +/-   ##
============================================
- Coverage     74.65%   74.64%   -0.01%     
  Complexity      943      943              
============================================
  Files          1228     1229       +1     
  Lines        120529   120577      +48     
  Branches      97263    97263              
============================================
+ Hits          89975    90004      +29     
- Misses        27614    27636      +22     
+ Partials       2940     2937       -3

Components	Coverage Δ
Rust Core	`75.85% <ø> (+<0.01%)`	⬆️
Java SDK	`58.44% <ø> (ø)`
C# SDK	`69.87% <ø> (+0.01%)`	⬆️
Python SDK	`81.06% <ø> (ø)`
Node SDK	`91.53% <ø> (ø)`
Go SDK	`40.31% <52.45%> (+0.11%)`	⬆️

Files with missing lines	Coverage Δ
foreign/go/client/tcp/tcp_session_management.go	`55.38% <100.00%> (+0.69%)`	⬆️
foreign/go/contracts/logger.go	`100.00% <100.00%> (ø)`
foreign/go/client/tcp/tcp_core.go	`54.64% <71.42%> (+0.44%)`	⬆️
foreign/go/client/iggy_client.go	`63.79% <60.00%> (-0.21%)`	⬇️
foreign/go/internal/util/leader_aware.go	`34.11% <36.11%> (-2.55%)`	⬇️

... and 8 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

hubcio · 2026-06-01T10:59:06Z

+}
+
+func NopLogger() *slog.Logger {
+	return slog.New(slog.NewTextHandler(io.Discard, nil))


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.

hubcio · 2026-06-01T10:59:06Z

+	return slog.New(handler)
+}
+
+func NewStderrLogger(level slog.Level) *slog.Logger {


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).

hubcio · 2026-06-01T10:59:06Z

+	"os"
+)
+
+func NewLogger(level slog.Level, writer io.Writer) *slog.Logger {


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.

hubcio · 2026-06-01T10:59:06Z

-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")


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.

hubcio · 2026-06-01T10:59:06Z

+	}
+
+	// Prepend the logger option so transport inherits it.
+	tcpOpts := append([]tcp.Option{tcp.WithLogger(logger)}, opts.tcpOptions...)


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.

I went with the documentation approach for now and clarified the behavior of WithLogger(...) vs tcp.WithLogger(...). WithLogger(...) remains the client-level logger used by heartbeat logging and is forwarded to the transport as a default, while an explicit tcp.WithLogger(...) only overrides the transport logger.

Another approach would be remove tcp.WithLogger, and change the signature of NewIggyTcpClient to
func NewIggyTcpClient(logger *slog.Logger, options ...Option) *IggyTcpClient. logger actually is not a optional parameter, we always use NewIggyClient to create TCP client, and we always provide a logger to it. This is actually better than document it. (in my opinion)

hubcio · 2026-06-01T10:59:06Z

 					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)


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.

Pushed the requested changes.

NopLogger uses slog.DiscardHandler (zero formatting, zero alloc)

dropped NewStderrLogger and its test

flipped NewLogger to (writer, level) and added doc comments

added a nil guard on the logger at the top of CheckAndRedirectToLeader

documented the WithLogger / tcp.WithLogger split on WithTcp, WithLogger, and tcp.WithLogger

changed the heartbeat key to "error" for consistency with leader_aware.go

…rt/client split

chengxilo

I think both logger.go and logger_test.go can be removed. But it doesn't mean we don't need test. Actually, I think it would be great if we can test wither the WithLogger works.

NopLogger() can be replaced with slog.New(slog.DiscardHandler) directly, very straight forward and easy to understand.

chengxilo · 2026-06-02T02:18:23Z

+	logger := opts.logger
+	if logger == nil {
+		logger = iggcon.NopLogger()
+	}


This if statement can be removed. We can just move the default value into the GetDefaultOptions() at R46.

chengxilo · 2026-06-02T02:20:58Z


 func GetDefaultOptions() Options {
 	return Options{
 		config: defaultTcpClientConfig(),


Suggested change

config: defaultTcpClientConfig(),

logger: iggcon.NopLogger() // Or just slog.New(slog.DiscardHandler) should be better in my opinion

chengxilo · 2026-06-02T03:47:43Z

+	logger := opts.logger
+	if logger == nil {
+		logger = iggcon.NopLogger()
+	}


same here, move it to GetDefaultOptions(). We set NopLogger() in default options, and let the

for _, opt := range options { opt(&opts) }

to overwrite it.

chengxilo · 2026-06-02T04:08:10Z

+// NopLogger returns a logger that silently discards all output.
+func NopLogger() *slog.Logger {
+	return slog.New(slog.DiscardHandler)
+}


Maybe we can delete logger.go and logger_test.go. If we use *slog.Logger, we can just inline slog.New(slog.DiscardHandler) for default logger. And for user who want to see the logs, they can do

client.NewIggyClient(client.WithLogger(slog.New(slog.NewTextHandler( os.Stdout, &slog.HandlerOptions{ Level: slog.LevelDebug, }))))

So the NewLogger won't be actually used.

The NopLogger makes sense, but I think it's also fine to just inline

slog.New(slog.DiscardHandler)

So we can remove both of them, and the corresponding tests.

chengxilo · 2026-06-02T04:10:58Z

+func CheckAndRedirectToLeader(ctx context.Context, c iggcon.Client, currentAddress string, transport iggcon.Protocol, logger *slog.Logger) (string, error) {
+	if logger == nil {
+		logger = iggcon.NopLogger()
+	}


Regarding here, if you change the GetDefaultOptions(), it should be safe to remove the nil check.

chengxilo · 2026-06-02T04:17:02Z

+
 	return &IggyTcpClient{
 		config:                 opts.config,
+		logger:                 logger,


Suggested change

logger: logger,

logger: opts.logger,

feat(go-sdk): add configurable logger interface and integrate through…

6773b71

…out SDK

github-actions Bot added the S-waiting-on-review PR is waiting on a reviewer label May 30, 2026

chengxilo reviewed May 31, 2026

View reviewed changes

Comment thread foreign/go/client/tcp/tcp_core.go Outdated

fix(go-sdk) : remove duplicate logger field from config struct

d1e9b27

slash-init added 2 commits May 31, 2026 09:04

refactor(go-sdk) : use slog directly

9d5ca37

test(go-sdk): add logger unit tests

7e5199f

slash-init changed the title ~~feat(go-sdk): implement golang sdk logger~~ feat(go): implement golang sdk logger Jun 1, 2026

hubcio requested changes Jun 1, 2026

View reviewed changes

github-actions Bot added S-waiting-on-author PR is waiting on author response and removed S-waiting-on-review PR is waiting on a reviewer labels Jun 1, 2026

slash-init added 2 commits June 1, 2026 18:25

Merge branch 'master' into feat/go-sdk-logger

a56ab5c

fix(go) : harden logger helpers, guard nil path, and document transpo…

9b5a03a

…rt/client split

slash-init requested a review from hubcio June 1, 2026 13:38

chengxilo suggested changes Jun 2, 2026

View reviewed changes


	config: defaultTcpClientConfig(),
	logger: iggcon.NopLogger() // Or just slog.New(slog.DiscardHandler) should be better in my opinion

Conversation

slash-init commented May 30, 2026

Which issue does this PR address?

Rationale

What changed?

Local Execution

AI Usage

Uh oh!

github-actions Bot commented May 30, 2026

Uh oh!

Uh oh!

chengxilo commented May 31, 2026

Uh oh!

slash-init commented May 31, 2026

Uh oh!

chengxilo commented May 31, 2026

Uh oh!

codecov Bot commented Jun 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chengxilo Jun 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chengxilo left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chengxilo Jun 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov Bot commented Jun 1, 2026 •

edited

Loading

chengxilo Jun 2, 2026 •

edited

Loading

chengxilo left a comment •

edited

Loading

chengxilo Jun 2, 2026 •

edited

Loading