[Proposal] Move batching to `exporterhelper`

[bogdandrutu]

Right now batching capability is offered as a processor, and this is extremely generic in our current pipeline model, which is a great thing, but also comes with limitations:

1. Batch processor uses a queue (implemented as a channel), which is not the same as the one used in the queue retry, that can lose data when the binary crashes even after the client was informed that data were accepted. There is already work to offer a persistent queue mechanism which will not be available in the processor.
2. If users want to use batching with the routing processor: will need to fix the Change batch processor to take client info into consideration #4544 issue, AND this will batch data that later will be split again. Very unnecessary.
3. SignalFx/SplunkHec receiver/exporter uses a "hack" that adds one header from the incoming request to the resource, batches things, then has to split based on that header. Batching inside the exporter helper can be configured with possible custom logic much easier, by making the batching library accepting a custom "batcher" func.
4. Allow customization of the "size" function. There were requests to support batching by the serialized size, we can offer this in the exporter helper much nicer since the custom sizer can actually work using the exporter wire format. So batching can happen after data serialization.
5. When multiple exporters configured, they may have different batching requirements. This can be achieved today with multiple pipelines, but that causes code duplicate between the pipelines.

This will also fix some more problems:

• Change batch processor to take client info into consideration #4544
• otelcol can't get cpu limit and may cause performance issues. #4459 - by using the same workers defined in the queue_retry for this, hence we allow users to control this and we can still default on NumCpu.
• Global sampling service telemetry logging configuration options #4554 - nothing to fix :)

The proposal is to look into offer the "batching" capability similar to timeout/queue/retry logic that we have in the exporterhelper.

[tigrannajaryan]

A couple quick counterarguments:

1. Dedicated batch processor keeps the batch in memory once even when there are multiple exporters in the pipeline. If we move the equivalently sized batch to queuedretry of every exporter we will slightly increase memory requirements (not by much since most of pdata is going to be shared assuming exporters' output throughput more or less keeps up with the input).
2. Virtually the same batching work needs to happen for every exporter, which adds some CPU work per exporter. It would be useful to benchmark to see how much this difference is when there is more than one exporter.
3. Batching probably helps to improve the performance of subsequent processors in the pipeline. We haven't benchmarked this, but it is possible that there is non-zero per-pdata cost that is amortized better if the batches are larger. This may be negligible, but I would like to see some benchmarks measuring it.

These are not necessarily strong enough to block the proposal, but I think it is worth thinking about these.

There is already work to offer a persistent queue mechanism which will not be available in the processor.

I think we discussed the possibility of adding persistency to the batch processor as well. Admittedly it may result in awkward user experience though since now you need to configure persistency in 2 different places.

[tigrannajaryan]

Another benefit of eliminating batch processor is that it is one less thing to configure. This will simplify the user experience.

[bogdandrutu]

While responding to your comments I realize that a possible thing is to offer both, do you think that is too confusing?

Dedicated batch processor keeps the batch in memory once even when there are multiple exporters in the pipeline. If we move the equivalently sized batch to queuedretry of every exporter we will slightly increase memory requirements (not by much since most of pdata is going to be shared assuming exporters' output throughput more or less keeps up with the input).

This is a good argument to possibly keep the processor as well, maybe using the same shared library that the exporterhelper uses. This comes with another downside that I haven't thought, if we want to have a fast batching we need to mutate the pdata (move them to the batch) which causes N (num exporter) copies needed even if the exporter may not need to mutate the data itself.

Virtually the same batching work needs to happen for every exporter, which adds some CPU work per exporter. It would be useful to benchmark to see how much this difference is when there is more than one exporter.

Indeed, I expect thought the work to be minimal, but definitely need to think about.

Batching probably helps to improve the performance of subsequent processors in the pipeline. We haven't benchmarked this, but it is possible that there is non-zero per-pdata cost that is amortized better if the batches are larger. This may be negligible, but I would like to see some benchmarks measuring it.

Need to think/propose of 1-2 components to benchmark for this, maybe the new "transform" processor since will be one of the most used? Any other idea?

[tigrannajaryan]

While responding to your comments I realize that a possible thing is to offer both, do you think that is too confusing?

Yes, I think it will be confusing if we do batching in 2 places, with 2 different overlapping sets of configuration options available.

[tigrannajaryan]

if the exporter may not need to mutate the data itself.

The exporters are currently not allowed to mutate data: https://github.com/open-telemetry/opentelemetry-collector/tree/main/exporter#data-ownership

[tigrannajaryan]

Need to think/propose of 1-2 components to benchmark for this, maybe the new "transform" processor since will be one of the most used? Any other idea?

Since "transform" doesn't exist yet perhaps test with a couple basic ones for now, like attributesprocessor or filterprocessor.

[bogdandrutu]

The exporters are currently not allowed to mutate data: https://github.com/open-telemetry/opentelemetry-collector/tree/main/exporter#data-ownership

you know that nobody reads documentation :)) One of them is the signalfx exporter which uses https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/pkg/batchperresourceattr/batchperresourceattr.go. Luckily after my change to move "capability" to Consumer, we now do the right thing for exporters because they also have capabilities and we clone if we need there as well.

So the documentation is out-dated unfortunately.

[mx-psi]

I think supporting both would be confusing. The 'customization of the "size" function' feature would be really helpful for us: we want batches to be below our intake's API max size, but currently we can't really give a precise number for the max batch size, since what we care about is the number of bytes in the payload we export, which can't be recovered from the number of points in a batch.

[bogdandrutu]

@dashpole would be good if you drive this to resolution, see open-telemetry/opentelemetry-collector-contrib#7134 which you also need :)

[pmm-sumo]

Batching probably helps to improve the performance of subsequent processors in the pipeline. We haven't benchmarked this, but it is possible that there is non-zero per-pdata cost that is amortized better if the batches are larger. This may be negligible, but I would like to see some benchmarks measuring it.

There's an edge case in which batch + groupbyattrs can be used for compacting traces and has a huge impact (as measured by @pkositsyn). Moving batching to exporterhelper would make it no longer possible but I think we could also consider making it a more generic helper, which could be used in processors as well?

[swiatekm]

1. Batch processor uses a queue (implemented as a channel), which is not the same as the one used in the queue retry, that can lose data when the binary crashes even after the client was informed that data were accepted. There is already work to offer a persistent queue mechanism which will not be available in the processor.

Correct me if I'm wrong, but we can lose data this way even without a crash, if we have an exporter with a queue and a batch processor, the data can be dropped due to the exporter queue being full, and the batch processor makes it impossible to propagate the error back to the receiver.

I don't know that I have an informed opinion on whether batching should happen in a separate processor or in an exporter, but it definitely shouldn't happen in both, for the above reason, unless we're willing to accept this kind of data loss and abandon any hope of applying backpressure to the receiver from any component after the batch processor.

[dashpole]

For now, we've implemented batching within the gcp exporter. There is only one "correct" way to batch for GCM, so it seemed sensible to make it work correctly without requiring configuration. It is also needed to work around other quirks of the GCM api, such as #4442. open-telemetry/opentelemetry-collector-contrib#7134 isn't blocking us--we've implemented our conversion in our exporter. That proposal would only be valuable if other exporters that need summary splitting also wanted to use it.

I agree with others that supporting both processor and exporterhelper would be confusing. For now, I think it is reasonable to ask unusual use-cases like ours to re-implement batching themselves. Making it available as a library would be nice, but probably not worth the dependency unless we wanted to mark it stable.