🐛 [logs] resolve problem with the logger source being set for a logr.Logger (#479)

<!--
Copyright (C) 2020-2022 Arm Limited or its affiliates and Contributors.
All rights reserved.
SPDX-License-Identifier: Apache-2.0
-->
### Description
- avoid infinite logger creation loop



### Test Coverage

<!--
Please put an `x` in the correct box e.g. `[x]` to indicate the testing
coverage of this change.
-->

- [x]  This change is covered by existing or additional automated tests.
- [ ] Manual testing has been performed (and evidence provided) as
automated testing was not feasible.
- [ ] Additional tests are not required for this change (e.g.
documentation update).

Author: acabarbaye

changes/20240627132421.bugfix

@@ -0,0 +1 @@
+:bug: `[logs]` resolve problem with the logger source being set for a logr.Logger

utils/logs/logr_logger.go

@@ -7,22 +7,29 @@ package logs
 import (
 	"context"
 	"fmt"
+	"strings"
+	"sync"
 
 	"github.com/go-logr/logr"
 	"github.com/go-logr/stdr"
+	"go.uber.org/atomic"
 
+	"github.com/ARM-software/golang-utils/utils/collection"
 	"github.com/ARM-software/golang-utils/utils/commonerrors"
 	"github.com/ARM-software/golang-utils/utils/reflection"
 )
 
 const (
-	KeyLogSource    = "source"
-	KeyLoggerSource = "logger-source"
+	KeyLogSource     = "source"
+	KeyLoggerSource  = "logger-source"
+	keyloggerSources = "logger-sources"
 )
 
 type logrLogger struct {
 	logger    logr.Logger
 	closeFunc func() error
+	source    *atomic.String
+	logSource *atomic.String
 }
 
 func (l *logrLogger) Close() error {
@@ -36,18 +43,28 @@ func (l *logrLogger) Check() error {
 	return nil
 }
 
+// SetLogSource will not overwrite the source if already defined
 func (l *logrLogger) SetLogSource(source string) error {
 	if reflection.IsEmpty(source) {
 		return commonerrors.ErrNoLogSource
 	}
+	if strings.EqualFold(source, l.logSource.Load()) {
+		return nil
+	}
+	l.logSource.Store(source)
 	l.logger = l.logger.WithValues(KeyLogSource, source)
 	return nil
 }
 
+// SetLoggerSource will not overwrite the source if already defined
 func (l *logrLogger) SetLoggerSource(source string) error {
 	if reflection.IsEmpty(source) {
 		return commonerrors.ErrNoLoggerSource
 	}
+	if strings.EqualFold(source, l.source.Load()) {
+		return nil
+	}
+	l.source.Store(source)
 	l.logger = l.logger.WithName(source).WithValues(KeyLoggerSource, source)
 	return nil
 }
@@ -76,7 +93,12 @@ func NewLogrLogger(logrImpl logr.Logger, loggerSource string) (Loggers, error) {
 
 // NewLogrLoggerWithClose creates loggers based on a logr implementation (https://github.com/go-logr/logr)
 func NewLogrLoggerWithClose(logrImpl logr.Logger, loggerSource string, closeFunc func() error) (loggers Loggers, err error) {
-	loggers = &logrLogger{logger: logrImpl, closeFunc: closeFunc}
+	loggers = &logrLogger{
+		logger:    logrImpl,
+		closeFunc: closeFunc,
+		source:    atomic.NewString(""),
+		logSource: atomic.NewString(""),
+	}
 	err = loggers.SetLoggerSource(loggerSource)
 	return
 }
@@ -131,13 +153,13 @@ func (s *plainLoggersSinkAdapter) Error(err error, msg string, keysAndValues ...
 	}
 }
 
-func (s *plainLoggersSinkAdapter) WithValues(keysAndValues ...any) logr.LogSink {
+func (s *plainLoggersSinkAdapter) WithValues(_ ...any) logr.LogSink {
 	return &plainLoggersSinkAdapter{underlying: s.underlying}
 }
 
 func (s *plainLoggersSinkAdapter) WithName(name string) logr.LogSink {
 	if s.underlying != nil {
-		_ = s.underlying.SetLogSource(name)
+		_ = s.underlying.SetLoggerSource(name)
 	}
 	return &plainLoggersSinkAdapter{underlying: s.underlying}
 }
@@ -151,6 +173,7 @@ func NewPlainLoggersSink(logger Loggers) logr.LogSink {
 type loggersLogSinkAdapter struct {
 	stdOut     logr.LogSink
 	stdErr     logr.LogSink
+	values     sync.Map
 	underlying Loggers
 }
 
@@ -172,23 +195,106 @@ func (l *loggersLogSinkAdapter) Error(err error, msg string, keysAndValues ...an
 	l.stdErr.Error(err, msg, keysAndValues...)
 }
 
+// WithValues will not overwrite already defined values.
 func (l *loggersLogSinkAdapter) WithValues(keysAndValues ...any) logr.LogSink {
-	return &loggersLogSinkAdapter{
-		stdOut:     l.stdOut.WithValues(keysAndValues...),
-		stdErr:     l.stdErr.WithValues(keysAndValues...),
+	sink := &loggersLogSinkAdapter{
+		stdOut:     l.stdOut,
+		stdErr:     l.stdErr,
+		values:     sync.Map{},
 		underlying: l.underlying,
 	}
+	l.transferValues(sink)
+	sink.withValues(keysAndValues...)
+	return sink
 }
 
-func (l *loggersLogSinkAdapter) WithName(name string) logr.LogSink {
+func (l *loggersLogSinkAdapter) transferValues(sink *loggersLogSinkAdapter) {
+	if sink == nil {
+		return
+	}
+	l.values.Range(func(k, v any) bool {
+		return sink.recordValue(k, v)
+	})
+}
+
+func (l *loggersLogSinkAdapter) withValues(keysAndValues ...any) {
+	if len(keysAndValues)%2 != 0 {
+		return
+	}
+	for i := 0; i < len(keysAndValues); i += 2 {
+		k := keysAndValues[i]
+		v := keysAndValues[i+1]
+		_, _ = l.values.LoadOrStore(k, v)
+	}
+}
+
+func (l *loggersLogSinkAdapter) recordValue(k, v any) bool {
+	l.values.Store(k, v)
+	return true
+}
+
+func (l *loggersLogSinkAdapter) recordName(name string) {
+	if reflection.IsEmpty(name) {
+		return
+	}
 	if l.underlying != nil {
-		_ = l.underlying.SetLogSource(name)
+		_ = l.underlying.SetLoggerSource(name)
 	}
-	return &loggersLogSinkAdapter{
-		stdOut:     l.stdOut.WithName(name),
-		stdErr:     l.stdErr.WithName(name),
-		underlying: l.underlying,
+	if l.hasName(name) {
+		return
+	}
+	namesAny, ok := l.values.Load(keyloggerSources)
+	var names []string
+	if ok {
+		namesC, ok := namesAny.([]string)
+		if !ok {
+			return
+		}
+		names = namesC
+	}
+	names = append(names, name)
+	_ = l.recordValue(keyloggerSources, names)
+}
+
+func (l *loggersLogSinkAdapter) hasName(name string) (found bool) {
+	if reflection.IsEmpty(name) {
+		return
+	}
+	namesAny, ok := l.values.Load(keyloggerSources)
+	if !ok {
+		return
+	}
+	names, ok := namesAny.([]string)
+	if !ok {
+		return
 	}
+	_, found = collection.FindInSlice(false, names, name)
+	return
+}
+
+func (l *loggersLogSinkAdapter) WithName(name string) logr.LogSink {
+	setName := !l.hasName(name)
+	var sink *loggersLogSinkAdapter
+	if setName {
+		sink = &loggersLogSinkAdapter{
+			stdOut:     l.stdOut.WithName(name),
+			stdErr:     l.stdErr.WithName(name),
+			values:     sync.Map{},
+			underlying: l.underlying,
+		}
+	} else {
+		sink = &loggersLogSinkAdapter{
+			stdOut:     l.stdOut,
+			stdErr:     l.stdErr,
+			values:     sync.Map{},
+			underlying: l.underlying,
+		}
+	}
+
+	l.transferValues(sink)
+	sink.recordName(name)
+
+	return sink
 }
 
 func NewLoggersLogSink(logger Loggers) logr.LogSink {

utils/logs/logr_logger_test.go

@@ -6,6 +6,8 @@ package logs
 
 import (
 	"context"
+	"fmt"
+	"strings"
 	"testing"
 
 	"github.com/go-faker/faker/v4"
@@ -36,6 +38,41 @@ func TestLogrLoggerConversion(t *testing.T) {
 	converted.Info(faker.Sentence(), faker.Word(), faker.Name())
 }
 
+func TestLogrFromLoggersWithMultipleName(t *testing.T) {
+	loggerSource := "src-" + faker.Name()
+	strLogger, err := NewStringLogger(loggerSource)
+	require.NoError(t, err)
+	converted := NewLogrLoggerFromLoggers(strLogger)
+	converted.WithName(loggerSource).WithName(faker.Name()).WithName(faker.Name()).WithName(faker.Name()).WithName(faker.Name()).WithName(faker.Name()).WithName(loggerSource).WithName(faker.Name()).Error(commonerrors.ErrUnexpected, faker.Sentence())
+	assert.Contains(t, strLogger.GetLogContent(), loggerSource)
+	assert.Equal(t, 2, strings.Count(strLogger.GetLogContent(), loggerSource))
+	converted.WithName(loggerSource).Info(faker.Sentence(), faker.Word(), faker.Name())
+	assert.Equal(t, 4, strings.Count(strLogger.GetLogContent(), loggerSource))
+	fmt.Println(strLogger.GetLogContent())
+}
+
+func TestLoggersFromLoggerWithMultipleSource(t *testing.T) {
+	loggerSource := "src-" + faker.Name()
+	strLogger, err := NewStringLogger(loggerSource)
+	require.NoError(t, err)
+	logrl := NewLogrLoggerFromLoggers(strLogger)
+	logger, err := NewLogrLogger(logrl, loggerSource)
+	require.NoError(t, err)
+	require.NoError(t, logger.SetLogSource("logsrc-"+faker.Name()))
+	require.NoError(t, logger.SetLoggerSource(loggerSource))
+	require.NoError(t, logger.SetLoggerSource(loggerSource))
+	require.NoError(t, logger.SetLoggerSource(faker.Name()))
+	require.NoError(t, logger.SetLoggerSource(loggerSource))
+	logger.Log(faker.Sentence())
+	assert.Contains(t, strLogger.GetLogContent(), loggerSource)
+	assert.Equal(t, 2, strings.Count(strLogger.GetLogContent(), loggerSource))
+	require.NoError(t, logger.SetLoggerSource(loggerSource))
+	require.NoError(t, logger.SetLoggerSource(faker.Name()))
+	logger.Log(faker.Sentence())
+	assert.Contains(t, strLogger.GetLogContent(), loggerSource)
+	assert.Equal(t, 4, strings.Count(strLogger.GetLogContent(), loggerSource))
+}
+
 func TestLogrLoggerConversionPlain(t *testing.T) {
 	loggers, err := NewPipeLogger()
 	require.NoError(t, err)