Split log/logtest into a recorder and a logger (#5365)

The current logtest.Recorder implementation is wrong. We have a single
`Recorder`, which acts as both a `LoggerProvider`, and a `Logger`,
making it possible to emit a log entry with the root recorder, which
shouldn't be possible with the API.

This change introduces a new private struct, `logger` that acts as the
recording logger, while `Recorder` becomes only a LoggerProvider and not
a Logger anymore.

Closes #5357.

---------

Co-authored-by: Robert Pająk <[email protected]>

CHANGELOG.md

@@ -37,6 +37,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 - Fix the empty output of `go.opentelemetry.io/otel/log.Value` in `go.opentelemetry.io/otel/exporters/stdout/stdoutlog`. (#5311)
 - Comparison of unordered maps in `go.opentelemetry.io/otel/log.KeyValue` and `go.opentelemetry.io/otel/log.Value`. (#5306)
 - Fix wrong package name of the error message when parsing endpoint URL in `go.opentelemetry.io/otel/exporters/otlp/otlplog/otlploghttp`. (#5371)
+- Split the behavior of `Recorder` in `go.opentelemetry.io/otel/log/logtest` so it behaves as a `LoggerProvider` only. (#5365)
 
 ## [1.26.0/0.48.0/0.2.0-alpha] 2024-04-24
 

log/logtest/recorder.go

@@ -11,10 +11,6 @@ import (
 	"go.opentelemetry.io/otel/log/embedded"
 )
 
-// embeddedLogger is a type alias so the embedded.Logger type doesn't conflict
-// with the Logger method of the Recorder when it is embedded.
-type embeddedLogger = embedded.Logger // nolint:unused  // Used below.
-
 type enabledFn func(context.Context, log.Record) bool
 
 var defaultEnabledFunc = func(context.Context, log.Record) bool {
@@ -57,11 +53,8 @@ func WithEnabledFunc(fn func(context.Context, log.Record) bool) Option {
 func NewRecorder(options ...Option) *Recorder {
 	cfg := newConfig(options)
 
-	sr := &ScopeRecords{}
-
 	return &Recorder{
-		currentScopeRecord: sr,
-		enabledFn:          cfg.enabledFn,
+		enabledFn: cfg.enabledFn,
 	}
 }
 
@@ -82,12 +75,9 @@ type ScopeRecords struct {
 // in-memory.
 type Recorder struct {
 	embedded.LoggerProvider
-	embeddedLogger // nolint:unused  // Used to embed embedded.Logger.
-
-	mu sync.Mutex
 
-	loggers            []*Recorder
-	currentScopeRecord *ScopeRecords
+	mu      sync.Mutex
+	loggers []*logger
 
 	// enabledFn decides whether the recorder should enable logging of a record or not
 	enabledFn enabledFn
@@ -98,41 +88,24 @@ type Recorder struct {
 func (r *Recorder) Logger(name string, opts ...log.LoggerOption) log.Logger {
 	cfg := log.NewLoggerConfig(opts...)
 
-	nr := &Recorder{
-		currentScopeRecord: &ScopeRecords{
+	nl := &logger{
+		scopeRecord: &ScopeRecords{
 			Name:      name,
 			Version:   cfg.InstrumentationVersion(),
 			SchemaURL: cfg.SchemaURL(),
 		},
 		enabledFn: r.enabledFn,
 	}
-	r.addChildLogger(nr)
-
-	return nr
-}
-
-func (r *Recorder) addChildLogger(nr *Recorder) {
-	r.mu.Lock()
-	defer r.mu.Unlock()
-
-	r.loggers = append(r.loggers, nr)
-}
-
-// Enabled indicates whether a specific record should be stored.
-func (r *Recorder) Enabled(ctx context.Context, record log.Record) bool {
-	if r.enabledFn == nil {
-		return defaultEnabledFunc(ctx, record)
-	}
+	r.addChildLogger(nl)
 
-	return r.enabledFn(ctx, record)
+	return nl
 }
 
-// Emit stores the log record.
-func (r *Recorder) Emit(_ context.Context, record log.Record) {
+func (r *Recorder) addChildLogger(nl *logger) {
 	r.mu.Lock()
 	defer r.mu.Unlock()
 
-	r.currentScopeRecord.Records = append(r.currentScopeRecord.Records, record)
+	r.loggers = append(r.loggers, nl)
 }
 
 // Result returns the current in-memory recorder log records.
@@ -141,22 +114,53 @@ func (r *Recorder) Result() []*ScopeRecords {
 	defer r.mu.Unlock()
 
 	ret := []*ScopeRecords{}
-	ret = append(ret, r.currentScopeRecord)
 	for _, l := range r.loggers {
-		ret = append(ret, l.Result()...)
+		ret = append(ret, l.scopeRecord)
 	}
 	return ret
 }
 
-// Reset clears the in-memory log records.
+// Reset clears the in-memory log records for all loggers.
 func (r *Recorder) Reset() {
 	r.mu.Lock()
 	defer r.mu.Unlock()
 
-	if r.currentScopeRecord != nil {
-		r.currentScopeRecord.Records = nil
-	}
 	for _, l := range r.loggers {
 		l.Reset()
 	}
 }
+
+type logger struct {
+	embedded.Logger
+
+	mu          sync.Mutex
+	scopeRecord *ScopeRecords
+
+	// enabledFn decides whether the recorder should enable logging of a record or not.
+	enabledFn enabledFn
+}
+
+// Enabled indicates whether a specific record should be stored.
+func (l *logger) Enabled(ctx context.Context, record log.Record) bool {
+	if l.enabledFn == nil {
+		return defaultEnabledFunc(ctx, record)
+	}
+
+	return l.enabledFn(ctx, record)
+}
+
+// Emit stores the log record.
+func (l *logger) Emit(_ context.Context, record log.Record) {
+	l.mu.Lock()
+	defer l.mu.Unlock()
+
+	l.scopeRecord.Records = append(l.scopeRecord.Records, record)
+}
+
+// Reset clears the in-memory log records.
+func (l *logger) Reset() {
+	l.mu.Lock()
+	defer l.mu.Unlock()
+
+	l.scopeRecord.Records = nil
+}

log/logtest/recorder_test.go

@@ -26,8 +26,8 @@ func TestRecorderLogger(t *testing.T) {
 		{
 			name: "provides a default logger",
 
-			wantLogger: &Recorder{
-				currentScopeRecord: &ScopeRecords{},
+			wantLogger: &logger{
+				scopeRecord: &ScopeRecords{},
 			},
 		},
 		{
@@ -39,8 +39,8 @@ func TestRecorderLogger(t *testing.T) {
 				log.WithSchemaURL("https://example.com"),
 			},
 
-			wantLogger: &Recorder{
-				currentScopeRecord: &ScopeRecords{
+			wantLogger: &logger{
+				scopeRecord: &ScopeRecords{
 					Name:      "test",
 					Version:   "logtest v42",
 					SchemaURL: "https://example.com",
@@ -51,7 +51,7 @@ func TestRecorderLogger(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			l := NewRecorder(tt.options...).Logger(tt.loggerName, tt.loggerOptions...)
 			// unset enabledFn to allow comparison
-			l.(*Recorder).enabledFn = nil
+			l.(*logger).enabledFn = nil
 
 			assert.Equal(t, tt.wantLogger, l)
 		})
@@ -63,7 +63,7 @@ func TestRecorderLoggerCreatesNewStruct(t *testing.T) {
 	assert.NotEqual(t, r, r.Logger("test"))
 }
 
-func TestRecorderEnabled(t *testing.T) {
+func TestLoggerEnabled(t *testing.T) {
 	for _, tt := range []struct {
 		name        string
 		options     []Option
@@ -97,32 +97,33 @@ func TestRecorderEnabled(t *testing.T) {
 		},
 	} {
 		t.Run(tt.name, func(t *testing.T) {
-			e := NewRecorder(tt.options...).Enabled(tt.ctx, tt.buildRecord())
+			e := NewRecorder(tt.options...).Logger("test").Enabled(tt.ctx, tt.buildRecord())
 			assert.Equal(t, tt.isEnabled, e)
 		})
 	}
 }
 
-func TestRecorderEnabledFnUnset(t *testing.T) {
-	r := &Recorder{}
+func TestLoggerEnabledFnUnset(t *testing.T) {
+	r := &logger{}
 	assert.True(t, r.Enabled(context.Background(), log.Record{}))
 }
 
 func TestRecorderEmitAndReset(t *testing.T) {
 	r := NewRecorder()
+	l := r.Logger("test")
 	assert.Len(t, r.Result()[0].Records, 0)
 
 	r1 := log.Record{}
 	r1.SetSeverity(log.SeverityInfo)
-	r.Emit(context.Background(), r1)
+	l.Emit(context.Background(), r1)
 	assert.Equal(t, r.Result()[0].Records, []log.Record{r1})
 
-	l := r.Logger("test")
+	nl := r.Logger("test")
 	assert.Empty(t, r.Result()[1].Records)
 
 	r2 := log.Record{}
 	r2.SetSeverity(log.SeverityError)
-	l.Emit(context.Background(), r2)
+	nl.Emit(context.Background(), r2)
 	assert.Equal(t, r.Result()[0].Records, []log.Record{r1})
 	assert.Equal(t, r.Result()[1].Records, []log.Record{r2})