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.

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 50.00000% with 22 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.64%. Comparing base (a8dd3a9) to head (7b74893).
⚠️ Report is 11 commits behind head on master.

Files with missing lines	Patch %	Lines
foreign/go/internal/util/leader_aware.go	38.23%	21 Missing ⚠️
foreign/go/client/iggy_client.go	85.71%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #3379      +/-   ##
============================================
- Coverage     74.65%   74.64%   -0.01%     
  Complexity      943      943              
============================================
  Files          1228     1228              
  Lines        120529   120559      +30     
  Branches      97263    97263              
============================================
+ Hits          89975    89993      +18     
- Misses        27614    27631      +17     
+ Partials       2940     2935       -5

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.24% <50.00%> (+0.03%)`	⬆️

Files with missing lines	Coverage Δ
foreign/go/client/tcp/tcp_core.go	`54.37% <100.00%> (+0.17%)`	⬆️
foreign/go/client/tcp/tcp_session_management.go	`55.38% <100.00%> (+0.69%)`	⬆️
foreign/go/client/iggy_client.go	`67.27% <85.71%> (+3.27%)`	⬆️
foreign/go/internal/util/leader_aware.go	`34.93% <38.23%> (-1.73%)`	⬇️

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

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-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,

slash-init · 2026-06-03T06:06:35Z

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.

Makes sense. If we remove logger.go and logger_test.go, where would you prefer the replacement tests to live? I was thinking of moving them closer to the actual usage and testing WithLogger(...) behavior in the client/TCP tests instead, but wanted to check if that's what you had in mind.

chengxilo · 2026-06-03T14:59:01Z

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.

Makes sense. If we remove logger.go and logger_test.go, where would you prefer the replacement tests to live? I was thinking of moving them closer to the actual usage and testing WithLogger(...) behavior in the client/TCP tests instead, but wanted to check if that's what you had in mind.

Yeah, great choice, under the foreign/go/client and foreign/go/client/tcp is appropriate.

slash-init · 2026-06-04T09:42:55Z

I removed the custom logger helpers and moved the default logger into GetDefaultOptions(). Per Chengxilo's suggestion, I kept the logger parameter mandatory and removed tcp.WithLogger, so the TCP client now receives the logger directly and the client and transport always use the same logger. I also added tests covering the logger behavior in both the client and TCP packages as well.

chengxilo

Just a few changes needed. Mainly the nil guard

chengxilo · 2026-06-05T03:50:53Z

+func TestGetDefaultOptions_ProvidesLogger(t *testing.T) {
+	opts := GetDefaultOptions()
+	if opts.logger == nil {
+		t.Fatal("expected default options to include a non-nil logger")
+	}
+}


Suggested change

func TestGetDefaultOptions_ProvidesLogger(t *testing.T) {

opts := GetDefaultOptions()

if opts.logger == nil {

t.Fatal("expected default options to include a non-nil logger")

}

}

unnecessary

chengxilo · 2026-06-05T03:51:41Z

+	ic, ok := cli.(*IggyClient)
+	if !ok {
+		t.Fatal("expected *IggyClient from NewIggyClient")
+	}


Suggested change

ic, ok := cli.(*IggyClient)

if !ok {

t.Fatal("expected *IggyClient from NewIggyClient")

}

ic := cli.(*IggyClient)

it will always be a IggyClient so the check is not needed.

…out SDK

…rt/client split

slash-init · 2026-06-05T05:39:55Z

I've added the nil guard for WithLogger(nil), updated the logger tests, and removed the unnecessary default-options test.

slash-init · 2026-06-05T05:40:11Z

/ready

chengxilo · 2026-06-05T17:52:00Z

Just minor typo issue. lgtm now.

We still need to add the logs in the tcp client to align with rust sdk. But it can be done in another PR (Or if you prefer, we can do it here too). We can open an issue when this is done.

Co-authored-by: Chengxi Luo <chengxi.luo2004@gmail.com>

slash-init · 2026-06-05T18:13:41Z

Just minor typo issue. lgtm now.

We still need to add the logs in the tcp client to align with rust sdk. But it can be done in another PR (Or if you prefer, we can do it here too). We can open an issue when this is done.

Thanks. I missed that typo too.
I guess let's keep it for another PR since the conversation history here is getting a bit too long haha.

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

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 requested a review from hubcio June 1, 2026 13:38

chengxilo suggested changes Jun 2, 2026

View reviewed changes

slash-init requested a review from chengxilo June 4, 2026 09:43

chengxilo reviewed Jun 5, 2026

View reviewed changes

slash-init added 7 commits June 5, 2026 10:53

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

a01f939

…out SDK

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

6e2bd67

refactor(go-sdk) : use slog directly

b649bd2

test(go-sdk): add logger unit tests

b030657

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

eb85b1f

…rt/client split

refactor(go): unify logger configuration and remove helpers

fe2473e

fix(go): guard WithLogger against nil and clean up tests

0ed2244

slash-init force-pushed the feat/go-sdk-logger branch from 60897bd to 0ed2244 Compare June 5, 2026 05:29

slash-init requested a review from chengxilo June 5, 2026 05:40

github-actions Bot removed the S-waiting-on-author PR is waiting on author response label Jun 5, 2026

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

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

c96fa1c

chengxilo reviewed Jun 5, 2026

View reviewed changes

Comment thread foreign/go/client/iggy_client_test.go Outdated

test(go-sdk): fix typo in logger test

93497bd

Co-authored-by: Chengxi Luo <chengxi.luo2004@gmail.com>

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

chengxilo Jun 2, 2026

Choose a reason for hiding this comment

Uh oh!

chengxilo Jun 2, 2026

Choose a reason for hiding this comment

Uh oh!

chengxilo Jun 2, 2026

Choose a reason for hiding this comment

Uh oh!

chengxilo Jun 2, 2026

Choose a reason for hiding this comment

Uh oh!

slash-init commented Jun 3, 2026

Uh oh!

chengxilo commented Jun 3, 2026

Uh oh!

slash-init commented Jun 4, 2026

Uh oh!

chengxilo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chengxilo Jun 5, 2026

Choose a reason for hiding this comment

Uh oh!

chengxilo Jun 5, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

slash-init commented Jun 5, 2026

Uh oh!

slash-init commented Jun 5, 2026

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

slash-init commented Jun 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

codecov Bot commented Jun 1, 2026 •

edited

Loading

chengxilo left a comment •

edited

Loading

chengxilo Jun 2, 2026 •

edited

Loading

chengxilo commented Jun 5, 2026 •

edited

Loading