Skip to content

Conversation

@pellared
Copy link
Member

@pellared pellared commented Feb 13, 2025

Per #6271 (comment)

We agreed that we can move FilterProcessor directly to sdk/log as Logs SDK does not look to be stabilized soon.

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 #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:

I also created for tracking purposes:

@pellared pellared added pkg:SDK Related to an SDK package area:logs Part of OpenTelemetry logs labels Feb 13, 2025
@pellared pellared self-assigned this Feb 13, 2025
@codecov
Copy link

codecov bot commented Feb 13, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.8%. Comparing base (b80639c) to head (34bae5a).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main   open-telemetry/opentelemetry-go#6317   +/-   ##
=====================================
  Coverage   81.8%   81.8%           
=====================================
  Files        283     283           
  Lines      24892   24900    +8     
=====================================
+ Hits       20378   20387    +9     
+ Misses      4110    4109    -1     
  Partials     404     404           

see 4 files with indirect coverage changes

@pellared pellared marked this pull request as ready for review February 13, 2025 21:28
@pellared pellared merged commit 1ee7c79 into open-telemetry:main Feb 18, 2025
38 checks passed
@pellared pellared deleted the filterproc branch February 18, 2025 21:35
XSAM added a commit that referenced this pull request Mar 5, 2025
## Overview

This release is the last to support [Go 1.22].
The next release will require at least [Go 1.23].

### Added

