Skip to content

Commit

Permalink
Clarify that ReadableLogRecord and ReadWriteLogRecord can be represen…
Browse files Browse the repository at this point in the history
…ted using a single type (open-telemetry#3898)

Currently, the [Logs SDK specification](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/sdk.md#additional-logrecord-interfaces) says:

> In addition to the [definition for LogRecord](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/data-model.md#log-and-event-record-definition), the following LogRecord-like interfaces are defined in the SDK: 
> ### ReadableLogRecord
> [...]
> Note: Typically this will be implemented with a new interface or (immutable) value type.
> ### ReadWriteLogRecord

Does it means that the SDK has to literally define those types (abstractions)?

The problem is that each abstraction/type conversion can lead (depending on language) to performance overhead.

E.g. for Go:
- casting a type to an interface can lead to a heap allocation[^1][^2] which can noticeably affect the performance[^3]
- converting to a different struct (value type) is also not free

Moreover, having less abstractions reduces the API surface and makes the design simpler.

[^1]: https://go101.org/optimizations/0.3-memory-allocations.html
[^2]: https://github.com/open-telemetry/opentelemetry-go/blob/main/log/DESIGN.md#record-as-interface
[^3]: https://tip.golang.org/doc/gc-guide#Eliminating_heap_allocations

I believe that for Logs signal, performance is more important than API esthetics. Based on https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/README.md, I think that the Logs SDK should be designed in a way to achieve to have high performance for a scenario when a OTLP exporter with a batch processor is used.

In my opinion, the `ReadableLogRecord`, `ReadWriteLogRecord` terms should only be used to describe what the functionalities accepting log records MUST be able to do with them. The key is that the log processor MUST be able to mutate the log record that is received in `OnEmit` while all other functionalities do not need mutate the record so they MAY  accept an immutable value.

I noticed that [.NET SDK](https://github.com/open-telemetry/opentelemetry-dotnet/tree/main/src/OpenTelemetry/Logs) does not define anything like `ReadableLogRecord`

Maybe it will help other languages as well in implementing the SDK.
  • Loading branch information
pellared authored Mar 21, 2024
1 parent da4048f commit af4af00
Show file tree
Hide file tree
Showing 2 changed files with 5 additions and 3 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ release.

- Refine description of Instrumentation Scope.
([#3855](https://github.com/open-telemetry/opentelemetry-specification/pull/3855))
- Clarify that `ReadableLogRecord` and `ReadWriteLogRecord` can be represented using a single type.
([#3898](https://github.com/open-telemetry/opentelemetry-specification/pull/3898))

### Events

Expand Down
6 changes: 3 additions & 3 deletions specification/logs/sdk.md
Original file line number Diff line number Diff line change
Expand Up @@ -129,8 +129,7 @@ the responsibility of the `LoggerProvider` instead.

## Additional LogRecord interfaces

In addition to the [definition for LogRecord](data-model.md#log-and-event-record-definition), the
following `LogRecord`-like interfaces are defined in the SDK:
In this document we refer to `ReadableLogRecord` and `ReadWriteLogRecord`, defined as follows.

### ReadableLogRecord

Expand All @@ -150,7 +149,8 @@ the [transformation to non-OTLP formats](../common/mapping-to-non-otlp.md#droppe
specification.

Note: Typically this will be implemented with a new interface or (immutable)
value type.
value type. The SDK may also use a single type to represent both `ReadableLogRecord`
and `ReadWriteLogRecord`.

### ReadWriteLogRecord

Expand Down

0 comments on commit af4af00

Please sign in to comment.