Only allocate named logger if necessary in activator (#11008)

cmd/activator/main.go

@@ -175,7 +175,7 @@ func main() {
 
 	// Create activation handler chain
 	// Note: innermost handlers are specified first, ie. the last handler in the chain will be executed first
-	var ah http.Handler = activatorhandler.New(ctx, throttler, transport)
+	var ah http.Handler = activatorhandler.New(ctx, throttler, transport, logger)
 	ah = concurrencyReporter.Handler(ah)
 	ah = tracing.HTTPSpanMiddleware(ah)
 	ah = configStore.HTTPMiddleware(ah)

pkg/activator/handler/context_handler.go

@@ -65,17 +65,15 @@ func (h *contextHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	}
 
 	revID := types.NamespacedName{Namespace: namespace, Name: name}
-	logger := h.logger.With(zap.String(logkey.Key, revID.String()))
 
 	revision, err := h.revisionLister.Revisions(namespace).Get(name)
 	if err != nil {
-		logger.Errorw("Error while getting revision", zap.Error(err))
+		h.logger.Errorw("Error while getting revision", zap.String(logkey.Key, revID.String()), zap.Error(err))
 		sendError(err, w)
 		return
 	}
 
 	ctx := r.Context()
-	ctx = logging.WithLogger(ctx, logger)
 	ctx = WithRevision(ctx, revision)
 	ctx = WithRevID(ctx, revID)
 

pkg/activator/handler/handler.go

@@ -28,7 +28,7 @@ import (
 	"k8s.io/apimachinery/pkg/types"
 
 	network "knative.dev/networking/pkg"
-	"knative.dev/pkg/logging"
+	"knative.dev/pkg/logging/logkey"
 	pkgnet "knative.dev/pkg/network"
 	tracingconfig "knative.dev/pkg/tracing/config"
 	"knative.dev/pkg/tracing/propagation/tracecontextb3"
@@ -50,10 +50,11 @@ type activationHandler struct {
 	tracingTransport http.RoundTripper
 	throttler        Throttler
 	bufferPool       httputil.BufferPool
+	logger           *zap.SugaredLogger
 }
 
 // New constructs a new http.Handler that deals with revision activation.
-func New(_ context.Context, t Throttler, transport http.RoundTripper) http.Handler {
+func New(_ context.Context, t Throttler, transport http.RoundTripper, logger *zap.SugaredLogger) http.Handler {
 	return &activationHandler{
 		transport: transport,
 		tracingTransport: &ochttp.Transport{
@@ -62,26 +63,27 @@ func New(_ context.Context, t Throttler, transport http.RoundTripper) http.Handl
 		},
 		throttler:  t,
 		bufferPool: network.NewBufferPool(),
+		logger:     logger,
 	}
 }
 
 func (a *activationHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
-	logger := logging.FromContext(r.Context())
 	tracingEnabled := activatorconfig.FromContext(r.Context()).Tracing.Backend != tracingconfig.None
 
 	tryContext, trySpan := r.Context(), (*trace.Span)(nil)
 	if tracingEnabled {
 		tryContext, trySpan = trace.StartSpan(r.Context(), "throttler_try")
 	}
 
-	if err := a.throttler.Try(tryContext, RevIDFrom(r.Context()), func(dest string) error {
+	revID := RevIDFrom(r.Context())
+	if err := a.throttler.Try(tryContext, revID, func(dest string) error {
 		trySpan.End()
 
 		proxyCtx, proxySpan := r.Context(), (*trace.Span)(nil)
 		if tracingEnabled {
 			proxyCtx, proxySpan = trace.StartSpan(r.Context(), "activator_proxy")
 		}
-		a.proxyRequest(logger, w, r.WithContext(proxyCtx), dest, tracingEnabled)
+		a.proxyRequest(revID, w, r.WithContext(proxyCtx), dest, tracingEnabled)
 		proxySpan.End()
 
 		return nil
@@ -90,7 +92,7 @@ func (a *activationHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 		trySpan.Annotate([]trace.Attribute{trace.StringAttribute("activator.throttler.error", err.Error())}, "ThrottlerTry")
 		trySpan.End()
 
-		logger.Errorw("Throttler try error", zap.Error(err))
+		a.logger.Errorw("Throttler try error", zap.String(logkey.Key, revID.String()), zap.Error(err))
 
 		if errors.Is(err, context.DeadlineExceeded) || errors.Is(err, queue.ErrRequestQueueFull) {
 			http.Error(w, err.Error(), http.StatusServiceUnavailable)
@@ -100,7 +102,7 @@ func (a *activationHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	}
 }
 
-func (a *activationHandler) proxyRequest(logger *zap.SugaredLogger, w http.ResponseWriter, r *http.Request, target string, tracingEnabled bool) {
+func (a *activationHandler) proxyRequest(revID types.NamespacedName, w http.ResponseWriter, r *http.Request, target string, tracingEnabled bool) {
 	network.RewriteHostIn(r)
 	r.Header.Set(network.ProxyHeaderName, activator.Name)
 
@@ -112,7 +114,9 @@ func (a *activationHandler) proxyRequest(logger *zap.SugaredLogger, w http.Respo
 		proxy.Transport = a.tracingTransport
 	}
 	proxy.FlushInterval = network.FlushInterval
-	proxy.ErrorHandler = pkgnet.ErrorHandler(logger)
+	proxy.ErrorHandler = func(w http.ResponseWriter, req *http.Request, err error) {
+		pkgnet.ErrorHandler(a.logger.With(zap.String(logkey.Key, revID.String())))(w, req, err)
+	}
 
 	proxy.ServeHTTP(w, r)
 }

pkg/activator/handler/handler_test.go

@@ -125,7 +125,7 @@ func TestActivationHandler(t *testing.T) {
 
 			ctx, cancel, _ := rtesting.SetupFakeContextWithCancel(t)
 			defer cancel()
-			handler := New(ctx, test.throttler, rt)
+			handler := New(ctx, test.throttler, rt, logging.FromContext(ctx))
 
 			resp := httptest.NewRecorder()
 			req := httptest.NewRequest(http.MethodPost, "http://example.com", nil)
@@ -164,7 +164,7 @@ func TestActivationHandlerProxyHeader(t *testing.T) {
 	ctx, cancel, _ := rtesting.SetupFakeContextWithCancel(t)
 	defer cancel()
 
-	handler := New(ctx, fakeThrottler{}, rt)
+	handler := New(ctx, fakeThrottler{}, rt, logging.FromContext(ctx))
 
 	writer := httptest.NewRecorder()
 	req := httptest.NewRequest(http.MethodPost, "http://example.com", nil)
@@ -241,7 +241,7 @@ func TestActivationHandlerTraceSpans(t *testing.T) {
 				oct.Finish()
 			}()
 
-			handler := New(ctx, fakeThrottler{}, rt)
+			handler := New(ctx, fakeThrottler{}, rt, logging.FromContext(ctx))
 
 			// Set up config store to populate context.
 			configStore := setupConfigStore(t, logging.FromContext(ctx))
@@ -311,7 +311,7 @@ func BenchmarkHandler(b *testing.B) {
 			}, nil
 		})
 
-		handler := New(ctx, fakeThrottler{}, rt)
+		handler := New(ctx, fakeThrottler{}, rt, logging.FromContext(ctx))
 
 		request := func() *http.Request {
 			req := httptest.NewRequest(http.MethodGet, "http://example.com", nil)