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
+33 −9
Closed

Add SimpleConcurrentProcessor for Logs, clarify existing export concurrency #4163

Show file tree
Hide file tree
Changes from 4 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
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,10 @@ release.

### Logs

- 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

### Resource
Expand Down
41 changes: 32 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 @@ -386,9 +387,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
export() method of the exporter MUST NOT be called concurrently for the same
exporter instance.
cijothomas marked this conversation as resolved.
Show resolved Hide resolved

**Configurable parameters:**

Expand Down Expand Up @@ -427,6 +429,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 export() concurrently,
cijothomas marked this conversation as resolved.
Show resolved Hide resolved
and hence MUST be documented clearly to be used only with exporters that support
concurrent export() calls.
cijothomas marked this conversation as resolved.
Show resolved Hide resolved

**Configurable parameters:**

* `exporter` - the exporter where the `LogRecord`s are pushed.
cijothomas marked this conversation as resolved.
Show resolved Hide resolved

## LogRecordExporter

`LogRecordExporter` defines the interface that protocol-specific exporters must
Expand All @@ -447,14 +464,20 @@ 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 supports concurrent `Export` calls, and
Copy link
Member

Choose a reason for hiding this comment

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

  1. We cannot force custom implementation on how they document their implementations
  2. It is not only about exporting but also ForceFlush and Shutdown
Suggested change
implementation MUST document whether it supports concurrent `Export` calls, and
implementation provided by the SDK MUST document whether it is concurrent safe, and

Copy link
Member Author

Choose a reason for hiding this comment

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

We cannot enforce, but the spec wording is to make sure any person authoring own exporter should follow this spec. If they don't follow, then there could be undesired behavior.
Alternatively, we can word it in such a way that "It must be documented to the exporter authors that they should document the exporters' concurrency characteristics....

Copy link
Member

Choose a reason for hiding this comment

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

I believe this was part of why it was went with that the processor should called all exporters the same and not have to worry about concurrency.

Copy link
Member

Choose a reason for hiding this comment

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

@tsloughter, the users could still use processors that does not require the exporters to be concurrent safe. But we would give a more performant option for cases where exporter are concurrent safe and can be used synchronously.

@cijothomas, the alternative could be that such exporter packages (like ETW or user_events exporters) would provide their own processors which are "tailored" to their exporters. It can give more flexibility and not increase the SDK functionality surface.

Copy link
Member Author

Choose a reason for hiding this comment

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

the alternative could be that such exporter packages (like ETW or user_events exporters) would provide their own processors which are "tailored" to their exporters

You are right! That is exactly what we have been doing for years. But that does not mean spec shouldn't support them.
The increase in SDK functionality surface is necessary, to support such scenarios. The spec already has wording that state implementations must have simple and batch (meaning others are optional), so additional processors don't put extra burden on implementations, unless they chose to support.

Copy link
Member

Choose a reason for hiding this comment

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

What is more (I have forgotten tot call it out explictly) I think that e.g. for ETW we can simply provide a single EtwProcessor which does the exporting. There is no need for batching or synchronization so there is no need to provide an Exporter interface implementation.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that e.g. for ETW we can simply provide a single EtwProcessor which does the exporting

Yes that is also possible. (In certain ways, OTel Rust does that. Its etwexporter is not really an exporter, but a ~processor).
But I think it is best to model exporters (the thing which does serialization, export telemetry to outside the process) as exporters itself consistently.

Copy link
Member

@pellared pellared Aug 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

But I think it is best to model exporters

I am not really sure as these exporters are emitting batches. And for use cases like ETW and user_events you probably prefer to operate on "single" log record.

Copy link
Member Author

Choose a reason for hiding this comment

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

They are exporters! (They do the job of serializing and transferring the telemetry to an external entity.) Whether it gets a batch of 1 item or multiple is purely based on choice of processor used. (when used with SimpleProcessor, even OTLPExporter gets a batch of single item only).

hence can be paired with the [Simple Concurrent
Processor](#simple-concurrent-processor).
cijothomas marked this conversation as resolved.
Show resolved Hide resolved

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. Each implementation MUST document the concurrency
characteristics the SDK requires of the exporter.
cijothomas marked this conversation as resolved.
Show resolved Hide resolved

`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