From ee8cf3e0c5f99cb445c0cea7befa9b661146895e Mon Sep 17 00:00:00 2001
From: jingyih <jingyih@google.com>
Date: Wed, 12 Feb 2020 08:51:20 -0800
Subject: [PATCH 1/2] pkg: remove capnslog

---
 etcdctl/ctlv3/command/ep_command.go |  7 ++++-
 etcdctl/ctlv3/command/global.go     |  6 +++-
 etcdmain/config.go                  | 21 ++++++++------
 pkg/fileutil/fileutil.go            |  4 ---
 pkg/fileutil/purge.go               | 15 ++++------
 pkg/flags/flag.go                   | 43 ++++++++++++++++++-----------
 pkg/flags/flag_test.go              |  8 ++++--
 pkg/flags/ignored.go                |  7 ++++-
 pkg/flags/strings.go                |  3 +-
 pkg/flags/unique_strings.go         |  3 +-
 pkg/flags/unique_urls.go            |  3 +-
 pkg/flags/urls.go                   |  3 +-
 pkg/osutil/interrupt_unix.go        |  2 --
 pkg/osutil/osutil.go                |  4 ---
 pkg/pbutil/pbutil.go                | 10 ++-----
 15 files changed, 78 insertions(+), 61 deletions(-)

diff --git a/etcdctl/ctlv3/command/ep_command.go b/etcdctl/ctlv3/command/ep_command.go
index b04285927b0..5ff9d212611 100644
--- a/etcdctl/ctlv3/command/ep_command.go
+++ b/etcdctl/ctlv3/command/ep_command.go
@@ -25,6 +25,7 @@ import (
 	"go.etcd.io/etcd/pkg/flags"
 
 	"github.com/spf13/cobra"
+	"go.uber.org/zap"
 )
 
 var epClusterEndpoints bool
