Remove batcher and related config in favor of sending queue

[lahsivjar]

Description

Removes the deprecated batcher configuration and makes sending_queue as default. To adhere to the previous behaviour, the sending queue defaults are kept as close to the current defaults as possible. The PR also drops asyncBulkIndexer and related code, a future PR will follow to refactor the code around bulk indexer sessions.

Link to tracking issue

Part of #42718

Testing

Updated

Documentation

Updated

[carsonip]

Did a first pass. Thanks for taking on this big task!

[carsonip]

++. 2 concerns here:

• block_on_overflow: I incline to have it as true so that the default es exporter behavior is not too breaking although both true and false come with different problems. You may easily convince me the other way around.
• wait_for_result: I believe the default is true and that is desired. But it is not our previous default and this behavior change should be clearly stated in the changelog

[lahsivjar]

I believe the default is true and that is desired

wait_for_result defaults to false, not true - this is close to what we currently have.

block_on_overflow: I incline to have it as true so that the default es exporter behavior is not too breaking although both true and false come with different problems. You may easily convince me the other way around.

I am in favor of true for block_on_overflow - I believe it is a better default to start with but it will deviate from the default queue configurations.

[carsonip]

wait_for_result defaults to false, not true

Is that the desired end state of the defaults though? Or is it an intermediate state to approximate the previous async behavior? This will have a significant impact on latency and throughput of small serial requests

[lahsivjar]

Is that the desired end state of the defaults though?

Yes, I think wait_for_result is a specialized config and we should not turn it on by default.

This will have a significant impact on latency and throughput of small serial requests

Agreed, wait_for_result set to true will have a significant impact on latency and throughput. That is why I have kept it to false and that is also the reason why I think false is a better default for this configuration option.

[lahsivjar]

@vigneshshanmugam What do you think about the block_on_overflow config? I am slightly inclined towards keeping block_on_overflow to be true as the recommended ES exporter defaults.

[lahsivjar]

I have updated the code to use block_on_overflow set to true as the default based on the above discussion so far. Happy to discuss it further if reviewers feel this to be not a good default.

[lahsivjar]

Also see: open-telemetry/opentelemetry-collector#13847

[vigneshshanmugam]

++ to keeping it as true as its a better default.

[carsonip]

the changes look good. Would love @axw 's eyes as well.

[carsonip]

thanks for covering the max flush bytes case.

nit: there's an outdated comment on async session if you grep for "async"

	// The behavior of Flush depends on whether the bulk indexer is
	// synchronous or asynchronous. Calling Flush on an asynchronous bulk
	// indexer session is effectively a no-op; flushing will be done in
	// the background. Calling Flush on a synchronous bulk indexer session
	// will wait for bulk indexing of added documents to complete,
	// successfully or not.

[vigneshshanmugam]

Batching defaults looks great, Thanks @lahsivjar LGTM

[axw]

Thanks @lahsivjar!

[atoulme]

Please address the conflict.

[lahsivjar]

@atoulme Conflicts are addressed.