Support client configuration via environmental variables

[gyuho]

As of etcd 3.4, we support ETCD_CLIENT_DEBUG to configure logging level.

etcd/clientv3/client.go

Lines 50 to 55 in 2702f9e

	func init() {
	lg := zap.NewNop()
	if os.Getenv("ETCD_CLIENT_DEBUG") != "" {
	lcfg := logutil.DefaultZapLoggerConfig
	lcfg.Level = zap.NewAtomicLevelAt(zap.DebugLevel)

We should make this more generic to support other options.

One use case is to configure our auto-sync interval, as it becomes harder to expose individual configuration via flags (e.g., kube-apiserver needs to add a new flag to support this).

ref. kubernetes/kubernetes#64742 and kubernetes/kubernetes#64746

/cc @jpbetz @ptabor @xiang90 @jqmichael

---

@ptabor Seems like we've accidentally removed this code as we upgraded gRPC for etcd 3.5?

etcd/client/v3/doc.go

Lines 102 to 104 in 4d9f1a9

	// The grpc load balancer is registered statically and is shared across etcd clients.
	// To enable detailed load balancer logging, set the ETCD_CLIENT_DEBUG environment
	// variable. E.g. "ETCD_CLIENT_DEBUG=1".

[ptabor]

@gyuho:

It was not accidental. I think that the flag had nearly no control in v3.4.

The flag was driving only global clientv3.lg logger:

etcd/clientv3/client.go

Lines 50 to 53 in 2702f9e

	func init() {
	lg := zap.NewNop()
	if os.Getenv("ETCD_CLIENT_DEBUG") != "" {
	lcfg := logutil.DefaultZapLoggerConfig

That was used only in 1 place.

All other logging was done through class-level logger (c.lg) configured here:

etcd/clientv3/client.go

Line 430 in 2702f9e

client.lg, err = lcfg.Build()

So I think that this flag should get replaced by generic mechanism of passing clientv3 config.
I would consider a single flag that carries json and that json got merged with app-given spec using [JSON-merge-patch] (https://tools.ietf.org/html/rfc7386).

[gyuho]

I think that the flag had nearly no control in v3.4.

It can be used to configure client-side logging level configurable (e.g., zap.NewAtomicLevelAt(zap.DebugLevel))?

ETCD_CLIENT_DEBUG=1 ETCDCTL_API=3 etcdctl --endpoints ETCD_HOST:2379 get foo

will help us debug gRPC layer logging (e.g., create subconn).

Now, we cannot override this (unless we use WithLogger in Go)?

etcd/client/v3/client.go

Lines 332 to 340 in 63c5170

	lcfg := logutil.DefaultZapLoggerConfig
	if cfg.LogConfig != nil {
	lcfg = *cfg.LogConfig
	}
	var err error
	client.lg, err = lcfg.Build()
	if err != nil {
	return nil, err
	}

We should add ETCD_CLIENT_DEBUG back with more generic solution.

[ptabor]

1. It does not require WithLogger.

The custom logging level can be configured using client-config & zap-config (that is JSON serializable):

  lcfg := logutil.DefaultZapLoggerConfig
  lcfg.Level = zap.NewAtomicLevelAt(zap.DebugLevel)
  ccfg := clientv3.Config{LogConfig: &lcfg}
  cli, err := clientv3.New(ccfg)

(I'm not saying that we shouldn't have env-veriable that drives the default behavior, just showing that this is not
different from all the other properties configuring client).

2. As grpclog config is global (not per client/connection) and not-threadsafe, we should reconfigure it on per-client basis.
   We might offer explicit helper methods how to configure it, but it should be explicit call from the user.

I contributed some work-around for the thread-safety problem in
grpc-ecosystem/go-grpc-middleware#402,
but its suitable for tests rather then implicit use in prod (where user can have other services and other preferred grpc config).

[ptabor]

Provided fix to maintain the feature in 3.5: https://github.com/etcd-io/etcd/pull/12786/commits

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed after 21 days if no further activity occurs. Thank you for your contributions.