Bug Report: etcd2topo zap logger creation allocates a lot of memory

[mdlayher]

Overview of the Issue

An internal profile indicated that when lots of etcd2 topo connections are open, the default constructor will create a zap logger for each connection. This ultimately resulted in a huge amount of memory allocations for logs which were not being consumed in any way.

After plumbing in a single nop logger internally, memory allocations plummeted:

var zapNop = zap.NewNop()

// NewServerWithOpts creates a new server with the provided TLS options
func NewServerWithOpts(serverAddr, root, certPath, keyPath, caPath string) (*Server, error) {
	// TODO: Rename this to NewServer and change NewServer to a name that signifies it uses the process-wide TLS settings.
	config := clientv3.Config{
		Endpoints:   strings.Split(serverAddr, ","),
		DialTimeout: 5 * time.Second,
		DialOptions: []grpc.DialOption{grpc.WithBlock()},

		// Discard logs from the etcdv3 client. Leaving the logger
		// nil here results in allocating a new logger and its metadata each
		// time we open a new topo connection.
		Logger: zapNop,
	}
	
	// ...

I'm opening this issue for discussion as to how to fix this in a way that is appropriate for open source Vitess.

Perhaps it would be reasonable to construct a single zap logger with known-good defaults and then we can use that throughout.

An example of where this optimization would apply would be the vitess-operator's toposerver pool: https://github.com/planetscale/vitess-operator/tree/main/pkg/operator/toposerver.

Reproduction Steps

Collect a pprof profile of a service importing the vitess topo libraries.

Binary Version

n/a

Operating System and Environment details

n/a

Log Fragments

n/a