From 1ee7c79b7312257de7e026106cdd5b659f8a35f4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20Paj=C4=85k?= Date: Tue, 18 Feb 2025 22:35:14 +0100 Subject: [PATCH] sdk/log: Add FilterProcessor and EnabledParameters (#6317) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per https://github.com/open-telemetry/opentelemetry-go/pull/6271#issuecomment-2657554647 > We agreed that we can move `FilterProcessor` directly to `sdk/log` as Logs SDK does not look to be stabilized soon. - Add the possibility to filter based on the resource and scope which is available for the SDK. The scope information is the most important as it gives the possibility to e.g. filter out logs emitted for a given logger. Thus e.g. https://github.com/open-telemetry/opentelemetry-specification/issues/4364 is not necessary. See https://github.com/open-telemetry/opentelemetry-specification/pull/4290#discussion_r1927546170 for more context. - It is going be an example for https://github.com/open-telemetry/opentelemetry-specification/issues/4363 There is a little overhead (IMO totally acceptable) because of data transformation. Most importantly, there is no new heap allocation. ``` goos: linux goarch: amd64 pkg: go.opentelemetry.io/otel/sdk/log cpu: 13th Gen Intel(R) Core(TM) i7-13800H │ old.txt │ new.txt │ │ sec/op │ sec/op vs base │ LoggerEnabled-20 4.589n ± 1% 319.750n ± 16% +6867.75% (p=0.000 n=10) │ old.txt │ new.txt │ │ B/op │ B/op vs base │ LoggerEnabled-20 0.000Ki ± 0% 1.093Ki ± 13% ? (p=0.000 n=10) │ old.txt │ new.txt │ │ allocs/op │ allocs/op vs base │ LoggerEnabled-20 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=10) ¹ ¹ all samples are equal ``` `Logger.Enabled` is still more efficient than `Logger.Emit` (benchmarks from https://github.com/open-telemetry/opentelemetry-go/pull/6315). ``` goos: linux goarch: amd64 pkg: go.opentelemetry.io/otel/sdk/log cpu: 13th Gen Intel(R) Core(TM) i7-13800H BenchmarkLoggerEmit/5_attributes-20 559934 2391 ns/op 39088 B/op 1 allocs/op BenchmarkLoggerEmit/10_attributes-20 1000000 5910 ns/op 49483 B/op 5 allocs/op BenchmarkLoggerEnabled-20 1605697 968.7 ns/op 1272 B/op 0 allocs/op PASS ok go.opentelemetry.io/otel/sdk/log 10.789s ``` Prior art: - https://github.com/open-telemetry/opentelemetry-go/pull/6271 - https://github.com/open-telemetry/opentelemetry-go/pull/6286 I also created for tracking purposes: - https://github.com/open-telemetry/opentelemetry-go/issues/6328 --- CHANGELOG.md | 3 ++ log/logger.go | 12 ++++--- sdk/log/doc.go | 3 -- sdk/log/example_test.go | 18 +++++------ sdk/log/filter_processor.go | 62 ++++++++++++++++++++++++++++++++++++ sdk/log/internal/x/README.md | 35 -------------------- sdk/log/internal/x/x.go | 47 --------------------------- sdk/log/logger.go | 17 +++++++--- sdk/log/logger_test.go | 50 ++++++++++++++++++++++------- sdk/log/processor.go | 9 +++--- sdk/log/provider.go | 9 +++--- sdk/log/provider_test.go | 7 ++-- 12 files changed, 145 insertions(+), 127 deletions(-) create mode 100644 sdk/log/filter_processor.go delete mode 100644 sdk/log/internal/x/README.md delete mode 100644 sdk/log/internal/x/x.go diff --git a/CHANGELOG.md b/CHANGELOG.md index f6666085a..5d56b7daa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,6 +31,9 @@ The next release will require at least [Go 1.23]. - Document the pitfalls of using `Resource` as a comparable type. `Resource.Equal` and `Resource.Equivalent` should be used instead. (#6272) - Support [Go 1.24]. (#6304) +- Add `FilterProcessor` and `EnabledParameters` in `go.opentelemetry.io/otel/sdk/log`. + It replaces `go.opentelemetry.io/otel/sdk/log/internal/x.FilterProcessor`. + Compared to previous version it additionally gives the possibility to filter by resource and instrumentation scope. (#6317) ### Changed diff --git a/log/logger.go b/log/logger.go index d7d8831be..1205f08e2 100644 --- a/log/logger.go +++ b/log/logger.go @@ -33,9 +33,13 @@ type Logger interface { // Enabled returns whether the Logger emits for the given context and // param. // - // 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 + // This is useful for users that want to know if a [Record] + // will be processed or dropped before they perform complex operations to + // construct the [Record]. + // + // The passed param is likely to be a partial record 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 @@ -46,7 +50,7 @@ type Logger interface { // exist (e.g. performance, correctness). // // 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. + // made if the param needs to be held after the call returns. // // Implementations of this method need to be safe for a user to call // concurrently. diff --git a/sdk/log/doc.go b/sdk/log/doc.go index 14a581db6..6a1f1b0e9 100644 --- a/sdk/log/doc.go +++ b/sdk/log/doc.go @@ -32,8 +32,5 @@ at a single endpoint their origin is decipherable. See [go.opentelemetry.io/otel/log] for more information about the OpenTelemetry Logs Bridge API. - -See [go.opentelemetry.io/otel/sdk/log/internal/x] for information about the -experimental features. */ package log // import "go.opentelemetry.io/otel/sdk/log" diff --git a/sdk/log/example_test.go b/sdk/log/example_test.go index 8070beef7..5cfd75e4b 100644 --- a/sdk/log/example_test.go +++ b/sdk/log/example_test.go @@ -52,7 +52,7 @@ func Example() { } // Use a processor that filters out records based on the provided context. -func ExampleProcessor_filtering() { +func ExampleFilterProcessor() { // Existing processor that emits telemetry. var processor log.Processor = log.NewBatchProcessor(nil) @@ -84,14 +84,12 @@ type ContextFilterProcessor struct { log.Processor lazyFilter sync.Once - // Use the experimental FilterProcessor interface - // (go.opentelemetry.io/otel/sdk/log/internal/x). - filter filter + // Support the FilterProcessor interface for the embedded processor. + filter log.FilterProcessor } -type filter interface { - Enabled(ctx context.Context, param logapi.EnabledParameters) bool -} +// Compile time check. +var _ log.FilterProcessor = (*ContextFilterProcessor)(nil) func (p *ContextFilterProcessor) OnEmit(ctx context.Context, record *log.Record) error { if ignoreLogs(ctx) { @@ -100,9 +98,9 @@ func (p *ContextFilterProcessor) OnEmit(ctx context.Context, record *log.Record) return p.Processor.OnEmit(ctx, record) } -func (p *ContextFilterProcessor) Enabled(ctx context.Context, param logapi.EnabledParameters) bool { +func (p *ContextFilterProcessor) Enabled(ctx context.Context, param log.EnabledParameters) bool { p.lazyFilter.Do(func() { - if f, ok := p.Processor.(filter); ok { + if f, ok := p.Processor.(log.FilterProcessor); ok { p.filter = f } }) @@ -115,7 +113,7 @@ func ignoreLogs(ctx context.Context) bool { } // Use a processor which redacts sensitive data from some attributes. -func ExampleProcessor_redact() { +func ExampleProcessor() { // Existing processor that emits telemetry. var processor log.Processor = log.NewBatchProcessor(nil) diff --git a/sdk/log/filter_processor.go b/sdk/log/filter_processor.go new file mode 100644 index 000000000..5b99a4a99 --- /dev/null +++ b/sdk/log/filter_processor.go @@ -0,0 +1,62 @@ +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +package log // import "go.opentelemetry.io/otel/sdk/log" + +import ( + "context" + + "go.opentelemetry.io/otel/log" + "go.opentelemetry.io/otel/sdk/instrumentation" + "go.opentelemetry.io/otel/sdk/resource" +) + +// FilterProcessor is a [Processor] that knows, and can identify, what [Record] +// it will process or drop when it is passed to [Processor.OnEmit]. +// +// This is useful for users that want to know if a [log.Record] +// will be processed or dropped before they perform complex operations to +// construct the [log.Record]. +// +// The SDK's Logger.Enabled returns false +// if all the registered Processors implement FilterProcessor +// and they all return false. +// +// Processor implementations that choose to support this by satisfying this +// interface are expected to re-evaluate the [Record] passed to [Processor.OnEmit], +// it is not expected that the caller to OnEmit will use the functionality +// from this interface prior to calling OnEmit. +// +// See the [go.opentelemetry.io/contrib/processors/minsev] for an example use-case. +// It provides a Processor used to filter out [Record] +// that has a [log.Severity] below a threshold. +type FilterProcessor interface { + // Enabled returns whether the Processor will process for the given context + // and param. + // + // The passed param is likely to be a partial record information being + // provided (e.g a param with only the Severity set). + // If a Processor 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 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 param should not be held by the implementation. A copy should be + // made if the param 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, param EnabledParameters) bool +} + +// EnabledParameters represents payload for [FilterProcessor]'s Enabled method. +type EnabledParameters struct { + Resource resource.Resource + InstrumentationScope instrumentation.Scope + Severity log.Severity +} diff --git a/sdk/log/internal/x/README.md b/sdk/log/internal/x/README.md deleted file mode 100644 index 73f4db626..000000000 --- a/sdk/log/internal/x/README.md +++ /dev/null @@ -1,35 +0,0 @@ -# Experimental Features - -The Logs SDK contains features that have not yet stabilized. -These features are added to the OpenTelemetry Go Logs SDK prior to stabilization so that users can start experimenting with them and provide feedback. - -These feature may change in backwards incompatible ways as feedback is applied. -See the [Compatibility and Stability](#compatibility-and-stability) section for more information. - -## Features - -- [Filter Processors](#filter-processor) - -### Filter Processor - -Users of logging libraries often want to know if a log `Record` will be processed or dropped before they perform complex operations to construct the `Record`. -The [`Logger`] in the Logs Bridge API provides the `Enabled` method for just this use-case. -In order for the Logs Bridge SDK to effectively implement this API, it needs to be known if the registered [`Processor`]s are enabled for the `Record` within a context. -A [`Processor`] that knows, and can identify, what `Record` it will process or drop when it is passed to `OnEmit` can communicate this to the SDK `Logger` by implementing the `FilterProcessor`. - -By default, the SDK `Logger.Enabled` will return true when called. -Only if all the registered [`Processor`]s implement `FilterProcessor` and they all return `false` will `Logger.Enabled` return `false`. - -See the [`minsev`] [`Processor`] for an example use-case. -It is used to filter `Record`s out that a have a `Severity` below a threshold. - -[`Logger`]: https://pkg.go.dev/go.opentelemetry.io/otel/log#Logger -[`Processor`]: https://pkg.go.dev/go.opentelemetry.io/otel/sdk/log#Processor -[`minsev`]: https://pkg.go.dev/go.opentelemetry.io/contrib/processors/minsev - -## Compatibility and Stability - -Experimental features do not fall within the scope of the OpenTelemetry Go versioning and stability [policy](../../../../VERSIONING.md). -These features may be removed or modified in successive version releases, including patch versions. - -When an experimental feature is promoted to a stable feature, a migration path will be included in the changelog entry of the release. diff --git a/sdk/log/internal/x/x.go b/sdk/log/internal/x/x.go deleted file mode 100644 index ca78d1097..000000000 --- a/sdk/log/internal/x/x.go +++ /dev/null @@ -1,47 +0,0 @@ -// Copyright The OpenTelemetry Authors -// SPDX-License-Identifier: Apache-2.0 - -// Package x contains support for Logs SDK experimental features. -package x // import "go.opentelemetry.io/otel/sdk/log/internal/x" - -import ( - "context" - - "go.opentelemetry.io/otel/log" -) - -// 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 -// 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 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 param. - // - // 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 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 param. - Enabled(ctx context.Context, param log.EnabledParameters) bool -} diff --git a/sdk/log/logger.go b/sdk/log/logger.go index 36dea3130..6211d5d92 100644 --- a/sdk/log/logger.go +++ b/sdk/log/logger.go @@ -11,7 +11,6 @@ import ( "go.opentelemetry.io/otel/log" "go.opentelemetry.io/otel/log/embedded" "go.opentelemetry.io/otel/sdk/instrumentation" - "go.opentelemetry.io/otel/sdk/log/internal/x" "go.opentelemetry.io/otel/trace" ) @@ -50,14 +49,22 @@ func (l *logger) Emit(ctx context.Context, r log.Record) { // 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. func (l *logger) Enabled(ctx context.Context, param log.EnabledParameters) bool { - // If there are more Processors than FilterProcessors we cannot be sure - // that all Processors will drop the record. Therefore, return true. + p := EnabledParameters{ + Resource: *l.provider.resource, + InstrumentationScope: l.instrumentationScope, + Severity: param.Severity, + } + + // If there are more Processors than FilterProcessors, + // which means not all Processors are 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(l.provider.fltrProcessors) || anyEnabled(ctx, param, l.provider.fltrProcessors) + return len(l.provider.processors) > len(l.provider.fltrProcessors) || anyEnabled(ctx, p, l.provider.fltrProcessors) } -func anyEnabled(ctx context.Context, param log.EnabledParameters, fltrs []x.FilterProcessor) bool { +func anyEnabled(ctx context.Context, param EnabledParameters, fltrs []FilterProcessor) bool { for _, f := range fltrs { if f.Enabled(ctx, param) { // At least one Processor will process the Record. diff --git a/sdk/log/logger_test.go b/sdk/log/logger_test.go index 7cb2c4ffb..cd1e11374 100644 --- a/sdk/log/logger_test.go +++ b/sdk/log/logger_test.go @@ -224,11 +224,17 @@ func TestLoggerEnabled(t *testing.T) { p1 := newFltrProcessor("1", true) p2WithDisabled := newFltrProcessor("2", false) + emptyResource := resource.Empty() + res := resource.NewSchemaless(attribute.String("key", "value")) + testCases := []struct { - name string - logger *logger - ctx context.Context - expected bool + name string + logger *logger + ctx context.Context + expected bool + expectedP0Params []EnabledParameters + expectedP1Params []EnabledParameters + expectedP2Params []EnabledParameters }{ { name: "NoProcessors", @@ -241,41 +247,63 @@ func TestLoggerEnabled(t *testing.T) { logger: newLogger(NewLoggerProvider( WithProcessor(p0), WithProcessor(p1), - ), instrumentation.Scope{}), + WithResource(res), + ), instrumentation.Scope{Name: "scope"}), ctx: context.Background(), expected: true, + expectedP0Params: []EnabledParameters{{ + Resource: *res, + InstrumentationScope: instrumentation.Scope{Name: "scope"}, + }}, + expectedP1Params: nil, }, { name: "WithDisabledProcessors", logger: newLogger(NewLoggerProvider( WithProcessor(p2WithDisabled), + WithResource(emptyResource), ), instrumentation.Scope{}), - ctx: context.Background(), - expected: false, + ctx: context.Background(), + expected: false, + expectedP2Params: []EnabledParameters{{}}, }, { name: "ContainsDisabledProcessor", logger: newLogger(NewLoggerProvider( WithProcessor(p2WithDisabled), WithProcessor(p0), + WithResource(emptyResource), ), instrumentation.Scope{}), - ctx: context.Background(), - expected: true, + ctx: context.Background(), + expected: true, + expectedP2Params: []EnabledParameters{{}}, + expectedP0Params: []EnabledParameters{{}}, }, { name: "WithNilContext", logger: newLogger(NewLoggerProvider( WithProcessor(p0), WithProcessor(p1), + WithResource(emptyResource), ), instrumentation.Scope{}), - ctx: nil, - expected: true, + ctx: nil, + expected: true, + expectedP0Params: []EnabledParameters{{}}, + expectedP1Params: nil, }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { + // Clean up the records before the test. + p0.params = nil + p1.params = nil + p2WithDisabled.params = nil + assert.Equal(t, tc.expected, tc.logger.Enabled(tc.ctx, log.EnabledParameters{})) + assert.Equal(t, tc.expectedP0Params, p0.params) + assert.Equal(t, tc.expectedP1Params, p1.params) + assert.Equal(t, tc.expectedP2Params, p2WithDisabled.params) }) } } diff --git a/sdk/log/processor.go b/sdk/log/processor.go index fcab34c7a..c9b306f23 100644 --- a/sdk/log/processor.go +++ b/sdk/log/processor.go @@ -13,8 +13,7 @@ import ( // or with other methods. It is the responsibility of the Processor to manage // this concurrency. // -// See [go.opentelemetry.io/otel/sdk/log/internal/x] for information about how -// a Processor can be extended to support experimental features. +// See [FilterProcessor] for information about how a Processor can support filtering. type Processor interface { // OnEmit is called when a Record is emitted. // @@ -30,11 +29,11 @@ type Processor interface { // Handler. // // The SDK invokes the processors sequentially in the same order as - // they were registered using [WithProcessor]. + // they were registered using WithProcessor. // Implementations may synchronously modify the record so that the changes // are visible in the next registered processor. - // Notice that [Record] is not concurrent safe. Therefore, asynchronous - // processing may cause race conditions. Use [Record.Clone] + // Notice that Record is not concurrent safe. Therefore, asynchronous + // processing may cause race conditions. Use Record.Clone // to create a copy that shares no state with the original. OnEmit(ctx context.Context, record *Record) error diff --git a/sdk/log/provider.go b/sdk/log/provider.go index fa6f4afcc..096944ea1 100644 --- a/sdk/log/provider.go +++ b/sdk/log/provider.go @@ -15,7 +15,6 @@ import ( "go.opentelemetry.io/otel/log/embedded" "go.opentelemetry.io/otel/log/noop" "go.opentelemetry.io/otel/sdk/instrumentation" - "go.opentelemetry.io/otel/sdk/log/internal/x" "go.opentelemetry.io/otel/sdk/resource" ) @@ -30,7 +29,7 @@ const ( type providerConfig struct { resource *resource.Resource processors []Processor - fltrProcessors []x.FilterProcessor + fltrProcessors []FilterProcessor attrCntLim setting[int] attrValLenLim setting[int] } @@ -65,7 +64,7 @@ type LoggerProvider struct { resource *resource.Resource processors []Processor - fltrProcessors []x.FilterProcessor + fltrProcessors []FilterProcessor attributeCountLimit int attributeValueLengthLimit int @@ -206,10 +205,12 @@ func WithResource(res *resource.Resource) LoggerProviderOption { // // For production, use [NewBatchProcessor] to batch log records before they are exported. // For testing and debugging, use [NewSimpleProcessor] to synchronously export log records. +// +// See [FilterProcessor] for information about how a Processor can support filtering. func WithProcessor(processor Processor) LoggerProviderOption { return loggerProviderOptionFunc(func(cfg providerConfig) providerConfig { cfg.processors = append(cfg.processors, processor) - if f, ok := processor.(x.FilterProcessor); ok { + if f, ok := processor.(FilterProcessor); ok { cfg.fltrProcessors = append(cfg.fltrProcessors, f) } return cfg diff --git a/sdk/log/provider_test.go b/sdk/log/provider_test.go index 133e75289..19871b307 100644 --- a/sdk/log/provider_test.go +++ b/sdk/log/provider_test.go @@ -22,7 +22,6 @@ import ( "go.opentelemetry.io/otel/log" "go.opentelemetry.io/otel/log/noop" ottest "go.opentelemetry.io/otel/sdk/internal/internaltest" - "go.opentelemetry.io/otel/sdk/log/internal/x" "go.opentelemetry.io/otel/sdk/resource" ) @@ -65,9 +64,10 @@ type fltrProcessor struct { *processor enabled bool + params []EnabledParameters } -var _ x.FilterProcessor = (*fltrProcessor)(nil) +var _ FilterProcessor = (*fltrProcessor)(nil) func newFltrProcessor(name string, enabled bool) *fltrProcessor { return &fltrProcessor{ @@ -76,7 +76,8 @@ func newFltrProcessor(name string, enabled bool) *fltrProcessor { } } -func (p *fltrProcessor) Enabled(context.Context, log.EnabledParameters) bool { +func (p *fltrProcessor) Enabled(_ context.Context, params EnabledParameters) bool { + p.params = append(p.params, params) return p.enabled }