From 719e157768b570a448db5ffbe757e28c7c9c0b99 Mon Sep 17 00:00:00 2001 From: Noah Date: Mon, 21 Aug 2023 15:22:50 +0100 Subject: [PATCH 1/3] Avoid memory leaking in `otelgrpc` interceptors --- api/client/client.go | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/api/client/client.go b/api/client/client.go index e437b6017d73d..2188bcaf0bc6b 100644 --- a/api/client/client.go +++ b/api/client/client.go @@ -391,12 +391,12 @@ func (c *Client) dialGRPC(ctx context.Context, addr string) error { dialOpts = append(dialOpts, grpc.WithContextDialer(c.grpcDialer())) dialOpts = append(dialOpts, grpc.WithChainUnaryInterceptor( - otelgrpc.UnaryClientInterceptor(), + unaryClientInterceptorOnce(), metadata.UnaryClientInterceptor, breaker.UnaryClientInterceptor(cb), ), grpc.WithChainStreamInterceptor( - otelgrpc.StreamClientInterceptor(), + streamClientInterceptorOnce(), metadata.StreamClientInterceptor, breaker.StreamClientInterceptor(cb), ), @@ -422,6 +422,19 @@ func (c *Client) dialGRPC(ctx context.Context, addr string) error { return nil } +// We wrap the creation of the otelgrpc interceptors in a sync.Once - this is +// because each time this is called, they create a new underlying metric. If +// something (e.g tbot) is repeatedly creating new clients and closing them, +// then this leads to a memory leak since the underlying metric is not cleaned +// up. +var streamClientInterceptorOnce = sync.OnceValue(func() grpc.StreamClientInterceptor { + return otelgrpc.StreamClientInterceptor() +}) + +var unaryClientInterceptorOnce = sync.OnceValue(func() grpc.UnaryClientInterceptor { + return otelgrpc.UnaryClientInterceptor() +}) + // ConfigureALPN configures ALPN SNI cluster routing information in TLS settings allowing for // allowing to dial auth service through Teleport Proxy directly without using SSH Tunnels. func ConfigureALPN(tlsConfig *tls.Config, clusterName string) *tls.Config { From 58ffb11371c4b63c3d8e48ba849665e052c0a54c Mon Sep 17 00:00:00 2001 From: Noah Date: Mon, 21 Aug 2023 15:51:30 +0100 Subject: [PATCH 2/3] Fix for non go 1.21 usage --- api/client/client.go | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/api/client/client.go b/api/client/client.go index 2188bcaf0bc6b..d480a032e47a5 100644 --- a/api/client/client.go +++ b/api/client/client.go @@ -391,12 +391,12 @@ func (c *Client) dialGRPC(ctx context.Context, addr string) error { dialOpts = append(dialOpts, grpc.WithContextDialer(c.grpcDialer())) dialOpts = append(dialOpts, grpc.WithChainUnaryInterceptor( - unaryClientInterceptorOnce(), + otelUnaryClientInterceptor(), metadata.UnaryClientInterceptor, breaker.UnaryClientInterceptor(cb), ), grpc.WithChainStreamInterceptor( - streamClientInterceptorOnce(), + otelStreamClientInterceptor(), metadata.StreamClientInterceptor, breaker.StreamClientInterceptor(cb), ), @@ -422,16 +422,32 @@ func (c *Client) dialGRPC(ctx context.Context, addr string) error { return nil } +// TODO(noah): Once we upgrade to go 1.21, change invocations of this to +// sync.OnceValue +func onceValue[T any](f func() T) func() T { + var ( + value T + once sync.Once + ) + return func() T { + once.Do(func() { + value = f() + }) + return value + } +} + // We wrap the creation of the otelgrpc interceptors in a sync.Once - this is // because each time this is called, they create a new underlying metric. If // something (e.g tbot) is repeatedly creating new clients and closing them, // then this leads to a memory leak since the underlying metric is not cleaned // up. -var streamClientInterceptorOnce = sync.OnceValue(func() grpc.StreamClientInterceptor { +// See https://github.com/gravitational/teleport/issues/30759 +var otelStreamClientInterceptor = onceValue(func() grpc.StreamClientInterceptor { return otelgrpc.StreamClientInterceptor() }) -var unaryClientInterceptorOnce = sync.OnceValue(func() grpc.UnaryClientInterceptor { +var otelUnaryClientInterceptor = onceValue(func() grpc.UnaryClientInterceptor { return otelgrpc.UnaryClientInterceptor() }) From f7d839e6bd7ddcbf5a73d7faefa1db957eeb4444 Mon Sep 17 00:00:00 2001 From: Noah Date: Mon, 21 Aug 2023 16:12:30 +0100 Subject: [PATCH 3/3] Add link to issue reported in otel-go-contrib --- api/client/client.go | 1 + 1 file changed, 1 insertion(+) diff --git a/api/client/client.go b/api/client/client.go index d480a032e47a5..52c741c4bdb11 100644 --- a/api/client/client.go +++ b/api/client/client.go @@ -443,6 +443,7 @@ func onceValue[T any](f func() T) func() T { // then this leads to a memory leak since the underlying metric is not cleaned // up. // See https://github.com/gravitational/teleport/issues/30759 +// See https://github.com/open-telemetry/opentelemetry-go-contrib/issues/4226 var otelStreamClientInterceptor = onceValue(func() grpc.StreamClientInterceptor { return otelgrpc.StreamClientInterceptor() })