[exporter/elasticsearch] Add retry.retry_on_status config

[carsonip]

Description:

Previously, the status codes that trigger retries were hardcoded to be 429, 500, 502, 503, 504.
It is now configurable using retry.retry_on_status, and defaults to [429, 500, 502, 503, 504] to avoid a breaking change.
To avoid duplicates, it is recommended to configure retry.retry_on_status to [429], which would be the default in a future version.

Link to tracking Issue: Part of #32584

Testing: Updated tests

Documentation: Updated readme

[lahsivjar]

LGTM! Some minor comments.

[lahsivjar]

On a seperate note, should EnableRetryOnTimeout be equal to !retryDisabled?

[carsonip]

I checked the v7 estransport and EnableRetryOnTimeout usage is wrapped inside MaxRetries. Therefore, in practice, changing it to !retryDisabled will have no effect. But I agree that it will be clearer to do so. Let me make the change.

[carsonip]

hmm, I've changed my mind. The default in go-elasticsearch for DisableRetry and EnableRetryOnTimeout are both false, which makes sense. As a user I would not expect the client to retry the bulk request on timeout. Let me separate this discussion into an issue, whether the existing setting to enable retry on timeout to config.Retry.Enabled is desirable.

[crobert-1]

Since this is a new feature on the component, it would be good to have code owner feedback in some form as well 👍

[ycombinator]

With this proposed breaking change, I'm a little concerned about existing users who are expecting retries on 500, 502, 503, 504, and 429 status codes. If these users are expecting retries for 500, 502, 503, and 504, those will no longer happen with this change (unless they explicitly configure these statuses in the new config) — does that mean they'll lose any docs that were part of that bulk request for these statuses by default?

That being said, I do like the idea of making the retryable statuses configurable with a less "aggressive" default of only 429 status responses being retried in the long run. So, is there some way we could introduce this change in a more gradual way? One way I can think of is something like this:

• Introduce the new retry.retry_on_status config setting, defaulting it to the current set of statuses: 429, 500, 502, 503, 504.
• Document very explicitly that this setting's default will change in $some_specific_future_version to just 429.
• Then change the default to just 429 in that $some_specific_future_version.

[carsonip]

Introduce the new retry.retry_on_status config setting, defaulting it to the current set of statuses: 429, 500, 502, 503, 504.

done

Document very explicitly that this setting's default will change in $some_specific_future_version to just 429.
Then change the default to just 429 in that $some_specific_future_version.

Since we don't control the release cycle, it is hard to say for sure which change will get into which version. For now, the changelog only says "the future"

[carsonip]

@crobert-1 I believe this should be ready to merge.

[JaredTan95]

Everything looks good, BTW, thepushDocuments function is loaded with too many arguments now. Hopefully, it will shrink in the future :-P.