-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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.
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
[exporter/prometheusremotewriteexporter] Retry on 5xx status codes using cenkalti/backoff
client
#23842
Conversation
go-retryablehttp
client
go-retryablehttp
clientgo-retryablehttp
client
1cc170d
to
a03bc9f
Compare
Do you want to remove |
I would leave the setting for now and can take the follow up on this. As I don't want to disable retries in this PR. |
@vasireddy99 I think what @dmitryax suggested is reasonable and valid to include in the scope of this PR: we never return a retryable error and this component cannot handle retries in the helper level, therefore we should remove this capability of this component here: https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/exporter/prometheusremotewriteexporter/factory.go#L60 |
Makes sense, I agree with that to, but wanted to discuss and also double nthe usage. Will commit the changes |
Commited the changes. |
@vasireddy99, doesn't seem like anything new was pushed, just merged main |
Thanks @dmitryax, Please review |
This PR #24729 shall also be review/merged after this PR is merged. Just adding the note. |
Codecov ReportPatch coverage is
📢 Thoughts on this report? Let us know!. |
Description: Fix the Retry on 5xx status code using
cenkalti/backoff
package and added unit test.Thanks to @rapphil for proposing the solution.
Link to tracking Issue: #20304
Testing: Added unit tests for retryOn5xx and noRetryOn4xx
Prometheus compliance test results -