Asynchronous processors/exporters

[ghelyar]

Feature Request

Is your feature request related to a problem?

Exporting telemetry is likely to be an I/O based operation, such as writing it to a file or a socket.

The built-in types for exporting logs and traces, such as BaseProcessor, BatchExportProcessor and BaseExporter are synchronous. This means that any asynchronous calls need to be synchronously waited, using something like .GetAwaiter().GetResult(), as seen in the ZipkinExporter.

MetricExporter is already asynchronous.

Describe the solution you'd like:

It would be nice if these methods were asynchronous e.g. Task<ExportResult> BaseExporter.ExportAsync(in Batch<T> batch, CancellationToken cancellationToken).

[CodeBlanch]

Hey @ghelyar! This is kind of tricky to answer. They used to all be asynchronous. But the spec is very specific about how the exporters work. From https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/sdk.md#exportbatch:

Export() will never be called concurrently for the same exporter instance. Export() can be called again only after the current call returns.

At the moment writing into the batch is done [highly] concurrently using a special circular buffer. Export happens on a dedicated thread once the buffer has enough data or on an interval.

So this is all very much by design. Zipkin is doing .GetAwaiter().GetResult() because HttpClient doesn't have a synchronous API. Not sure how to solve that, at the moment. Any ideas?

/cc @reyang

[reyang]

@ghelyar would you help us to understand what are the outcome you would expect? For example, do you expect the exporting thread to be reused (e.g. share a pool of worker threads), or the export API to be async for easier integration with other async I/O APIs?

[ghelyar]

It was just to make it easier to write exporters. As exporters are very likely to use I/O, I expected it to have an ExportAsync. It's a bit nicer implementing ExportAsync and returning with a Task.FromResult if the implementation is synchronous, than implementing Export and using GetAwaiter().GetResult() if the implementation is asynchronous.

BatchExportProcessor using a background thread does make ExportAsync a bit less useful, although defining the shape of all exporters to be synchronous because one processor (albeit probably the most frequently used processor) has a background thread seems like the implementation is leaking into the abstraction a bit.

In terms of implementation, maybe add a virtual ValueTask<ExportResult> ExportAsync to BaseExporter, and also make Export virtual, with the default implementation of Export to call ExportAsync(batch).GetAwaiter().GetResult(). Exporter implementations can then choose whether to implement Export or ExportAsync. However, any processors that do exporting would still need to call Export, which possibly makes writing an exporter easier at the expense of making writing a processor more error prone. The only ways I can think of to get around that is to completely remove Export and have processors do the GetAwaiter().GetResult(), or to introduce another abstract class below BaseExporter that maps from Export to an abstract ExportAsync.

If the processors are synchronous and can't fire and forget the exports, that synchronous blocking has to happen somewhere I suppose, and if it has to happen inside the exporter, that isn't the end of the world.

[reyang]

In terms of implementation, maybe add a virtual ValueTask<ExportResult> ExportAsync to BaseExporter, and also make Export virtual, with the default implementation of Export to call ExportAsync(batch).GetAwaiter().GetResult(). Exporter implementations can then choose whether to implement Export or ExportAsync. However, any processors that do exporting would still need to call Export, which possibly makes writing an exporter easier at the expense of making writing a processor more error prone. The only ways I can think of to get around that is to completely remove Export and have processors do the GetAwaiter().GetResult(), or to introduce another abstract class below BaseExporter that maps from Export to an abstract ExportAsync.

Like the idea! I think this is doable and has a potential to simplify the job of writing exporters using async I/O.

[ThatRendle]

I've been playing with a trace Exporter that writes to Azure Table Storage and came looking for this issue.

For each Trace, the exporter writes to multiple tables (two or three depending on content), and also writes in batches. Maximum batch size for Azure Tables is 100, maximum batch size looks like 512, so there may be up to eighteen separate calls to table storage for a given batch.

