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

log: Introduce EnabledParameters #5791

Merged
merged 19 commits into from
Sep 13, 2024
Merged
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
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
### Changed

- Enable exemplars by default in `go.opentelemetry.io/otel/sdk/metric`. Exemplars can be disabled by setting `OTEL_METRICS_EXEMPLAR_FILTER=always_off` (#5778)
- `Logger.Enabled` in `go.opentelemetry.io/otel/log` now accepts a newly introduced `EnabledParameters` type instead of `Record`. (#5791)
- `FilterProcessor.Enabled` in `go.opentelemetry.io/otel/sdk/log/internal/x` now accepts `EnabledParameters` instead of `Record`. (#5791)

<!-- Released section -->
<!-- Don't change this section unless doing release -->
Expand Down
1 change: 0 additions & 1 deletion example/dice/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ module go.opentelemetry.io/otel/example/dice
go 1.22

require (
go.opentelemetry.io/contrib/bridges/otelslog v0.5.0
go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp v0.55.0
go.opentelemetry.io/otel v1.30.0
go.opentelemetry.io/otel/exporters/stdout/stdoutlog v0.6.0
Expand Down
2 changes: 0 additions & 2 deletions example/dice/go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,6 @@ github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZb
github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4=
github.com/stretchr/testify v1.9.0 h1:HtqpIVDClZ4nwg75+f6Lvsy/wHu+3BoSGCbBAcpTsTg=
github.com/stretchr/testify v1.9.0/go.mod h1:r2ic/lqez/lEtzL7wO/rwa5dbSLXVDPFyf8C91i36aY=
go.opentelemetry.io/contrib/bridges/otelslog v0.5.0 h1:lU3F57OSLK5mQ1PDBVAfDDaKCPv37MrEbCfTzsF4bz0=
go.opentelemetry.io/contrib/bridges/otelslog v0.5.0/go.mod h1:I84u06zJFr8T5D73fslEUbnRBimVVSBhuVw8L8I92AU=
go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp v0.55.0 h1:ZIg3ZT/aQ7AfKqdwp7ECpOK6vHqquXXuyTjIO8ZdmPs=
go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp v0.55.0/go.mod h1:DQAwmETtZV00skUwgD6+0U89g80NKsJE3DCKeLLPQMI=
golang.org/x/sys v0.25.0 h1:r+8e+loiHxRqhXVl6ML1nO3l1+oFoWbnlu2Ehimmi34=
Expand Down
7 changes: 5 additions & 2 deletions example/dice/rolldice.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,14 @@ package main
import (
"fmt"
"io"
"log/slog"
"math/rand"
"net/http"
"os"
"strconv"

"go.opentelemetry.io/contrib/bridges/otelslog"
// TODO: https://github.com/open-telemetry/opentelemetry-go/issues/5801
// "go.opentelemetry.io/contrib/bridges/otelslog".
"go.opentelemetry.io/otel"
"go.opentelemetry.io/otel/attribute"
"go.opentelemetry.io/otel/metric"
Expand All @@ -21,7 +24,7 @@ const name = "go.opentelemetry.io/otel/example/dice"
var (
tracer = otel.Tracer(name)
meter = otel.Meter(name)
logger = otelslog.NewLogger(name)
logger = slog.New(slog.NewJSONHandler(os.Stdout, nil)) // TODO: logger = otelslog.NewLogger(name).
MrAlias marked this conversation as resolved.
Show resolved Hide resolved
rollCnt metric.Int64Counter
)

Expand Down
17 changes: 17 additions & 0 deletions log/DESIGN.md
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,23 @@ Rejected alternatives:
- [Add XYZ method to Logger](#add-xyz-method-to-logger)
- [Rename KeyValue to Attr](#rename-keyvalue-to-attr)

### Logger.Enabled

The `Enabled` method implements the [`Enabled` operation](https://opentelemetry.io/docs/specs/otel/logs/bridge-api/#enabled).

[`Context` associated with the `LogRecord`](https://opentelemetry.io/docs/specs/otel/context/)
is accepted as a `context.Context` method argument.

Calls to `Enabled` are supposed to be on the hot path and the list of arguments
can be extendend in future. Therefore, in order to reduce the number of heap
allocations and make it possible to handle new arguments, `Enabled` accepts
a `EnabledParameters` struct, defined in [logger.go](logger.go), as the second
method argument.

The `EnabledParameters` getters are returning values using the `(value, ok)`
idiom in order to indicate if the values were actually set by the caller or if
there are unspecified.

### noop package

The `go.opentelemetry.io/otel/log/noop` package provides
Expand Down
4 changes: 2 additions & 2 deletions log/internal/global/log.go
Original file line number Diff line number Diff line change
Expand Up @@ -87,10 +87,10 @@ func (l *logger) Emit(ctx context.Context, r log.Record) {
}
}

func (l *logger) Enabled(ctx context.Context, r log.Record) bool {
func (l *logger) Enabled(ctx context.Context, param log.EnabledParameters) bool {
var enabled bool
if del, ok := l.delegate.Load().(log.Logger); ok {
enabled = del.Enabled(ctx, r)
enabled = del.Enabled(ctx, param)
}
return enabled
}
Expand Down
8 changes: 5 additions & 3 deletions log/internal/global/log_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,11 +51,12 @@ func TestLoggerConcurrentSafe(t *testing.T) {

ctx := context.Background()
var r log.Record
var param log.EnabledParameters

var enabled bool
for {
l.Emit(ctx, r)
enabled = l.Enabled(ctx, r)
enabled = l.Enabled(ctx, param)

select {
case <-stop:
Expand Down Expand Up @@ -103,16 +104,17 @@ 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) Enabled(context.Context, log.EnabledParameters) bool {
l.enabledN++
return true
}

func emitRecord(l log.Logger) {
ctx := context.Background()
var param log.EnabledParameters
var r log.Record

_ = l.Enabled(ctx, r)
_ = l.Enabled(ctx, param)
l.Emit(ctx, r)
}

Expand Down
30 changes: 24 additions & 6 deletions log/logger.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,26 +31,26 @@ type Logger interface {
Emit(ctx context.Context, record Record)

// Enabled returns whether the Logger emits for the given context and
// record.
// param.
//
// The passed record is likely to be a partial record with only the
// bridge-relevant information being provided (e.g a record with only the
// The passed param is likely to be a partial record with only the
// bridge-relevant information being provided (e.g a param with only the
// Severity set). If a Logger needs more information than is provided, it
// is said to be in an indeterminate state (see below).
//
// The returned value will be true when the Logger will emit for the
// provided context and record, and will be false if the Logger will not
// provided context and param, and will be false if the Logger will not
// emit. The returned value may be true or false in an indeterminate state.
// An implementation should default to returning true for an indeterminate
// state, but may return false if valid reasons in particular circumstances
// exist (e.g. performance, correctness).
//
// The record should not be held by the implementation. A copy should be
// The param should not be held by the implementation. A copy should be
// made if the record needs to be held after the call returns.
//
// Implementations of this method need to be safe for a user to call
// concurrently.
Enabled(ctx context.Context, record Record) bool
Enabled(ctx context.Context, param EnabledParameters) bool
}

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

// EnabledParameters represents payload for [Logger]'s Enabled method.
type EnabledParameters struct {
severity Severity
severitySet bool
}

// Severity returns the [Severity] level value, or [SeverityUndefined] if no value was set.
// The ok result indicates whether the value was set.
func (r *EnabledParameters) Severity() (value Severity, ok bool) {
return r.severity, r.severitySet
}

// SetSeverity sets the [Severity] level.
func (r *EnabledParameters) SetSeverity(level Severity) {
r.severity = level
r.severitySet = true
}
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.EnabledParameters) bool

var defaultEnabledFunc = func(context.Context, log.Record) bool {
var defaultEnabledFunc = func(context.Context, log.EnabledParameters) 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.EnabledParameters) 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) Enabled(ctx context.Context, opts log.EnabledParameters) 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
24 changes: 12 additions & 12 deletions log/logtest/recorder_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,47 +65,47 @@ 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
buildEnabledParameters func() log.EnabledParameters

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

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

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").Enabled(tt.ctx, tt.buildEnabledParameters())
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.Enabled(context.Background(), log.EnabledParameters{}))
}

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

nr := r.Logger("test")
nr.Enabled(context.Background(), log.Record{})
nr.Enabled(context.Background(), log.EnabledParameters{})
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) Enabled(context.Context, log.EnabledParameters) bool { return false }
11 changes: 3 additions & 8 deletions sdk/log/example_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ type ContextFilterProcessor struct {
}

type filter interface {
Enabled(ctx context.Context, record log.Record) bool
Enabled(ctx context.Context, param logapi.EnabledParameters) bool
}

func (p *ContextFilterProcessor) OnEmit(ctx context.Context, record *log.Record) error {
Expand All @@ -100,13 +100,13 @@ func (p *ContextFilterProcessor) OnEmit(ctx context.Context, record *log.Record)
return p.Processor.OnEmit(ctx, record)
}

func (p *ContextFilterProcessor) Enabled(ctx context.Context, record log.Record) bool {
func (p *ContextFilterProcessor) Enabled(ctx context.Context, param logapi.EnabledParameters) bool {
p.lazyFilter.Do(func() {
if f, ok := p.Processor.(filter); ok {
p.filter = f
}
})
return !ignoreLogs(ctx) && (p.filter == nil || p.filter.Enabled(ctx, record))
return !ignoreLogs(ctx) && (p.filter == nil || p.filter.Enabled(ctx, param))
}

func ignoreLogs(ctx context.Context) bool {
Expand Down Expand Up @@ -147,11 +147,6 @@ func (p *RedactTokensProcessor) OnEmit(ctx context.Context, record *log.Record)
return nil
}

// Enabled returns true.
func (p *RedactTokensProcessor) Enabled(context.Context, log.Record) bool {
return true
}

// Shutdown returns nil.
func (p *RedactTokensProcessor) Shutdown(ctx context.Context) error {
return nil
Expand Down
19 changes: 10 additions & 9 deletions sdk/log/internal/x/x.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,37 +10,38 @@ import (
"go.opentelemetry.io/otel/log"
)

// FilterProcessor is a [Processor] that knows, and can identify, what
// [log.Record] it will process or drop when it is passed to OnEmit.
// FilterProcessor is a [go.opentelemetry.io/otel/sdk/log.Processor] that knows,
// and can identify, what [log.Record] it will process or drop when it is
// passed to OnEmit.
//
// This is useful for users of logging libraries that want to know if a [log.Record]
// will be processed or dropped before they perform complex operations to
// construct the [log.Record].
//
// [Processor] implementations that choose to support this by satisfying this
// Processor implementations that choose to support this by satisfying this
// interface are expected to re-evaluate the [log.Record]s passed to OnEmit, it is
// not expected that the caller to OnEmit will use the functionality from this
// interface prior to calling OnEmit.
//
// This should only be implemented for [Processor]s that can make reliable
// This should only be implemented for Processors that can make reliable
// enough determination of this prior to processing a [log.Record] and where
// the result is dynamic.
//
// [Processor]: https://pkg.go.dev/go.opentelemetry.io/otel/sdk/log#Processor
type FilterProcessor interface {
// Enabled returns whether the Processor will process for the given context
// and record.
// and param.
//
// The passed record is likely to be a partial record with only the
// The passed param is likely to be a partial record with only the
// bridge-relevant information being provided (e.g a record with only the
// Severity set). If a Logger needs more information than is provided, it
// is said to be in an indeterminate state (see below).
//
// The returned value will be true when the Processor will process for the
// provided context and record, and will be false if the Processor will not
// provided context and param, and will be false if the Processor will not
// process. An implementation should default to returning true for an
// indeterminate state.
//
// Implementations should not modify the record.
Enabled(ctx context.Context, record log.Record) bool
// Implementations should not modify the param.
Enabled(ctx context.Context, param log.EnabledParameters) bool
pellared marked this conversation as resolved.
Show resolved Hide resolved
}
Loading
Loading