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.

Sign up for GitHub

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

Jump to bottom

Add SimpleConcurrentProcessor for Logs, clarify existing export concurrency #4163

Closed
wants to merge 8 commits into from
Closed

Add SimpleConcurrentProcessor for Logs, clarify existing export concurrency #4163

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f2b9c32
Add SimpleConcurrentProcessor, clarify existing export concurrency
cijothomas Jul 25, 2024
8ecc57b
toc
cijothomas Jul 25, 2024
71114f8
Merge branch 'main' into cijothomas/processor
cijothomas Jul 25, 2024
de530e8
Update specification/logs/sdk.md
pellared Jul 26, 2024
109f817
Merge branch 'main' into cijothomas/processor
cijothomas Jul 26, 2024
8d70036
add review suggestions
cijothomas Jul 26, 2024
e445921
Merge branch 'main' into cijothomas/processor
cijothomas Jul 29, 2024
a40ab74
Merge branch 'main' into cijothomas/processor
cijothomas Aug 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,9 @@ release.

- The SDK MAY provide an operation that makes a deep clone of a `ReadWriteLogRecord`.
([#4090](https://github.com/open-telemetry/opentelemetry-specification/pull/4090))
- Add the in-development Simple Concurrent log record processor.
([#4163](https://github.com/open-telemetry/opentelemetry-specification/pull/4163))
- Clarify that `Export` should not called by built-in processors concurrently.

### Events

Expand Down
39 changes: 30 additions & 9 deletions specification/logs/sdk.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
+ [Simple processor](#simple-processor)
+ [Batching processor](#batching-processor)
+ [Isolating processor](#isolating-processor)
+ [Simple Concurrent processor](#simple-concurrent-processor)
- [LogRecordExporter](#logrecordexporter)
* [LogRecordExporter operations](#logrecordexporter-operations)
+ [Export](#export)
Expand Down Expand Up @@ -392,9 +393,10 @@ in [OpenTelemetry Collector](../overview.md#collector).
#### Simple processor

This is an implementation of `LogRecordProcessor` which passes finished logs and
passes the export-friendly `ReadableLogRecord` representation to the
configured [LogRecordExporter](#logrecordexporter), as soon as they are
finished.
passes the export-friendly `ReadableLogRecord` representation to the configured
[LogRecordExporter](#logrecordexporter), as soon as they are finished. The
exporter methods of the exporter MUST NOT be called concurrently for the same
exporter instance.

**Configurable parameters:**

Expand Down Expand Up @@ -433,6 +435,21 @@ the configured `processor`.

* `processor` - processor to be isolated.

#### Simple Concurrent processor
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I strongly suggest NOT using the term "concurrent" as usually it conveys that there is some of synchronization behind. E.g. "concurrent collections" are collections that can be used in multithreaded code. In some languages there is an idiom that if the type has Concurrent in its name, the method calls are synchronized.

My naming proposal is "Passthrough processor"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This resonates with me too -- the use of "concurrent" suggests some kind of attention to concurrency, a limit of some sort in addition to offering concurrency. Maybe the name could be AsynchronousProcessor -- starts the export and returns success immediately.

I admit this worries me a bit -- is the exporter expected to implement some kind of limit, to avoid running out of memory? Can this be composed with the BatchSpanProcessor? What will control whether the BatchSpanProcessor drops spans vs. this processor consuming unlimited amounts of data?

Here, we have an OTel Collector processor that offers unlimited concurrency, but subject to a limit on the total amount of pending data, I consider it a potential solution to the problems posed above: https://github.com/open-telemetry/otel-arrow/tree/main/collector/processor/concurrentbatchprocessor

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe the name could be AsynchronousProcessor -- starts the export and returns success immediately.

That is not the intend. The intended exporters here not just 'starts' the export. It does everything inline (i.e serialization, writing to destination.), and then return success/failure. (from what I know, such exporters are typically writing to ETW, user-events)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the exporter expected to implement some kind of limit, to avoid running out of memory?

No. Such exporters (etw/user-events) does not buffer anything. The logRecord is serialized and handed over to destination inline. ETW/user-events system is backed by OperatingSystem kernel memory, and they have ample mechanisms to keep memory in control, but those are outside the scope of the exporter.

Copy link
Member

@pellared pellared Jul 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cijothomas any thoughts about my naming proposal?

My other proposals are

  • "Direct processor" meaning that it directly calls the exporter.

  • "Adapter processor" meaning that it is only an adapter and has no logic/ synchronization

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

naming is hard 🤣 I am not sure if any of the alternate suggestions are significantly better. I'll keep thinking and hope we get more suggestions.


**Status**: [Development](../document-status.md)

This is an implementation of `LogRecordProcessor` which passes finished logs and
passes the export-friendly `ReadableLogRecord` representation to the configured
[LogRecordExporter](#logrecordexporter), as soon as they are finished. Unlike
[Simple processor](#simple-processor), this can invoke exporter methods
concurrently, and hence MUST be documented clearly to be used only with
concurrent safe exporters.

**Configurable parameters:**

* `exporter` - the concurrent safe exporter where the `LogRecord`s are pushed.

## LogRecordExporter

`LogRecordExporter` defines the interface that protocol-specific exporters must
Expand All @@ -453,14 +470,18 @@ Exports a batch of [ReadableLogRecords](#readablelogrecord). Protocol exporters
that will implement this function are typically expected to serialize and
transmit the data to the destination.

`Export` will never be called concurrently for the same exporter instance.
Depending on the implementation the result of the export may be returned to the
`Export` MAY be called concurrently for the same exporter instance if paired
with [Simple Concurrent Processor](#simple-concurrent-processor). Each exporter
implementation MUST document whether it is concurrent safe, and hence can be
paired with the [Simple Concurrent Processor](#simple-concurrent-processor).

Depending on the implementation, the result of the export may be returned to the
Processor not in the return value of the call to `Export` but in a language
specific way for signaling completion of an asynchronous task. This means that
while an instance of an exporter will never have it `Export` called concurrently
it does not mean that the task of exporting can not be done concurrently. How
this is done is outside the scope of this specification. Each implementation
MUST document the concurrency characteristics the SDK requires of the exporter.
while an instance of an exporter that does not support concurrent calls will
never have `Export` called concurrently, it does not mean that the task of
exporting cannot be done concurrently. How this is done is outside the scope of
this specification.

`Export` MUST NOT block indefinitely, there MUST be a reasonable upper limit
after which the call must time out with an error result (`Failure`).
Expand Down
Loading