The Azure.Data.Tables package has non-async methods (although I think those are calling GetAwaiter().GetResult() on the underlying HttpClient anyway) but using those methods would mean those writes had to run serially. Using the Async methods I can gather a bunch of Tasks together and await Task.WhenAll so the writes can happen in parallel.

Currently I'm just doing Task.WhenAll(tasks).ConfigureAwait(false).GetAwaiter().GetResult() to block, but as OP said, having an ExportAsync overload would feel cleaner.

[CodeBlanch]

I don't see what virtual ValueTask<ExportResult> ExportAsync really accomplishes. That just pushes the problem up to the caller (SDK) to do GetAwaiter().GetResult(). This is the Sync over async anti-pattern. The issue is we already have a dedicated thread for exporting. So starting a Task to do the work while we block that thread is just tying up 2 threads. Unless we force the task to run on the same thread. Might be a way to do that, I don't know of it off the top of my head but maybe runtime team can provide some insight.

[ghelyar]

For what it's worth, reactive extensions is very similar and lets you do this because an asynchronous operation can be mapped 1:1 to an observable. You can just use something like Buffer and SelectMany.

These don't block while it writes so multiple batches may be writing at the same time, but this might go against the otel spec.

I think if you really want to do this then the answer might be to just write your own processor rather than using the built in batch processor, and then call your exporter asynchronously.

Another way to do it would be to create an exporter that publishes to an IObservable and then observe it outside of otel.

The problem as I see it is that the implementation of the batch processor runs all exporters synchronously on a single thread, and that has caused the abstraction of the exporter to be synchronous, even though almost every exporter is going to do something with I/O asynchronously. This is the wrong way around. The abstraction should not be designed based on the implementation.

The sync over async antipattern is caused because the implementation of the batch processor is synchronous, just running on a separate thread. When exporters need to do async I/O and then block synchronously on it, this blocks that dedicated thread in the processor while it waits for I/O in the exporter. Sync over async is already happening.

If the spec says that activities must be exported sequentially, which I think it does but I haven't read it for a while, it might all be a bit moot anyway.

[ThatRendle]

The issue with using an IObservable (or a Channel or whatever) and offloading the write to run asynchronously is that the Export method returns a result saying whether the export was successful or not. If you offload to a background process of any kind you can't return an honest result: you don't know whether the data was actually written to the external store or not.

[ghelyar]

Yes, you are effectively telling OpenTelemetry that you have successfully published to an in-memory sink, and it's up to something else to persist that. It's really just a work around for a limitation of OpenTelemetry.

You can also write your own processor that calls your exporter asynchronously, but unless you're willing to overlap the asynchronous exports it doesn't really gain you very much. It might use the thread pool rather than a background thread, but you would have to measure the performance to know if it was better or worse.

If you want it to finish writing one set of events before it starts writing the next, which I think is what the spec says it must do, the current implementation does that, it just doesn't smell very nice that the processor expects all exporters to be synchronous when really most exporters will want to do some asynchronous I/O.

[reyang]

OpenTelemetry Processors by design runs synchronously, and they have read/write access to the Context. I guess it is very unlikely that the spec will introduce async processors (considering async exporting only occurs when the BatchExportProcessor is used).

For Exporters, it seems there are two main reasons that people might want async exporter:

1. Simplicity, easiness - in my exporter code I have to call async I/O methods, doing all the sync over async or vice versa is a tedious job.
2. Performance, throughput - in my exporter I want to leverage multiple CPU cores (either by having multiple explicit threads, or using overlapped I/O backed by some thread pools) so I can have better throughput for CPU intensive operations (e.g. serialization).

The 2nd case would definitely touch the semantic of the ExportResult, which is defined by the spec. By having the async/deferred exporter, the semantic could essentially become "By returning Success, I acknowledge that I have verified the correctness/integrity of the payload and commit to deliver it, but there could be things out of my control that I failed my promise (e.g. process crashed, container trashed/recycled)".