[exporter/elasticsearch] Clarify and decide retry.max_requests behavior

[carsonip]

Component(s)

exporter/elasticsearch

What happened?

Description

Discovered while working on https://github.com/open-telemetry/opentelemetry-collector-contrib/pull/32323

Docs (code comments, README) do not clearly reflect the meaning of retry.max_requests, as in practice it counts the initial publishing attempt as well. Therefore, max_requests=1 means retry is disabled, which is a little counter intuitive.

We should either fix the documentation to align with implementation, or fix the implementation to make it more intuitive. The issue with fixing the implementation is that it is a breaking change for existing users.

Either way, docs need to be updated such that it is clear that the same retry value is used for both HTTP retries and per-document retries, and as a result the total attempts can be greater than max_requests.

Steps to Reproduce

set retry.max_requests=1, get a per-document 429 from ES

Expected Result

the document is retried once

Actual Result

the document is not retried

Collector version

0.97.0

Environment information

No response

OpenTelemetry Collector configuration

No response

Log output

No response

Additional context

No response

[github-actions[bot]]

Pinging code owners:

• exporter/elasticsearch: @JaredTan95 @ycombinator

See Adding Labels via Comments if you do not have permissions to add labels yourself.

[JaredTan95]

Nice catch. Would you be interested in trying to clarify it?

[carsonip]

The scope of this issue is larger than a documentation fix.

Ideally I would like to either

• modify the existing retry.max_requests behavior, which is a "breaking change" if existing users rely on the behavior. But if it is seen as a bug given the unintuitive behavior and inconsistency with docs, then it can be called a bugfix.
• (the complete solution) deprecate retry.max_requests and introduce e.g. retry.max_retries with a more intuitive behavior

Either way, docs will need to be updated.

Would like the community's and the maintainers' @JaredTan95 @ycombinator views on this

[ycombinator]

(the complete solution) deprecate retry.max_requests and introduce e.g. retry.max_retries with a more intuitive behavior

I'm in favor of this option. @JaredTan95 WDYT?

[github-actions[bot]]

This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.

Pinging code owners:

• exporter/elasticsearch: @JaredTan95 @ycombinator @carsonip

See Adding Labels via Comments if you do not have permissions to add labels yourself.

[andrzej-stencel]

I :100: agree that deprecating max_requests (without changing its behavior) and introducing a new option max_retries is the way to go. The name max_requests would be misleading even if we added documentation on it. The name max_retries is much better as even without documentation it is clear how it works.

[github-actions[bot]]

This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.

Pinging code owners:

• exporter/elasticsearch: @JaredTan95 @ycombinator @carsonip

See Adding Labels via Comments if you do not have permissions to add labels yourself.