[exporter/awss3exporter] Add the retry mode, max attempts and max backoff settings#39509
Conversation
namco1992
force-pushed
the
mengnan/add-retry-awss3
branch
from
4dd798f to
528dfe0
Compare
|
Please add default values to default config in createDefaultConfig in factory.go |
| | `sending_queue` | [exporters common queuing](https://github.com/open-telemetry/opentelemetry-collector/blob/main/exporter/exporterhelper/README.md) | disabled | | ||
| | `timeout` | [exporters common timeout](https://github.com/open-telemetry/opentelemetry-collector/blob/main/exporter/exporterhelper/README.md) | 5s | | ||
|
|
||
| | `retry_mode` | The retryer implementation, the supported values are "standard"(default) and "adaptive"(experimental) | none | |
There was a problem hiding this comment.
| | `retry_mode` | The retryer implementation, the supported values are "standard"(default) and "adaptive"(experimental) | none | | |
| | `retry_mode` | The retryer implementation, the supported values are "standard"(default) and "adaptive"(experimental) | default | |
|
Moving to draft while you review my comments, please mark ready for review once they are addressed. |
@atoulme Thanks for the review! For the default values, it's controlled by the SDK, which means if nothing is configured, it will by default have a Do you prefer we duplicate the work in the |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
Just add default values to the README if possible or point to the AWS SDK docs, up to you. Please take a look at the conflicts. |
|
Hey @namco1992, any update on this, also looking for this addition |
|
Hi @akats7, sorry for the delay, will try to address all the comments later today! |
…koff to the settings The AWS S3 client comes with a "standard" retryer implementation enabled by default. Explicitly mention it in the documentation to avoid confusion. Also expose the retry settings so the user can have control. Signed-off-by: Mengnan Gong <namco1992@gmail.com>
namco1992
force-pushed
the
mengnan/add-retry-awss3
branch
from
528dfe0 to
821ebfb
Compare
| DisableHTTPS: conf.S3Uploader.DisableSSL, | ||
| } | ||
| o.UsePathStyle = conf.S3Uploader.S3ForcePathStyle | ||
| o.Retryer = retry.AddWithMaxAttempts(o.Retryer, conf.S3Uploader.RetryMaxAttempts) |
There was a problem hiding this comment.
The SDK provides too many ways to config the same thing, but it seems this util wrapper func is the only way to set the max attempts to 0 and allow unlimited retries. Init the retrier with NewStandard or overwrite StandardOptions actually doesn't allow setting MaxAttempts to 0: https://github.com/aws/aws-sdk-go-v2/blob/c1c1cda2978dcef4de6db7747e2f427105023cde/aws/retry/standard.go#L186-L188.
I even doubted if the documentation was lying about it, but it turns out setting MaxAttempts to 0 indeed allows unlimited retries: https://github.com/aws/aws-sdk-go-v2/blob/c1c1cda2978dcef4de6db7747e2f427105023cde/aws/retry/middleware.go#L248-L255
I find it quite confusing and would like a fresh pair of eyes to double check.
There was a problem hiding this comment.
This matches my expectations. To be fair, this would work with a negative value as well, which might seem less confusing.
…koff settings (open-telemetry#39509) <!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description The queue and timeout settings have been added in open-telemetry#36264. However, there is no retry settings added. Initially I thought we should rely on the standard `configretry.BackOffConfig`, and then realized that the AWS S3 client has a "standard" retryer implementation enabled by default. It also handles the definition of retryable errors https://pkg.go.dev/github.com/aws/aws-sdk-go-v2/aws/retry#hdr-Standard, which makes more sense to leverage the AWS SDK instead of shoehorning it to the standard `configretry.BackOffConfig`. Explicitly mention the default retry mode in the documentation to avoid confusion. Also expose the retry settings so the user can have control. <!-- Issue number (e.g. open-telemetry#1234) or full URL to issue, if applicable. --> #### Link to tracking issue Mentioned in open-telemetry#36264. <!--Describe what testing was performed and which tests were added.--> #### Testing <!--Describe the documentation added.--> #### Documentation Updated the exporter documentation to include the retry settings. Signed-off-by: Mengnan Gong <namco1992@gmail.com>
* chore: bump otel to 127 * workaround open-telemetry/opentelemetry-collector#13117 * fix pipeline.MustNewIDWithName deprecation warnings * fix awss3 exporter tests open-telemetry/opentelemetry-collector-contrib#39509 * otlphttpexporter debugexporter
* chore: bump otel to 127 * workaround open-telemetry/opentelemetry-collector#13117 * fix pipeline.MustNewIDWithName deprecation warnings * fix awss3 exporter tests open-telemetry/opentelemetry-collector-contrib#39509 * otlphttpexporter debugexporter * tmp * fix more tests * fix TestOTelConfig/empty_receiver_config * rm outdated tests * fix build * fix nil deref in test * fix nil deref in test * revert test change * changelog * bump beyla * update index.md.t * review fixes * lint
…koff settings (open-telemetry#39509) <!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description The queue and timeout settings have been added in open-telemetry#36264. However, there is no retry settings added. Initially I thought we should rely on the standard `configretry.BackOffConfig`, and then realized that the AWS S3 client has a "standard" retryer implementation enabled by default. It also handles the definition of retryable errors https://pkg.go.dev/github.com/aws/aws-sdk-go-v2/aws/retry#hdr-Standard, which makes more sense to leverage the AWS SDK instead of shoehorning it to the standard `configretry.BackOffConfig`. Explicitly mention the default retry mode in the documentation to avoid confusion. Also expose the retry settings so the user can have control. <!-- Issue number (e.g. #1234) or full URL to issue, if applicable. --> #### Link to tracking issue Mentioned in open-telemetry#36264. <!--Describe what testing was performed and which tests were added.--> #### Testing <!--Describe the documentation added.--> #### Documentation Updated the exporter documentation to include the retry settings. Signed-off-by: Mengnan Gong <namco1992@gmail.com>
5 participants
Description
The queue and timeout settings have been added in #36264. However, there is no retry settings added. Initially I thought we should rely on the standard
configretry.BackOffConfig, and then realized that the AWS S3 client has a "standard" retryer implementation enabled by default. It also handles the definition of retryable errors https://pkg.go.dev/github.com/aws/aws-sdk-go-v2/aws/retry#hdr-Standard, which makes more sense to leverage the AWS SDK instead of shoehorning it to the standardconfigretry.BackOffConfig.Explicitly mention the default retry mode in the documentation to avoid confusion. Also expose the retry settings so the user can have control.
Link to tracking issue
Mentioned in #36264.
Testing
Documentation
Updated the exporter documentation to include the retry settings.