DelegatingLogger seems to be a memory hog

[alvaroaleman]

We have a controllermanager that doesn't use the built-in logging, which means the logger construction in c-r falls back to the default in pkg/log/log.go which is NewDelegatingLogger(NullLogger{}).

Analyzing the memory usage of this controller I noticed that logging seems to have the second highest number of heap allocations:

  584.03MB 17.25% 17.25%  1370.90MB 40.49%  sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).reconcileHandler
  551.55MB 16.29% 33.54%   551.55MB 16.29%  sigs.k8s.io/controller-runtime/pkg/log.(*loggerPromise).WithValues
  542.76MB 16.03% 49.57%   542.76MB 16.03%  k8s.io/apimachinery/pkg/runtime.(*RawExtension).UnmarshalJSON
  399.21MB 11.79% 61.36%   413.97MB 12.23%  github.com/json-iterator/go.(*Iterator).ReadString
  297.66MB  8.79% 70.15%   297.66MB  8.79%  reflect.unsafe_NewArray
  243.51MB  7.19% 77.34%   243.51MB  7.19%  fmt.Sprintf
  189.01MB  5.58% 82.92%   740.56MB 21.87%  sigs.k8s.io/controller-runtime/pkg/log.(*DelegatingLogger).WithValues
  171.58MB  5.07% 87.99%   171.58MB  5.07%  reflect.mapassign
  100.15MB  2.96% 90.94%   100.15MB  2.96%  encoding/base64.(*Encoding).DecodeString
   60.01MB  1.77% 92.72%    60.01MB  1.77%  github.com/modern-go/reflect2.(*unsafeType).UnsafeNew

IMHO we should be a lot more efficient in doing nothing.

/kind bug

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[vincepri]

This might be worth fixing

[vincepri]

/lifecycle frozen

[vincepri]

Reading the code a bit, I think we're expecting the caller to call SetLogger with NullLogger{} if they actually want a null logger. Seems the delegating logger's only purpose is to keep log lines around until SetLogger is called, which might never be called if someone never sets it.

I'm not sure if this is an actual problem or not, but I'm in favor of completely removing the DelegatingLogger and default to a plain NullLogger{} for all logs, or we can also default to klogr if we prefer, which now defaults to stderr as well.