@@ -85,7 +86,11 @@ type epHealth struct {
 
 // epHealthCommandFunc executes the "endpoint-health" command.
 func epHealthCommandFunc(cmd *cobra.Command, args []string) {
-	flags.SetPflagsFromEnv("ETCDCTL", cmd.InheritedFlags())
+	lg, err := zap.NewProduction()
+	if err != nil {
+		ExitWithError(ExitError, err)
+	}
+	flags.SetPflagsFromEnv(lg, "ETCDCTL", cmd.InheritedFlags())
 	initDisplayFromCmd(cmd)
 
 	sec := secureCfgFromCmd(cmd)
diff --git a/etcdctl/ctlv3/command/global.go b/etcdctl/ctlv3/command/global.go
index 49a22037983..b3ad325c850 100644
--- a/etcdctl/ctlv3/command/global.go
+++ b/etcdctl/ctlv3/command/global.go
@@ -113,6 +113,10 @@ func (*discardValue) Set(string) error { return nil }
 func (*discardValue) Type() string     { return "" }
 
 func clientConfigFromCmd(cmd *cobra.Command) *clientConfig {
+	lg, err := zap.NewProduction()
+	if err != nil {
+		ExitWithError(ExitError, err)
+	}
 	fs := cmd.InheritedFlags()
 	if strings.HasPrefix(cmd.Use, "watch") {
 		// silence "pkg/flags: unrecognized environment variable ETCDCTL_WATCH_KEY=foo" warnings
@@ -120,7 +124,7 @@ func clientConfigFromCmd(cmd *cobra.Command) *clientConfig {
 		fs.AddFlag(&pflag.Flag{Name: "watch-key", Value: &discardValue{}})
 		fs.AddFlag(&pflag.Flag{Name: "watch-range-end", Value: &discardValue{}})
 	}
-	flags.SetPflagsFromEnv("ETCDCTL", fs)
+	flags.SetPflagsFromEnv(lg, "ETCDCTL", fs)
 
 	debug, err := cmd.Flags().GetBool("debug")
 	if err != nil {
diff --git a/etcdmain/config.go b/etcdmain/config.go
index df4c68c1207..2282568dc87 100644
--- a/etcdmain/config.go
+++ b/etcdmain/config.go
@@ -308,20 +308,25 @@ func (cfg *config) parse(arguments []string) error {
 }
 
 func (cfg *config) configFromCmdLine() error {
+	// user-specified logger is not setup yet, use this logger during flag parsing
+	lg, err := zap.NewProduction()
+	if err != nil {
+		return err
+	}
+
 	verKey := "ETCD_VERSION"
 	if verVal := os.Getenv(verKey); verVal != "" {
 		// unset to avoid any possible side-effect.
 		os.Unsetenv(verKey)
-		if lg := cfg.ec.GetLogger(); lg != nil {
-			lg.Warn(
-				"cannot set special environment variable",
-				zap.String("key", verKey),
-				zap.String("value", verVal),
-			)
-		}
+
+		lg.Warn(
+			"cannot set special environment variable",
+			zap.String("key", verKey),
+			zap.String("value", verVal),
+		)
 	}
 
-	err := flags.SetFlagsFromEnv("ETCD", cfg.cf.flagSet)
+	err = flags.SetFlagsFromEnv(lg, "ETCD", cfg.cf.flagSet)
 	if err != nil {
 		return err
 	}
diff --git a/pkg/fileutil/fileutil.go b/pkg/fileutil/fileutil.go
index 5d9fb530395..03d82ee71b6 100644
--- a/pkg/fileutil/fileutil.go
+++ b/pkg/fileutil/fileutil.go
@@ -20,8 +20,6 @@ import (
 	"io/ioutil"
 	"os"
 	"path/filepath"
-
-	"github.com/coreos/pkg/capnslog"
 )
 
 const (
@@ -31,8 +29,6 @@ const (
 	PrivateDirMode = 0700
 )
 
-var plog = capnslog.NewPackageLogger("go.etcd.io/etcd", "pkg/fileutil")
-
 // IsDirWriteable checks if dir is writable by writing and removing a file
 // to dir. It returns nil if dir is writable.
 func IsDirWriteable(dir string) error {
diff --git a/pkg/fileutil/purge.go b/pkg/fileutil/purge.go
index d116f340b6f..e8ac0ca6f58 100644
--- a/pkg/fileutil/purge.go
+++ b/pkg/fileutil/purge.go
@@ -37,6 +37,9 @@ func PurgeFileWithDoneNotify(lg *zap.Logger, dirname string, suffix string, max
 // purgeFile is the internal implementation for PurgeFile which can post purged files to purgec if non-nil.
 // if donec is non-nil, the function closes it to notify its exit.
 func purgeFile(lg *zap.Logger, dirname string, suffix string, max uint, interval time.Duration, stop <-chan struct{}, purgec chan<- string, donec chan<- struct{}) <-chan error {
+	if lg == nil {
+		lg = zap.NewNop()
+	}
 	errC := make(chan error, 1)
 	go func() {
 		if donec != nil {
@@ -67,19 +70,11 @@ func purgeFile(lg *zap.Logger, dirname string, suffix string, max uint, interval
 					return
 				}
 				if err = l.Close(); err != nil {
-					if lg != nil {
-						lg.Warn("failed to unlock/close", zap.String("path", l.Name()), zap.Error(err))
-					} else {
-						plog.Errorf("error unlocking %s when purging file (%v)", l.Name(), err)
-					}
+					lg.Warn("failed to unlock/close", zap.String("path", l.Name()), zap.Error(err))
 					errC <- err
 					return
 				}
-				if lg != nil {
-					lg.Info("purged", zap.String("path", f))
-				} else {
-					plog.Infof("purged file %s successfully", f)
-				}
+				lg.Info("purged", zap.String("path", f))
 				newfnames = newfnames[1:]
 			}
 			if purgec != nil {
diff --git a/pkg/flags/flag.go b/pkg/flags/flag.go
index 215902cf8f3..76a51a89019 100644
--- a/pkg/flags/flag.go
+++ b/pkg/flags/flag.go
@@ -21,18 +21,16 @@ import (
 	"os"
 	"strings"
 
-	"github.com/coreos/pkg/capnslog"
 	"github.com/spf13/pflag"
+	"go.uber.org/zap"
 )
 
-var plog = capnslog.NewPackageLogger("go.etcd.io/etcd", "pkg/flags")
-
 // SetFlagsFromEnv parses all registered flags in the given flagset,
 // and if they are not already set it attempts to set their values from
 // environment variables. Environment variables take the name of the flag but
 // are UPPERCASE, have the given prefix  and any dashes are replaced by
 // underscores - for example: some-flag => ETCD_SOME_FLAG
-func SetFlagsFromEnv(prefix string, fs *flag.FlagSet) error {
+func SetFlagsFromEnv(lg *zap.Logger, prefix string, fs *flag.FlagSet) error {
 	var err error
 	alreadySet := make(map[string]bool)
 	fs.Visit(func(f *flag.Flag) {
@@ -40,17 +38,17 @@ func SetFlagsFromEnv(prefix string, fs *flag.FlagSet) error {
 	})
 	usedEnvKey := make(map[string]bool)
 	fs.VisitAll(func(f *flag.Flag) {
-		if serr := setFlagFromEnv(fs, prefix, f.Name, usedEnvKey, alreadySet, true); serr != nil {
+		if serr := setFlagFromEnv(lg, fs, prefix, f.Name, usedEnvKey, alreadySet, true); serr != nil {
 			err = serr
 		}
 	})
-	verifyEnv(prefix, usedEnvKey, alreadySet)
+	verifyEnv(lg, prefix, usedEnvKey, alreadySet)
 	return err
 }
 
 // SetPflagsFromEnv is similar to SetFlagsFromEnv. However, the accepted flagset type is pflag.FlagSet
 // and it does not do any logging.
-func SetPflagsFromEnv(prefix string, fs *pflag.FlagSet) error {
+func SetPflagsFromEnv(lg *zap.Logger, prefix string, fs *pflag.FlagSet) error {
 	var err error
 	alreadySet := make(map[string]bool)
 	usedEnvKey := make(map[string]bool)
@@ -58,11 +56,11 @@ func SetPflagsFromEnv(prefix string, fs *pflag.FlagSet) error {
 		if f.Changed {
 			alreadySet[FlagToEnv(prefix, f.Name)] = true
 		}
-		if serr := setFlagFromEnv(fs, prefix, f.Name, usedEnvKey, alreadySet, false); serr != nil {
+		if serr := setFlagFromEnv(lg, fs, prefix, f.Name, usedEnvKey, alreadySet, false); serr != nil {
 			err = serr
 		}
 	})
-	verifyEnv(prefix, usedEnvKey, alreadySet)
+	verifyEnv(lg, prefix, usedEnvKey, alreadySet)
 	return err
 }
 
@@ -71,20 +69,29 @@ func FlagToEnv(prefix, name string) string {
 	return prefix + "_" + strings.ToUpper(strings.Replace(name, "-", "_", -1))
 }
 
-func verifyEnv(prefix string, usedEnvKey, alreadySet map[string]bool) {
+func verifyEnv(lg *zap.Logger, prefix string, usedEnvKey, alreadySet map[string]bool) {
 	for _, env := range os.Environ() {
 		kv := strings.SplitN(env, "=", 2)
 		if len(kv) != 2 {
-			plog.Warningf("found invalid env %s", env)
+			if lg != nil {
+				lg.Warn("found invalid environment variable", zap.String("environment-variable", env))
+			}
 		}
 		if usedEnvKey[kv[0]] {
 			continue
 		}
 		if alreadySet[kv[0]] {
-			plog.Fatalf("conflicting environment variable %q is shadowed by corresponding command-line flag (either unset environment variable or disable flag)", kv[0])
+			if lg != nil {
+				lg.Fatal(
+					"conflicting environment variable is shadowed by corresponding command-line flag (either unset environment variable or disable flag))",
+					zap.String("environment-variable", kv[0]),
+				)
+			}
 		}
 		if strings.HasPrefix(env, prefix+"_") {
-			plog.Warningf("unrecognized environment variable %s", env)
+			if lg != nil {
+				lg.Warn("unrecognized environment variable", zap.String("environment-variable", env))
+			}
 		}
 	}
 }
@@ -93,7 +100,7 @@ type flagSetter interface {
 	Set(fk string, fv string) error
 }
 
-func setFlagFromEnv(fs flagSetter, prefix, fname string, usedEnvKey, alreadySet map[string]bool, log bool) error {
+func setFlagFromEnv(lg *zap.Logger, fs flagSetter, prefix, fname string, usedEnvKey, alreadySet map[string]bool, log bool) error {
 	key := FlagToEnv(prefix, fname)
 	if !alreadySet[key] {
 		val := os.Getenv(key)
@@ -102,8 +109,12 @@ func setFlagFromEnv(fs flagSetter, prefix, fname string, usedEnvKey, alreadySet
 			if serr := fs.Set(fname, val); serr != nil {
 				return fmt.Errorf("invalid value %q for %s: %v", val, key, serr)
 			}
-			if log {
-				plog.Infof("recognized and used environment variable %s=%s", key, val)
+			if log && lg != nil {
+				lg.Info(
+					"recognized and used environment variable",
+					zap.String("variable-name", key),
+					zap.String("variable-value", val),
+				)
 			}
 		}
 	}
diff --git a/pkg/flags/flag_test.go b/pkg/flags/flag_test.go
index 3161bbc4755..a176497ae50 100644
--- a/pkg/flags/flag_test.go
+++ b/pkg/flags/flag_test.go
@@ -19,6 +19,8 @@ import (
 	"os"
 	"strings"
 	"testing"
+
+	"go.uber.org/zap"
 )
 
 func TestSetFlagsFromEnv(t *testing.T) {
@@ -47,7 +49,7 @@ func TestSetFlagsFromEnv(t *testing.T) {
 	}
 
 	// now read the env and verify flags were updated as expected
-	err := SetFlagsFromEnv("ETCD", fs)
+	err := SetFlagsFromEnv(zap.NewExample(), "ETCD", fs)
 	if err != nil {
 		t.Errorf("err=%v, want nil", err)
 	}
@@ -66,7 +68,7 @@ func TestSetFlagsFromEnvBad(t *testing.T) {
 	fs := flag.NewFlagSet("testing", flag.ExitOnError)
 	fs.Int("x", 0, "")
 	os.Setenv("ETCD_X", "not_a_number")
-	if err := SetFlagsFromEnv("ETCD", fs); err == nil {
+	if err := SetFlagsFromEnv(zap.NewExample(), "ETCD", fs); err == nil {
 		t.Errorf("err=nil, want != nil")
 	}
 }
@@ -81,7 +83,7 @@ func TestSetFlagsFromEnvParsingError(t *testing.T) {
 	}
 	defer os.Unsetenv("ETCD_HEARTBEAT_INTERVAL")
 
-	err := SetFlagsFromEnv("ETCD", fs)
+	err := SetFlagsFromEnv(zap.NewExample(), "ETCD", fs)
 	for _, v := range []string{"invalid syntax", "parse error"} {
 		if strings.Contains(err.Error(), v) {
 			err = nil
diff --git a/pkg/flags/ignored.go b/pkg/flags/ignored.go
index 9953049000f..9443935354c 100644
--- a/pkg/flags/ignored.go
+++ b/pkg/flags/ignored.go
@@ -14,10 +14,13 @@
 
 package flags
 
+import "go.uber.org/zap"
+
 // IgnoredFlag encapsulates a flag that may have been previously valid but is
 // now ignored. If an IgnoredFlag is set, a warning is printed and
 // operation continues.
 type IgnoredFlag struct {
+	lg   *zap.Logger
 	Name string
 }
 
@@ -27,7 +30,9 @@ func (f *IgnoredFlag) IsBoolFlag() bool {
 }
 
 func (f *IgnoredFlag) Set(s string) error {
-	plog.Warningf(`flag "-%s" is no longer supported - ignoring.`, f.Name)
+	if f.lg != nil {
+		f.lg.Warn("flag is no longer supported - ignoring", zap.String("flag-name", f.Name))
+	}
 	return nil
 }
 
diff --git a/pkg/flags/strings.go b/pkg/flags/strings.go
index 3e47fb38e19..a80190658e4 100644
--- a/pkg/flags/strings.go
+++ b/pkg/flags/strings.go
@@ -16,6 +16,7 @@ package flags
 
 import (
 	"flag"
+	"fmt"
 	"sort"
 	"strings"
 )
@@ -41,7 +42,7 @@ func NewStringsValue(s string) (ss *StringsValue) {
 	}
 	ss = new(StringsValue)
 	if err := ss.Set(s); err != nil {
-		plog.Panicf("new StringsValue should never fail: %v", err)
+		panic(fmt.Sprintf("new StringsValue should never fail: %v", err))
 	}
 	return ss
 }
diff --git a/pkg/flags/unique_strings.go b/pkg/flags/unique_strings.go
index e220ee07a73..e67af1f9b5a 100644
--- a/pkg/flags/unique_strings.go
+++ b/pkg/flags/unique_strings.go
@@ -16,6 +16,7 @@ package flags
 
 import (
 	"flag"
+	"fmt"
 	"sort"
 	"strings"
 )
@@ -60,7 +61,7 @@ func NewUniqueStringsValue(s string) (us *UniqueStringsValue) {
 		return us
 	}
 	if err := us.Set(s); err != nil {
-		plog.Panicf("new UniqueStringsValue should never fail: %v", err)
+		panic(fmt.Sprintf("new UniqueStringsValue should never fail: %v", err))
 	}
 	return us
 }
diff --git a/pkg/flags/unique_urls.go b/pkg/flags/unique_urls.go
index 9b4178c3a14..fde01d2753e 100644
--- a/pkg/flags/unique_urls.go
+++ b/pkg/flags/unique_urls.go
@@ -16,6 +16,7 @@ package flags
 
 import (
 	"flag"
+	"fmt"
 	"net/url"
 	"sort"
 	"strings"
@@ -76,7 +77,7 @@ func NewUniqueURLsWithExceptions(s string, exceptions ...string) *UniqueURLs {
 		return us
 	}
 	if err := us.Set(s); err != nil {
-		plog.Panicf("new UniqueURLs should never fail: %v", err)
+		panic(fmt.Sprintf("new UniqueURLs should never fail: %v", err))
 	}
 	return us
 }
diff --git a/pkg/flags/urls.go b/pkg/flags/urls.go
index ca90970c2b9..6627d8a235c 100644
--- a/pkg/flags/urls.go
+++ b/pkg/flags/urls.go
@@ -16,6 +16,7 @@ package flags
 
 import (
 	"flag"
+	"fmt"
 	"net/url"
 	"strings"
 
@@ -54,7 +55,7 @@ func NewURLsValue(s string) *URLsValue {
 	}
 	v := &URLsValue{}
 	if err := v.Set(s); err != nil {
-		plog.Panicf("new URLsValue should never fail: %v", err)
+		panic(fmt.Sprintf("new URLsValue should never fail: %v", err))
 	}
 	return v
 }
diff --git a/pkg/osutil/interrupt_unix.go b/pkg/osutil/interrupt_unix.go
index 315b2c66d06..4a8f907a8ad 100644
--- a/pkg/osutil/interrupt_unix.go
+++ b/pkg/osutil/interrupt_unix.go
@@ -61,8 +61,6 @@ func HandleInterrupts(lg *zap.Logger) {
 
 		if lg != nil {
 			lg.Info("received signal; shutting down", zap.String("signal", sig.String()))
-		} else {
-			plog.Noticef("received %v signal, shutting down...", sig)
 		}
 
 		for _, h := range ihs {
diff --git a/pkg/osutil/osutil.go b/pkg/osutil/osutil.go
index f5c63e5e017..cbf96e2e04d 100644
--- a/pkg/osutil/osutil.go
+++ b/pkg/osutil/osutil.go
@@ -18,13 +18,9 @@ package osutil
 import (
 	"os"
 	"strings"
-
-	"github.com/coreos/pkg/capnslog"
 )
 
 var (
-	plog = capnslog.NewPackageLogger("go.etcd.io/etcd", "pkg/osutil")
-
 	// support to override setting SIG_DFL so tests don't terminate early
 	setDflSignal = dflSignal
 )
diff --git a/pkg/pbutil/pbutil.go b/pkg/pbutil/pbutil.go
index 53167ffa51c..821f59703ae 100644
--- a/pkg/pbutil/pbutil.go
+++ b/pkg/pbutil/pbutil.go
@@ -15,11 +15,7 @@
 // Package pbutil defines interfaces for handling Protocol Buffer objects.
 package pbutil
 
-import "github.com/coreos/pkg/capnslog"
-
-var (
-	plog = capnslog.NewPackageLogger("go.etcd.io/etcd", "pkg/pbutil")
-)
+import "fmt"
 
 type Marshaler interface {
 	Marshal() (data []byte, err error)
@@ -32,14 +28,14 @@ type Unmarshaler interface {
 func MustMarshal(m Marshaler) []byte {
 	d, err := m.Marshal()
 	if err != nil {
-		plog.Panicf("marshal should never fail (%v)", err)
+		panic(fmt.Sprintf("marshal should never fail (%v)", err))
 	}
 	return d
 }
 
 func MustUnmarshal(um Unmarshaler, data []byte) {
 	if err := um.Unmarshal(data); err != nil {
-		plog.Panicf("unmarshal should never fail (%v)", err)
+		panic(fmt.Sprintf("unmarshal should never fail (%v)", err))
 	}
 }
 

From 81d9872a889739038ada5eef483770d6efc75462 Mon Sep 17 00:00:00 2001
From: jingyih <jingyih@google.com>
Date: Wed, 12 Feb 2020 10:22:02 -0800
Subject: [PATCH 2/2] CHANGELOG: function signature change

---
 CHANGELOG-3.5.md | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/CHANGELOG-3.5.md b/CHANGELOG-3.5.md
index 5371cd39070..3e25486ed4c 100644
--- a/CHANGELOG-3.5.md
+++ b/CHANGELOG-3.5.md
@@ -53,6 +53,9 @@ See [code changes](https://github.com/etcd-io/etcd/compare/v3.4.0...v3.5.0) and
   - Previously, `NewClusterProxy(c *clientv3.Client, advaddr string, prefix string) (pb.ClusterServer, <-chan struct{})`, now `NewClusterProxy(lg *zap.Logger, c *clientv3.Client, advaddr string, prefix string) (pb.ClusterServer, <-chan struct{})`.
   - Previously, `Register(c *clientv3.Client, prefix string, addr string, ttl int)`, now `Register(lg *zap.Logger, c *clientv3.Client, prefix string, addr string, ttl int) <-chan struct{}`.
   - Previously, `NewHandler(t *http.Transport, urlsFunc GetProxyURLs, failureWait time.Duration, refreshInterval time.Duration) http.Handler`, now `NewHandler(lg *zap.Logger, t *http.Transport, urlsFunc GetProxyURLs, failureWait time.Duration, refreshInterval time.Duration) http.Handler`.
+- Changed `pkg/flags` function signature to [support structured logger](https://github.com/etcd-io/etcd/pull/11616).
+  - Previously, `SetFlagsFromEnv(prefix string, fs *flag.FlagSet) error`, now `SetFlagsFromEnv(lg *zap.Logger, prefix string, fs *flag.FlagSet) error`.
+  - Previously, `SetPflagsFromEnv(prefix string, fs *pflag.FlagSet) error`, now `SetPflagsFromEnv(lg *zap.Logger, prefix string, fs *pflag.FlagSet) error`.
 
 ### Metrics, Monitoring