-
Notifications
You must be signed in to change notification settings - Fork 933
OTEP: Logger.Enabled #4290
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
OTEP: Logger.Enabled #4290
Changes from 54 commits
b78c2b3
dad7d90
8d194d6
e90776f
44c26df
0dfdb46
28c7ca4
5707a91
f26dfaa
26f915c
ba900ee
84a2779
d092b72
a5aee99
45b6c90
d258965
cf05e57
4e60d96
8096622
21b1702
36bcf8d
fa7ad76
18424ce
2bf0540
e907d2e
9cd2311
9f3ed05
e108b90
b782c8f
1bc5bb4
ce6d2a6
6978351
6d25690
b963292
e1e727b
aee7348
93860bf
e4cd587
ebab731
8b98332
2bbdab7
472d151
96fbf43
4ee1696
a6a87a9
509c501
222425d
0e6c0ae
07cf515
0c555f9
48c1e11
9431fac
be8343e
1a5761e
c5c8f3b
76d59e3
257fbf3
ccc1b92
bb36dd3
fe2772b
84205c3
a298337
b01b35d
cb2dcbd
04da313
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,259 @@ | ||
| # Logger.Enabled | ||
|
|
||
| ## Motivation | ||
|
|
||
| In applications requiring high performance, | ||
| while optimizing the performance of enabled logging is important, | ||
| it is often equally or more critical to minimize the overhead of logging | ||
| for disabled or un-exported logs. | ||
|
|
||
| The consumers of OpenTelemetry clients want to: | ||
|
|
||
| 1. **Correctly** and **efficiently** bridge features | ||
| like `LogLevelEnabled` in log bridge/appender implementations. | ||
| 2. Avoid allocating memory to store a log record, | ||
| avoid performing computationally expensive operations, | ||
| and avoid exporting | ||
| when emitting a log or event record is unnecessary. | ||
| 3. Configure a minimum a log severity level on the SDK level. | ||
pellared marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| 4. Filter out log and event records when they are inside a span | ||
| that has been sampled out (span is valid and has sampled flag of `false`). | ||
| 5. **Efficiently** support high-performance logging destination | ||
| like [Linux user_events](https://docs.kernel.org/trace/user_events.html) | ||
| and [ETW (Event Tracing for Windows)](https://learn.microsoft.com/windows/win32/etw/about-event-tracing). | ||
| 6. Allow **fine-grained** filtering control for logging pipelines | ||
| when using an OpenTelemetry Collector is not feasible | ||
| e.g., for mobile devices, serverless, or IoT. | ||
|
|
||
| Without a `Logger.Enabled` check in the OpenTelemetry Logs API | ||
| and corresponding implementations in the SDK, | ||
| achieving this goal is not feasible. | ||
|
|
||
| OpenTelemetry Logs SDK users require: | ||
|
|
||
| - A declarative configuration to conveniently address | ||
| the most popular use cases. | ||
|
Comment on lines
+34
to
+35
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems to be doing too much. Why not just focus on adding logger enabled like the title indicates? |
||
| - A dynamic hook for advanced customization and flexibility. | ||
|
|
||
| The main purpose of this OTEP is to have foundations for: | ||
|
|
||
| - extending the SDK's `LoggerConfig` with `minimum_severity_level` field, | ||
| - extending the `LogRecordProcessor` with an `Enabled` operation, | ||
|
|
||
| and address [Specify how Logs SDK implements Enabled #4207](https://github.com/open-telemetry/opentelemetry-specification/issues/4207). | ||
|
|
||
| ## Explanation | ||
|
|
||
| For (1) (2), the user can use the Logs API `Logger.Enabled` function, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These number references are confusing as the seem to be referencing something from another section. |
||
| which tells the user whether a `Logger` for given arguments | ||
| is going to emit a log record. | ||
|
|
||
| For (3), the user can declaratively configure the Logs SDK | ||
| using `LoggerConfigurator` to set the `minimum_severity_level` | ||
lmolkova marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| of a `LoggerConfig` for given `Logger`. | ||
|
|
||
| For (4), the user can use the Tracing API to check whether | ||
| there is a sampled span in the current context before creating | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is the user here referring to end-users? Are you expecting users to do this pattern? do_something_important() do_something_important() I think this should be a SDK setting and instrumentations should not have to worry about this. They always call logger.log. Upto SDK to decide (based on config) if the log must be dropped as the active span is not recorded. (Sorry If I misunderstood the section!) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. It could be also implemented via a Processor which filters out not recorded spans. |
||
| and emitting a log record. | ||
|
|
||
| For (5) (6), the user can hook to `Logger.Enabled` Logs API calls | ||
| by adding to the Logs SDK a `LogRecordProcessor` implementing `Enabled` | ||
| (or alternatively other hook such as `LogRecordFilterer`; | ||
| see [here](#separate-logrecordfilterer-abstraction)). | ||
|
|
||
| ## Internal details | ||
|
|
||
| Regarding (1) (2), the Logs API specification has already introduced `Logger.Enabled`: | ||
|
|
||
| - [Add Enabled method to Logger #4020](https://github.com/open-telemetry/opentelemetry-specification/pull/4020) | ||
| - [Define Enabled parameters for Logger #4203](https://github.com/open-telemetry/opentelemetry-specification/pull/4203) | ||
|
|
||
| The addition of `LoggerConfig.minimum_severity_level` is supposed | ||
| to serve the (3) use case in an easy-to-setup and efficient way. | ||
|
|
||
| Regarding (4) the API callers can decide whether to emit log records | ||
| for spans that are not sampled. Example in Go: | ||
pellared marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| <!-- markdownlint-disable no-hard-tabs --> | ||
| ```go | ||
| if trace.SpanContextFromContext(ctx).IsSampled() && logger.Enabled(ctx, params) { | ||
| logger.Emit(ctx, createLogRecord(payload)) | ||
| } | ||
| ``` | ||
| <!-- markdownlint-enable no-hard-tabs --> | ||
|
|
||
| For instrumentation libraries, the API-level control might be more appropriate, | ||
| than configuring such behavior on the SDK level. | ||
pellared marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| The addition of `LogRecordProcessor.Enabled` is necessary for | ||
| use cases where filtering is dynamic and coupled to processing, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd like to get more clarification on this. For 5 (when exporting to OS kernel facilities like etw/user-events), this is primarily a perf saving mechanism. If OS says nobody is interested in your log/event, then the producer can short circuit. This has to be done very quickly and typically only has 1-2 top-level parameter like Severity/EventName/ScopeName. 6 is referring to scenarios where users want to do filtering, primarily to control volume of telemetry exported. This usually is done based on LogRecord (i.e body/attributes/name/severity etc.). I believe this is already doable by writing normal LogRecordProcessors, like the one shown in #4407 In other words, I see 5 requires LogProcessor.Enabled, but 6 does not require. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks. My description of 6 is indeed not clear. I meant that the perf saving mechanism works when the procesors are composed - supports the advanced scenarios which I described in #4407 |
||
| such as (5) and (6). | ||
|
|
||
| Both `LoggerConfig` and registered `LogRecordProcessors` take part | ||
| in the evalaution of the `Logger.Enabled` operation. | ||
| If it returns `true`, meaning the logger is enabled, | ||
pellared marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| then the `LogRecordProcessors` are evaluated. | ||
| In such cases, `false` is returned only if all registered | ||
| `LogRecordProcessors` return `false` in their `Enabled` calls, | ||
| as this means no processor will process the log record. | ||
| Pseudo-code: | ||
|
|
||
| <!-- markdownlint-disable no-hard-tabs --> | ||
| ```go | ||
| func (l *logger) Enabled(ctx context.Context, param EnabledParameters) bool { | ||
| config := l.config() | ||
| if config.Disabled { | ||
| // The logger is disabled. | ||
| return false | ||
| } | ||
| if params.Severity > config.MinSeverityLevel { | ||
dashpole marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| // The severity is less severe than the logger minimum level. | ||
| return false | ||
| } | ||
|
|
||
| processors := l.provider.processors() | ||
| for _, p := range processors { | ||
| if p.Enabled(ctx, param) { | ||
pellared marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| // At least one processor will process the record. | ||
| return true | ||
| } | ||
| } | ||
| // No processor will process the record. | ||
| return false | ||
| } | ||
| ``` | ||
| <!-- markdownlint-enable no-hard-tabs --> | ||
|
|
||
| There is nothing preventing having both `LoggerConfig.minimum_severity_level` | ||
| and something like a `MinimumSeverityLevelProcessor`. | ||
|
|
||
| `LoggerConfig.minimum_severity_level` is a configuration for concrete loggers. | ||
| For instance, it would be a easy to use feature when one would like to change | ||
| the minimum severity level for all loggers with names matching | ||
| `MyCompany.NoisyModule.*` wildcard pattern. | ||
| With `LoggerConfigurator` the user is not able to change/apply a processor. | ||
|
|
||
| `MinimumSeverityLevelProcessor` is for configuring log processing pipelines. | ||
| It is the only choice when one would like to set the minimum severity level | ||
| for a certain exporting pipeline. | ||
| For example, one batching processor would be exporting logs only above info level | ||
| via OTLP and a second simple processor would be exporting all logs to stdout. | ||
|
|
||
| ## Trade-offs and mitigations | ||
|
|
||
| For some langagues extending the `LogRecordProcessor` may be seen as breaking. | ||
| For these languages implementing `LogRecordProcessor.Enabled` must be optional. | ||
| The SDK `LogRecordProcessor` must return `true` if `Enabled` is not implemented. | ||
| This approach is currently taken by OpenTelemetry Go. | ||
|
|
||
| ## Prior art | ||
|
|
||
| `Logger.Enabled` is already defined by: | ||
|
|
||
| - [OpenTelemetry C++](https://github.com/open-telemetry/opentelemetry-cpp/blob/main/api/include/opentelemetry/logs/logger.h) | ||
| - [OpenTelemetry Go](https://github.com/open-telemetry/opentelemetry-go/blob/main/log/logger.go) | ||
| - [OpenTelemetry PHP](https://github.com/open-telemetry/opentelemetry-php/blob/main/src/API/Logs/LoggerInterface.php) | ||
| - [OpenTelemetry Rust](https://github.com/open-telemetry/opentelemetry-rust/blob/main/opentelemetry/src/logs/logger.rs) | ||
|
|
||
| `LoggerConfig` (with only `disabled`) is already defined by: | ||
|
|
||
| - [OpenTelemetry Java](https://github.com/open-telemetry/opentelemetry-java/blob/main/sdk/logs/src/main/java/io/opentelemetry/sdk/logs/internal/LoggerConfig.java) | ||
| - [OpenTelemetry PHP](https://github.com/open-telemetry/opentelemetry-php/blob/main/src/SDK/Logs/LoggerConfig.php) | ||
|
|
||
| `LogRecordProcessor.Enabled` is already defined by: | ||
|
|
||
| - [OpenTelemetry Go](https://github.com/open-telemetry/opentelemetry-go/tree/main/sdk/log/internal/x) | ||
| - [OpenTelemetry Rust](https://github.com/open-telemetry/opentelemetry-rust/blob/main/opentelemetry-sdk/src/logs/log_processor.rs) | ||
|
|
||
| Regarding the (5) use case, | ||
| OpenTelemetry Rust provides | ||
| [OpenTelemetry Log Exporter for Linux user_events](https://github.com/open-telemetry/opentelemetry-rust-contrib/blob/1cb39edbb6467375f71f5dab25ccbc49ac9bf1d5/opentelemetry-user-events-logs/src/logs/exporter.rs) | ||
| enabling efficient log emission to user_events. | ||
|
|
||
| Regarding the (6) use case, | ||
| OpenTelemetry Go Contrib provides | ||
| [`minsev` processor](https://pkg.go.dev/go.opentelemetry.io/contrib/processors/minsev) | ||
| allowing distinict minimum severity levels | ||
| for different log destinations. | ||
|
|
||
| ## Alternatives | ||
|
|
||
| Below are some alternatives that would be further explored | ||
| during the Development phase. | ||
|
|
||
| ### Separate LogRecordFilterer Abstraction | ||
|
|
||
| There is a [proposal](https://github.com/open-telemetry/opentelemetry-specification/issues/4207#issuecomment-2354859647) | ||
| to add a distinct `LogRecordFilterer` abstraction. | ||
| Here is [a dicsussion](https://github.com/open-telemetry/opentelemetry-specification/pull/4290#discussion_r1887795096). | ||
|
|
||
| However, this approach is less suited for use case (5) | ||
| and offers limited flexibility for use case (6). | ||
| The initial feedback after [an experiment](https://github.com/open-telemetry/opentelemetry-go/pull/5825) | ||
| with `LogRecordFilterer` was that it can make composing | ||
| processing pipelines harder and more bug-prone. | ||
|
|
||
| Adding `Enabled` to `LogRecordProcessor` should be: | ||
|
|
||
| - Simpler. There is no need for separate code for | ||
| global filterers and processor filterers. | ||
| - More composable. Filtering could be applied at any place, | ||
| even after some pre-processing. | ||
| - More cohesive: Filtering can be coupled to processing, | ||
| e.g. making a rate limiting processor should be more | ||
| straighforward. | ||
|
|
||
| ## Open questions | ||
|
|
||
| ### Need of LogRecordExporter.Enabled | ||
|
|
||
| There is a [proposal](https://github.com/open-telemetry/opentelemetry-specification/pull/4290#discussion_r1878379347) | ||
| to additionally extend `LogRecordExporter` with `Enabled` operation. | ||
| However, at this point of time, this extension seems unnecessary. | ||
| The `LogRecordExporter` abstraction is primarily designed | ||
| for batching exporters, whereas the (5) use cases is focused | ||
| on synchronous exporters, which can be implemented as a `LogRecordProcessor`. | ||
| Skipping this additional level of abstraction would also reduce overhead. | ||
| It is worth noticing that, `LogRecordExporter.Enabled` | ||
| can always be added in future. | ||
|
|
||
| ## Future possibilities | ||
|
|
||
| ### Extending LoggerConfig | ||
|
|
||
| In future, more fields can be added to `LoggerConfig` | ||
| in order to conveniently address the most popular use cases. | ||
|
|
||
| One example could be the addition of `LoggerConfig.trace_based`. | ||
| This configuration can be used only capture the log records that are | ||
| within sampled spans. It could serve the (4) use case in a declarative way | ||
| configured on the SDK level. This configuration was discussed | ||
| e.g. [here](https://github.com/open-telemetry/opentelemetry-specification/pull/4290#discussion_r1898672657) | ||
| and is beyond the scope of this OTEP. | ||
|
|
||
| ### Dynamic Evaluation in LoggerConfig | ||
|
|
||
| There is a [proposal](https://github.com/open-telemetry/opentelemetry-specification/issues/4207#issuecomment-2501688210) | ||
| suggested dynamic evaluation in `LoggerConfig` instead of static configuration | ||
| to make the `LoggerConfig` to support dynamic evaluation. | ||
|
|
||
| Additionally, nothing prevents adding some other way of configuring Loggers similar to | ||
| [Metrics View](../../specification/metrics/sdk.md#view). | ||
| This was proposed [here](https://github.com/open-telemetry/opentelemetry-specification/pull/4290#discussion_r1908897211). | ||
|
|
||
| It is worth mentioning that the purpose of `LoggerConfig` is `Logger` configuration. | ||
| It won't solve use cases (5) and (6) that are tied to log processing pipelines. | ||
| This mechanism could not be used to setup different filters for each registered | ||
| processor. | ||
|
|
||
| ### Extending Logger.Enabled | ||
|
|
||
| The `Enabled` API could be extended in the future | ||
| to include additional parameters, such as `Event Name`, | ||
| for processing event records. | ||
| This would fit well a simple design where `LogRecordProcessor` | ||
| is used for both log records and event records. | ||
| References: | ||
|
|
||
| - [Add EventName parameter to Logger.Enabled #4220](https://github.com/open-telemetry/opentelemetry-specification/issues/4220). | ||
| - OpenTelemetry Go: [[Prototype] log: Events support with minimal non-breaking changes #6018](https://github.com/open-telemetry/opentelemetry-go/pull/6018) | ||
Uh oh!
There was an error while loading. Please reload this page.