From ebd0adee357f3539d7b19488eef9cf7bcd8aa0da Mon Sep 17 00:00:00 2001 From: Damien Mathieu Date: Fri, 17 May 2024 16:28:40 +0200 Subject: [PATCH] Split log/logtest into a recorder and a logger (#5365) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- CHANGELOG.md | 1 + log/logtest/recorder.go | 88 +++++++++++++++++++----------------- log/logtest/recorder_test.go | 25 +++++----- 3 files changed, 60 insertions(+), 54 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1d4c5e70a72..d6925232f0d 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/log/logtest/recorder.go b/log/logtest/recorder.go index 62c3eaf607e..5cdb0f290f4 100644 --- a/log/logtest/recorder.go +++ b/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 +} diff --git a/log/logtest/recorder_test.go b/log/logtest/recorder_test.go index eb84ec22c32..33af64cb92b 100644 --- a/log/logtest/recorder_test.go +++ b/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})