Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert Logger.IsEnabled to not require Record #5770

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions log/internal/global/log.go
Original file line number Diff line number Diff line change
Expand Up @@ -87,12 +87,11 @@ func (l *logger) Emit(ctx context.Context, r log.Record) {
}
}

func (l *logger) Enabled(ctx context.Context, r log.Record) bool {
var enabled bool
func (l *logger) IsEnabled(ctx context.Context, opts ...log.LoggerEnabledOption) bool {
if del, ok := l.delegate.Load().(log.Logger); ok {
enabled = del.Enabled(ctx, r)
return del.IsEnabled(ctx, opts...)
}
return enabled
return false
}

func (l *logger) setDelegate(provider log.LoggerProvider) {
Expand Down
14 changes: 7 additions & 7 deletions log/internal/global/log_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ func TestLoggerConcurrentSafe(t *testing.T) {
var enabled bool
for {
l.Emit(ctx, r)
enabled = l.Enabled(ctx, r)
enabled = l.IsEnabled(ctx)

select {
case <-stop:
Expand Down Expand Up @@ -103,16 +103,16 @@ type testLogger struct {
}

func (l *testLogger) Emit(context.Context, log.Record) { l.emitN++ }
func (l *testLogger) Enabled(context.Context, log.Record) bool {
func (l *testLogger) IsEnabled(context.Context, ...log.LoggerEnabledOption) bool {
l.enabledN++
return true
}

func emitRecord(l log.Logger) {
func emitRecord(t testing.TB, l log.Logger) {
ctx := context.Background()
var r log.Record

_ = l.Enabled(ctx, r)
assert.True(t, l.IsEnabled(ctx))
l.Emit(ctx, r)
}

Expand Down Expand Up @@ -143,15 +143,15 @@ func TestDelegation(t *testing.T) {
want++
assert.Equal(t, want, delegate.loggerN, "new Logger not delegated")

emitRecord(pre0)
emitRecord(pre2)
emitRecord(t, pre0)
emitRecord(t, pre2)

if assert.IsType(t, &testLogger{}, pre2, "wrong pre-delegation Logger type") {
assert.Equal(t, 2, pre2.(*testLogger).emitN, "Emit not delegated")
assert.Equal(t, 2, pre2.(*testLogger).enabledN, "Enabled not delegated")
}

emitRecord(post)
emitRecord(t, post)

if assert.IsType(t, &testLogger{}, post, "wrong post-delegation Logger type") {
assert.Equal(t, 1, post.(*testLogger).emitN, "Emit not delegated")
Expand Down
10 changes: 8 additions & 2 deletions log/logger.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ type Logger interface {
// concurrently.
Emit(ctx context.Context, record Record)

// Enabled returns whether the Logger emits for the given context and
// IsEnabled returns whether the Logger emits for the given context and
// record.
//
// The passed record is likely to be a partial record with only the
Expand All @@ -50,7 +50,7 @@ type Logger interface {
//
// Implementations of this method need to be safe for a user to call
// concurrently.
Enabled(ctx context.Context, record Record) bool
IsEnabled(ctx context.Context, opts ...LoggerEnabledOption) bool
}

// LoggerOption applies configuration options to a [Logger].
Expand Down Expand Up @@ -129,3 +129,9 @@ func WithSchemaURL(schemaURL string) LoggerOption {
return config
})
}

// LoggerEnabledOption applies configuration options to a [Logger.IsEnabled].
type LoggerEnabledOption interface {
// Define EnabledConfig when needed.
applyLogger()
}
12 changes: 6 additions & 6 deletions log/logtest/recorder.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,9 @@ import (
"go.opentelemetry.io/otel/log/embedded"
)

type enabledFn func(context.Context, log.Record) bool
type enabledFn func(context.Context, []log.LoggerEnabledOption) bool

var defaultEnabledFunc = func(context.Context, log.Record) bool {
var defaultEnabledFunc = func(context.Context, []log.LoggerEnabledOption) bool {
return true
}

Expand Down Expand Up @@ -42,7 +42,7 @@ func (f optFunc) apply(c config) config { return f(c) }
// WithEnabledFunc allows configuring whether the [Recorder] is enabled for specific log entries or not.
//
// By default, the Recorder is enabled for every log entry.
func WithEnabledFunc(fn func(context.Context, log.Record) bool) Option {
func WithEnabledFunc(fn func(context.Context, []log.LoggerEnabledOption) bool) Option {
return optFunc(func(c config) config {
c.enabledFn = fn
return c
Expand Down Expand Up @@ -155,12 +155,12 @@ type logger struct {
}

// Enabled indicates whether a specific record should be stored.
func (l *logger) Enabled(ctx context.Context, record log.Record) bool {
func (l *logger) IsEnabled(ctx context.Context, opts ...log.LoggerEnabledOption) bool {
if l.enabledFn == nil {
return defaultEnabledFunc(ctx, record)
return defaultEnabledFunc(ctx, opts)
}

return l.enabledFn(ctx, record)
return l.enabledFn(ctx, opts)
}

// Emit stores the log record.
Expand Down
21 changes: 7 additions & 14 deletions log/logtest/recorder_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,47 +65,40 @@ func TestRecorderLoggerCreatesNewStruct(t *testing.T) {

func TestLoggerEnabled(t *testing.T) {
for _, tt := range []struct {
name string
options []Option
ctx context.Context
buildRecord func() log.Record
name string
options []Option
ctx context.Context

isEnabled bool
}{
{
name: "the default option enables every log entry",
ctx: context.Background(),
buildRecord: func() log.Record {
return log.Record{}
},

isEnabled: true,
},
{
name: "with everything disabled",
options: []Option{
WithEnabledFunc(func(context.Context, log.Record) bool {
WithEnabledFunc(func(context.Context, []log.LoggerEnabledOption) bool {
return false
}),
},
ctx: context.Background(),
buildRecord: func() log.Record {
return log.Record{}
},

isEnabled: false,
},
} {
t.Run(tt.name, func(t *testing.T) {
e := NewRecorder(tt.options...).Logger("test").Enabled(tt.ctx, tt.buildRecord())
e := NewRecorder(tt.options...).Logger("test").IsEnabled(tt.ctx)
assert.Equal(t, tt.isEnabled, e)
})
}
}

func TestLoggerEnabledFnUnset(t *testing.T) {
r := &logger{}
assert.True(t, r.Enabled(context.Background(), log.Record{}))
assert.True(t, r.IsEnabled(context.Background()))
}

func TestRecorderEmitAndReset(t *testing.T) {
Expand Down Expand Up @@ -158,7 +151,7 @@ func TestRecorderConcurrentSafe(t *testing.T) {
defer wg.Done()

nr := r.Logger("test")
nr.Enabled(context.Background(), log.Record{})
nr.IsEnabled(context.Background())
nr.Emit(context.Background(), log.Record{})

r.Result()
Expand Down
2 changes: 1 addition & 1 deletion log/noop/noop.go
Original file line number Diff line number Diff line change
Expand Up @@ -47,4 +47,4 @@ type Logger struct{ embedded.Logger }
func (Logger) Emit(context.Context, log.Record) {}

// Enabled returns false. No log records are ever emitted.
func (Logger) Enabled(context.Context, log.Record) bool { return false }
func (Logger) IsEnabled(context.Context, ...log.LoggerEnabledOption) bool { return false }
16 changes: 3 additions & 13 deletions sdk/log/logger.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,20 +43,10 @@ func (l *logger) Emit(ctx context.Context, r log.Record) {
}
}

// Enabled returns true if at least one Processor held by the LoggerProvider
// IsEnabled returns true if at least one Processor held by the LoggerProvider
// that created the logger will process the record for the provided context.
//
// If it is not possible to definitively determine the record will be
// processed, true will be returned by default. A value of false will only be
// returned if it can be positively verified that no Processor will process the
// record.
func (l *logger) Enabled(ctx context.Context, record log.Record) bool {
fltrs := l.provider.filterProcessors()
// If there are more Processors than FilterProcessors we cannot be sure
// that all Processors will drop the record. Therefore, return true.
//
// If all Processors are FilterProcessors, check if any is enabled.
return len(l.provider.processors) > len(fltrs) || anyEnabled(ctx, record, fltrs)
func (l *logger) IsEnabled(context.Context, ...log.LoggerEnabledOption) bool {
return len(l.provider.processors) > 0
}

func anyEnabled(ctx context.Context, r log.Record, fltrs []x.FilterProcessor) bool {
Expand Down
33 changes: 5 additions & 28 deletions sdk/log/logger_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -215,9 +215,7 @@ func TestLoggerEmit(t *testing.T) {
}

func TestLoggerEnabled(t *testing.T) {
p0 := newFltrProcessor("0", true)
p1 := newFltrProcessor("1", true)
p2WithDisabled := newFltrProcessor("2", false)
p0 := newProcessor("0")

testCases := []struct {
name string
Expand All @@ -235,24 +233,6 @@ func TestLoggerEnabled(t *testing.T) {
name: "WithProcessors",
logger: newLogger(NewLoggerProvider(
WithProcessor(p0),
WithProcessor(p1),
), instrumentation.Scope{}),
ctx: context.Background(),
expected: true,
},
{
name: "WithDisabledProcessors",
logger: newLogger(NewLoggerProvider(
WithProcessor(p2WithDisabled),
), instrumentation.Scope{}),
ctx: context.Background(),
expected: false,
},
{
name: "ContainsDisabledProcessor",
logger: newLogger(NewLoggerProvider(
WithProcessor(p2WithDisabled),
WithProcessor(p0),
), instrumentation.Scope{}),
ctx: context.Background(),
expected: true,
Expand All @@ -261,7 +241,6 @@ func TestLoggerEnabled(t *testing.T) {
name: "WithNilContext",
logger: newLogger(NewLoggerProvider(
WithProcessor(p0),
WithProcessor(p1),
), instrumentation.Scope{}),
ctx: nil,
expected: true,
Expand All @@ -270,27 +249,25 @@ func TestLoggerEnabled(t *testing.T) {

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
assert.Equal(t, tc.expected, tc.logger.Enabled(tc.ctx, log.Record{}))
assert.Equal(t, tc.expected, tc.logger.IsEnabled(tc.ctx))
})
}
}

func BenchmarkLoggerEnabled(b *testing.B) {
provider := NewLoggerProvider(
WithProcessor(newFltrProcessor("0", false)),
WithProcessor(newFltrProcessor("1", true)),
WithProcessor(newProcessor("0")),
)
logger := provider.Logger("BenchmarkLoggerEnabled")
ctx, r := context.Background(), log.Record{}
r.SetSeverityText("test")
ctx := context.Background()

var enabled bool

b.ReportAllocs()
b.ResetTimer()

for n := 0; n < b.N; n++ {
enabled = logger.Enabled(ctx, r)
enabled = logger.IsEnabled(ctx)
}

_ = enabled
Expand Down
22 changes: 1 addition & 21 deletions sdk/log/provider_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ import (
"go.opentelemetry.io/otel/internal/global"
"go.opentelemetry.io/otel/log"
"go.opentelemetry.io/otel/log/noop"
"go.opentelemetry.io/otel/sdk/log/internal/x"
"go.opentelemetry.io/otel/sdk/resource"
)

Expand Down Expand Up @@ -57,25 +56,6 @@ func (p *processor) ForceFlush(context.Context) error {
return p.Err
}

type fltrProcessor struct {
*processor

enabled bool
}

var _ x.FilterProcessor = (*fltrProcessor)(nil)

func newFltrProcessor(name string, enabled bool) *fltrProcessor {
return &fltrProcessor{
processor: newProcessor(name),
enabled: enabled,
}
}

func (p *fltrProcessor) Enabled(context.Context, log.Record) bool {
return p.enabled
}

func TestNewLoggerProviderConfiguration(t *testing.T) {
t.Cleanup(func(orig otel.ErrorHandler) func() {
otel.SetErrorHandler(otel.ErrorHandlerFunc(func(err error) {
Expand Down Expand Up @@ -321,5 +301,5 @@ func BenchmarkLoggerProviderLogger(b *testing.B) {
}

b.StopTimer()
loggers[0].Enabled(context.Background(), log.Record{})
loggers[0].IsEnabled(context.Background())
}
Loading