Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[exporter/elasticsearch] Make retry statuses configurable and default to 429 only #32584

Closed
carsonip opened this issue Apr 22, 2024 · 6 comments · Fixed by #34684
Closed

[exporter/elasticsearch] Make retry statuses configurable and default to 429 only #32584

carsonip opened this issue Apr 22, 2024 · 6 comments · Fixed by #34684
Labels
enhancement New feature or request exporter/elasticsearch

Comments

@carsonip
Copy link
Contributor

carsonip commented Apr 22, 2024 •
edited
Loading

Component(s)

exporter/elasticsearch

Is your feature request related to a problem? Please describe.

Elasticsearch exporter retries on 500, 502, 503, 504, 429 by default, on both request level and document level. This is not ideal and may lead to duplicates.

Describe the solution you'd like

The retriable statuses should be configurable such that users may decide whether they prefer guaranteed delivery or no duplicates. It should default to retrying on 429 only.

Additional context

Integration tests need to be updated since it is testing with document level 500s now.
@carsonip carsonip added enhancement New feature or request needs triage New item requiring triage labels Apr 22, 2024
@github-actions GitHub Actions
Copy link
Contributor

Pinging code owners:

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

@carsonip carsonip mentioned this issue Apr 22, 2024
Merged
@crobert-1
Copy link
Member

It looks like the current thought, from what I'm gathering in the PR discussion, is that the default will stay the same for now, with warnings that functionality will change in the near future. A code owner was involved in this discussion and has signed off, so I'm removing needs triage.

@crobert-1 crobert-1 removed the needs triage New item requiring triage label Apr 23, 2024
@carsonip
Copy link
Contributor Author

#32585 will add the config option and another PR in the future will change the default.

codeboten added a commit that referenced this issue May 1, 2024
@carsonip @lahsivjar @codeboten
[exporter/elasticsearch] Add retry.retry_on_status config (#32585)
7a19fb3 
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.

Part of #32584

---------

Co-authored-by: Vishal Raj <vishal.raj@elastic.co>
Co-authored-by: Alex Boten <223565+codeboten@users.noreply.github.com>
@github-actions GitHub Actions
Copy link
Contributor

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:

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

@github-actions github-actions bot added the Stale label Jun 24, 2024
@crobert-1 crobert-1 removed the Stale label Jun 27, 2024
@felixbarny
Copy link
Contributor

@carsonip as #32585 has been merged, can this be closed or is there more to do?

@crobert-1 crobert-1 mentioned this issue Aug 14, 2024
Merged
@carsonip
Copy link
Contributor Author

@carsonip as #32585 has been merged, can this be closed or is there more to do?

It will be closed via #34684

andrzej-stencel pushed a commit that referenced this issue Aug 23, 2024
@carsonip
[exporter/elasticsearch] Change default retry.retry_to_status to [429] (
a19aa30 
#34684)

**Description:** <Describe what has changed.>
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
Breaking change to change default retry_on_status to 429

**Link to tracking Issue:** <Issue number if applicable>
Fixes
#32584

**Testing:** <Describe what testing was performed and which tests were
added.>

**Documentation:** <Describe the documentation added.>
f7o pushed a commit to f7o/opentelemetry-collector-contrib that referenced this issue Sep 12, 2024
@carsonip @f7o
[exporter/elasticsearch] Change default retry.retry_to_status to [429] (
9aaa3ed 
open-telemetry#34684)

**Description:** <Describe what has changed.>
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
Breaking change to change default retry_on_status to 429

**Link to tracking Issue:** <Issue number if applicable>
Fixes
open-telemetry#32584

**Testing:** <Describe what testing was performed and which tests were
added.>

**Documentation:** <Describe the documentation added.>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement New feature or request exporter/elasticsearch
Projects
None yet
Milestone
No milestone
3 participants
@felixbarny @carsonip @crobert-1