- Add `ValueFromAttribute` and `KeyValueFromAttribute` in
`go.opentelemetry.io/otel/log`. (#6180)
- Add `EventName` and `SetEventName` to `Record` in
`go.opentelemetry.io/otel/log`. (#6187)
- Add `EventName` to `RecordFactory` in
`go.opentelemetry.io/otel/log/logtest`. (#6187)
- `AssertRecordEqual` in `go.opentelemetry.io/otel/log/logtest` checks
`Record.EventName`. (#6187)
- Add `EventName` and `SetEventName` to `Record` in
`go.opentelemetry.io/otel/sdk/log`. (#6193)
- Add `EventName` to `RecordFactory` in
`go.opentelemetry.io/otel/sdk/log/logtest`. (#6193)
- Emit `Record.EventName` field in
`go.opentelemetry.io/otel/exporters/otlp/otlplog/otlploggrpc`. (#6211)
- Emit `Record.EventName` field in
`go.opentelemetry.io/otel/exporters/otlp/otlplog/otlploghttp`. (#6211)
- Emit `Record.EventName` field in
`go.opentelemetry.io/otel/exporters/stdout/stdoutlog` (#6210)
- The `go.opentelemetry.io/otel/semconv/v1.28.0` package.
The package contains semantic conventions from the `v1.28.0` version of
the OpenTelemetry Semantic Conventions.
See the [migration documentation](./semconv/v1.28.0/MIGRATION.md) for
information on how to upgrade from
`go.opentelemetry.io/otel/semconv/v1.27.0`(#6236)
- The `go.opentelemetry.io/otel/semconv/v1.30.0` package.
The package contains semantic conventions from the `v1.30.0` version of
the OpenTelemetry Semantic Conventions.
See the [migration documentation](./semconv/v1.30.0/MIGRATION.md) for
information on how to upgrade from
`go.opentelemetry.io/otel/semconv/v1.28.0`(#6240)
- 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

- Update `github.com/prometheus/common` to v0.62.0., which changes the
`NameValidationScheme` to `NoEscaping`. This allows metrics names to
keep original delimiters (e.g. `.`), rather than replacing with
underscores. This is controlled by the `Content-Type` header, or can be
reverted by setting `NameValidationScheme` to `LegacyValidation` in
`github.com/prometheus/common/model`. (#6198)

### Fixes

- Eliminate goroutine leak for the processor returned by
`NewSimpleSpanProcessor` when `Shutdown` is called and the passed `ctx`
is canceled and `SpanExporter.Shutdown` has not returned. (#6368)
- Eliminate goroutine leak for the processor returned by
`NewBatchSpanProcessor` when `ForceFlush` is called and the passed `ctx`
is canceled and `SpanExporter.Export` has not returned. (#6369)

[Go 1.23]: https://go.dev/doc/go1.23
[Go 1.22]: https://go.dev/doc/go1.22

---------

Co-authored-by: Robert Pająk <[email protected]>
Co-authored-by: Tyler Yahn <[email protected]>
jsuereth pushed a commit to open-telemetry/opentelemetry-specification that referenced this pull request Mar 25, 2025
Fixes
#4363

Towards
#4208
(uses Severity Level passed via Logger.Enabled)

Towards stabilization of OpenTelemetry Go Logs API and SDK.

## Use cases

Below are some use cases where the new functionality can be used:

1. Bridge features like `LogLevelEnabled` in log bridge/appender
implementations. This is needed for **all** (but one) currently
supported log bridges in OTel Go Contrib.
2. Configure a minimum log severity level for a certain log processor.
3. 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`).
4. **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).
5. Bridge Logs API to a language-specific logging library (the other way
than usual).

## Changes

Add `Enabled` opt-in operation to the `LogRecordProcessor`.

I created an OTEP first which was a great for having a lot of
discussions and evaluations of different proposals:
-
#4290

Most importantly from
#4290 (comment):

> Among Go SIG we were evaluating a few times an alternative to provide
some new "filter" abstraction which is decoupled from the "processor".
However, we faced more issues than benefits going this route (some if
this is described here, but there were more issues:
open-telemetry/opentelemetry-go#5825 (comment)
. With the current opt-in `Processor.Enabled` we faced less issues so
far.
> We also do not want to replicate all features from the logging
libraries. If someone prefer the log4j (or other) filter design then
someone can always use a bridge and use log4j for filtering. `Enabled`
callback hook is the simplest design (yet very flexible) which makes it
easy to implement in the SDKs. This design is inspired from the design
of the two most popular Go structured logging libraries:
https://pkg.go.dev/log/slog (standard library) and
https://pkg.go.dev/go.uber.org/zap.
> 
> It is worth to adding that Rust design is similar and it also has an
`Enabled` hook. See
#4363 (comment).
Basically we want to add something like
https://docs.rs/log/latest/log/trait.Log.html#tymethod.enabled to the
`LogRecordProcessor` and allow users to implement `Enabled` in the way
that it will meet their requirements.
> 
> I also want to call out form
https://github.com/open-telemetry/opentelemetry-specification/blob/main/oteps/0265-event-vision.md#open-questions:
> 
> > How to support routing logs from the Logs API to a language-specific
logging library
> 
> To support this we would need a log record processor which bridges the
Logs API calls to given logging library. For such case we would need an
`Enabled` hook in `Processor` to efficiently bridge `Logger.Enabled`
calls. A filterer design would not satisfy such use case.

I decided to name the new operation `Enabled` as:
1. this name is already used in logging libraries in many languages:
#4439 (comment)
2. it matches the name of the API call (for all trace, metrics and logs
APIs).

I was also considering `OnEnabled` to have the same pattern as for
`Emit` and `OnEmit`. However, we already have `ForceFlush` and
`Shutdown` which does not follow this pattern so I preferred to keep the
simple `Enabled` name. For `OnEmit` I could also imagine `OnEmitted` (or
`OnEmitting`) which does something after (or just before like we have
`OnEnding` in `SpanProcessor`) `OnEmit` on all registered processors
were called. Yet, I do not imagine something similar for `Enabled` as
calling `Enabled` should not have any side-effects. Therefore, I decided
to name it `Enabled`.

I want to highlight that a processor cannot assume `Enabled` was called
before `OnEmit`, because of the following reasons:

1. **Backward compatibility** – Existing processors may already perform
filtering without relying on `Enabled`. For example: [Add Advanced
Processing to Logs Supplementary Guidelines
#4407](#4407).
2. **Self-sufficiency of `OnEmit`** – Since `Enabled` is optional,
`OnEmit` should be able to handle filtering independently. A processor
filtering events should do so in `OnEmit`, not just in `Enabled`.
3. **Greater flexibility** – Some processors, such as the ETW processor,
don’t benefit from redundant filtering. ETW already filters out events
internally, making an additional check unnecessary.
4. **Performance considerations** – Calling `Enabled` from `OnEmit`
introduces overhead, as it requires converting `OnEmit` parameters to
match `Enabled`'s expected input.
5. **Avoiding fragile assumptions** – Enforcing constraints that the
compiler cannot validate increases the risk of introducing bugs.


This feature is already implemented in OpenTelemetry Go:
- open-telemetry/opentelemetry-go#6317
We have one processor in Contrib which takes advantage of this
functionality:
- https://pkg.go.dev/go.opentelemetry.io/contrib/processors/minsev

This feautre (however with some differences) is also avaiable in OTel
Rust;
#4363 (comment):

> OTel Rust also has this capability. Here's an example where it is
leveraged to improve performance by dropping unwanted log early.
https://github.com/open-telemetry/opentelemetry-rust/blob/88cae2cf7d0ff54a042d281a0df20f096d18bf82/opentelemetry-appender-tracing/benches/logs.rs#L78-L85

---------

Co-authored-by: Tyler Yahn <[email protected]>
Co-authored-by: Sam Xie <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:logs Part of OpenTelemetry logs pkg:SDK Related to an SDK package

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants