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

Clarify that ReadableLogRecord and ReadWriteLogRecord can be represented using a single type #3898

Merged
merged 14 commits into from
Mar 21, 2024

Conversation

pellared
Copy link
Member

@pellared pellared commented Feb 23, 2024

Currently, the Logs SDK specification says:

In addition to the definition for LogRecord, 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 allocation12 which can noticeably affect the performance3
  • converting to a different struct (value type) is also not free

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

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 does not define anything like ReadableLogRecord

Maybe it will help other languages as well in implementing the SDK.

Footnotes

  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

@pellared pellared marked this pull request as ready for review February 23, 2024 09:23
@pellared pellared requested review from a team February 23, 2024 09:23
@pellared
Copy link
Member Author

pellared commented Feb 23, 2024

@open-telemetry/dotnet-maintainers, can you please provide some additional information why .NET Logs SDK does not define a ReadableLogRecord?

@pellared
Copy link
Member Author

CC @open-telemetry/go-approvers

@MrAlias
Copy link
Contributor

MrAlias commented Feb 23, 2024

cc @open-telemetry/specs-logs-approvers @tigrannajaryan

@MrAlias MrAlias added the spec:logs Related to the specification/logs directory label Feb 23, 2024
@tigrannajaryan
Copy link
Member

I am supportive of this clarification.

My opinion is that specification concepts are just that - concepts. They don't necessarily have to materialize as specific runtime objects or code symbols, although it is desirable to have them when possible for consistency reasons. The implementation should do whatever is idiomatic and performant for the language, provided that it conceptually follows the spec requirements.

As far as I know we never had an intent to prescribe specific language constructs to be used by implementations.

When the spec says "interface" it does not refer to any particular concrete definition of the "interface" in particular language (like Go interface). "Interface" is used in an abstract sense, as a defined boundary between code components. There is no obligation for the implementation for example in Go to use a Go interface when the spec says "interface". See how it is already relaxed in the spec:

Typically this will be implemented with a new interface or (immutable) value type.

So, a value type (e.g. a struct with methods), as an example, is valid implementation of a spec "interface".

It also says "typically", intentionally leaving further room for deviation if necessary.

@pellared pellared changed the title [DoNotMerge] Redefine ReadableLogRecord and ReadWriteLogRecord Redefine ReadableLogRecord and ReadWriteLogRecord Feb 26, 2024
@pellared pellared changed the title Redefine ReadableLogRecord and ReadWriteLogRecord Claridy that ReadableLogRecord and ReadWriteLogRecord can be represented using a single type Mar 13, 2024
@pellared pellared changed the title Claridy that ReadableLogRecord and ReadWriteLogRecord can be represented using a single type Clarify that ReadableLogRecord and ReadWriteLogRecord can be represented using a single type Mar 13, 2024
@pellared
Copy link
Member Author

I have update the PR so that the it contains only clarification changes.

@pellared pellared requested a review from jack-berg March 13, 2024 07:54
@pellared
Copy link
Member Author

PTAL @open-telemetry/specs-logs-approvers

@pellared
Copy link
Member Author

I am supportive of this clarification.

@tigrannajaryan PTAL. I did my best to address my concerns with minimal changes.

@tigrannajaryan
Copy link
Member

Has enough approvals, more than 2 days since last change, merging.

@tigrannajaryan tigrannajaryan merged commit 012f594 into open-telemetry:main Mar 21, 2024
7 checks passed
carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 31, 2024
…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spec:logs Related to the specification/logs directory
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants