From 6c818f5ad9e3c2dbf812327df9abb735ef3a1f75 Mon Sep 17 00:00:00 2001
From: Yuxuan 'fishy' Wang <yuxuan.wang@reddit.com>
Date: Thu, 15 Aug 2024 09:14:12 -0700
Subject: [PATCH] Add omitempty yaml tag to deprecated log.Wrapper fields

Also implement log.Wrapper.MarshalYAML for nil values.

Also add `yaml:"-"` explicitly for config struct fields that's not
yaml-unmarshal-able.
---
 filewatcher/filewatcher.go |  2 +-
 log/wrapper.go             | 12 ++++++++++++
 log/wrapper_test.go        | 28 ++++++++++++++++++++++++++++
 thriftbp/client_pool.go    |  8 ++++----
 4 files changed, 45 insertions(+), 5 deletions(-)

diff --git a/filewatcher/filewatcher.go b/filewatcher/filewatcher.go
index 75cae4658..e0fe93943 100644
--- a/filewatcher/filewatcher.go
+++ b/filewatcher/filewatcher.go
@@ -119,7 +119,7 @@ type Config struct {
 	// call, which will be returned directly.
 	//
 	// Deprecated: Errors will be logged via slog at error level instead.
-	Logger log.Wrapper `yaml:"logger"`
+	Logger log.Wrapper `yaml:"logger,omitempty"`
 
 	// Optional. When <=0 DefaultMaxFileSize will be used instead.
 	//
diff --git a/log/wrapper.go b/log/wrapper.go
index 29857c4b7..4a96c1769 100644
--- a/log/wrapper.go
+++ b/log/wrapper.go
@@ -179,6 +179,18 @@ func (w *Wrapper) UnmarshalText(text []byte) error {
 	return nil
 }
 
+// MarshalYAML implements yaml.Marshaler (both v2 and v3).
+//
+// It only support nil values, as it's almost impossible to detect which non-nil
+// value it really is to marshal to some meaningful value that can be
+// unmarshaled.
+func (w Wrapper) MarshalYAML() (any, error) {
+	if w == nil {
+		return nil, nil
+	}
+	return nil, errors.New("log.Wrapper.MarshalYAML only supports nil value")
+}
+
 var _ encoding.TextUnmarshaler = (*Wrapper)(nil)
 
 // WrapToThriftLogger wraps a Wrapper into thrift.Logger.
diff --git a/log/wrapper_test.go b/log/wrapper_test.go
index 3f22765cc..333978e68 100644
--- a/log/wrapper_test.go
+++ b/log/wrapper_test.go
@@ -10,6 +10,7 @@ import (
 	"github.com/go-kit/kit/metrics"
 	"github.com/prometheus/client_golang/prometheus"
 	"github.com/reddit/baseplate.go/log"
+	"gopkg.in/yaml.v2"
 )
 
 func TestLogWrapperNilSafe(t *testing.T) {
@@ -102,6 +103,33 @@ func TestLogWrapperUnmarshalText(t *testing.T) {
 	}
 }
 
+func TestWrapperMarshalYAML(t *testing.T) {
+	type foo struct {
+		Log log.Wrapper `yaml:"log,omitempty"`
+	}
+	t.Run("nil", func(t *testing.T) {
+		bar := foo{Log: nil}
+		var sb strings.Builder
+		if err := yaml.NewEncoder(&sb).Encode(bar); err != nil {
+			t.Fatal(err)
+		}
+		if got, want := strings.TrimSpace(sb.String()), `log: null`; got != want {
+			t.Errorf("yaml got %q want %q", got, want)
+		}
+	})
+	t.Run("non-nil", func(t *testing.T) {
+		bar := foo{Log: log.NopWrapper}
+		var sb strings.Builder
+		if err := yaml.NewEncoder(&sb).Encode(bar); err == nil {
+			t.Errorf("Expected marshal error, got yaml %q", sb.String())
+		}
+	})
+}
+
+var (
+	_ yaml.Marshaler = (log.Wrapper)(nil)
+)
+
 var (
 	_ log.Counter = (prometheus.Counter)(nil)
 	_ log.Counter = (metrics.Counter)(nil)
diff --git a/thriftbp/client_pool.go b/thriftbp/client_pool.go
index 48a58b57b..8898ec791 100644
--- a/thriftbp/client_pool.go
+++ b/thriftbp/client_pool.go
@@ -95,7 +95,7 @@ type ClientPoolConfig struct {
 	InitialConnectionsFallback bool `yaml:"initialConnectionsFallback"`
 	// Deprecated: Individual connection errors during initialization is always
 	// logged via zap logger on warning level.
-	InitialConnectionsFallbackLogger log.Wrapper `yaml:"initialConnectionsFallbackLogger"`
+	InitialConnectionsFallbackLogger log.Wrapper `yaml:"initialConnectionsFallbackLogger,omitempty"`
 
 	// MaxConnections is the maximum number of thrift connections the client
 	// pool can maintain.
@@ -171,7 +171,7 @@ type ClientPoolConfig struct {
 	// retry.Attempts(1).  This sets up the retry middleware but does not
 	// automatically retry any requests.  You can set retry behavior per-call by
 	// using retrybp.WithOptions.
-	DefaultRetryOptions []retry.Option
+	DefaultRetryOptions []retry.Option `yaml:"-"`
 
 	// ReportPoolStats signals to the ClientPool that it should report
 	// statistics on the underlying clientpool.Pool in a background
@@ -206,7 +206,7 @@ type ClientPoolConfig struct {
 	// See MonitorClientArgs.ErrorSpanSuppressor for more details.
 	//
 	// This is optional. If it's not set IDLExceptionSuppressor will be used.
-	ErrorSpanSuppressor errorsbp.Suppressor
+	ErrorSpanSuppressor errorsbp.Suppressor `yaml:"-"`
 
 	// When BreakerConfig is non-nil,
 	// a breakerbp.FailureRatioBreaker will be created for the pool,
@@ -216,7 +216,7 @@ type ClientPoolConfig struct {
 	// The edge context implementation. Optional.
 	//
 	// If it's not set, the global one from ecinterface.Get will be used instead.
-	EdgeContextImpl ecinterface.Interface
+	EdgeContextImpl ecinterface.Interface `yaml:"-"`
 
 	// The name for the server to identify this client,
 	// via the "User-Agent" (HeaderUserAgent) THeader.