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.

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

Add connectTimeout configuration option OtlpHttp{Signal}Exporters #5941

Merged

Conversation

jack-berg
Copy link
Member

Connect timeouts are common failure mode for OTLP exporters. Retrying when a connection error occurs can improve the chances of a successful export.

As of version 1.31.0, the JdkHttpSender has no connect timeout, and will use the entire timeout period trying to establish a connection. The OkHttpHttpSender has a static connect timeout of 10s. #5867 updates JdkhttpSender to have static connect timeout of 10s aligned with OkHttpHttpSender. However, if you want to enable retries and have a total timeout of < 10s, the export will spend the entire timeout trying to connect.

Allowing the connect timeout to be configurable allows exporters to trigger retries earlier and possibly succeed.

This PR adds new setConnectTimeout(..) methods to the OtlpHttp{Signal}ExporterBuilders. This concept should make sense for the OtlpGrpc{Signal}Exporters as well. I can add that in a followup if folks agree with the direction.

@jack-berg jack-berg requested a review from a team October 26, 2023 19:07
@codecov
Copy link

codecov bot commented Oct 26, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (f36cc92) 91.16% compared to head (c9e5a5d) 91.18%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #5941      +/-   ##
============================================
+ Coverage     91.16%   91.18%   +0.02%     
- Complexity     5666     5683      +17     
============================================
  Files           625      625              
  Lines         16618    16666      +48     
  Branches       1641     1648       +7     
============================================
+ Hits          15149    15197      +48     
+ Misses         1019     1018       -1     
- Partials        450      451       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jack-berg jack-berg added this to the 1.32.0 milestone Nov 3, 2023
@jack-berg jack-berg modified the milestones: 1.32.0, 1.33.0 Nov 13, 2023
Comment on lines +2 to +13
=== CLASS FILE FORMAT VERSION: 52.0 <- 52.0
+++ NEW METHOD: PUBLIC(+) io.opentelemetry.exporter.otlp.http.logs.OtlpHttpLogRecordExporterBuilder setConnectTimeout(long, java.util.concurrent.TimeUnit)
+++ NEW METHOD: PUBLIC(+) io.opentelemetry.exporter.otlp.http.logs.OtlpHttpLogRecordExporterBuilder setConnectTimeout(java.time.Duration)
*** MODIFIED CLASS: PUBLIC FINAL io.opentelemetry.exporter.otlp.http.metrics.OtlpHttpMetricExporterBuilder (not serializable)
=== CLASS FILE FORMAT VERSION: 52.0 <- 52.0
+++ NEW METHOD: PUBLIC(+) io.opentelemetry.exporter.otlp.http.metrics.OtlpHttpMetricExporterBuilder setConnectTimeout(long, java.util.concurrent.TimeUnit)
+++ NEW METHOD: PUBLIC(+) io.opentelemetry.exporter.otlp.http.metrics.OtlpHttpMetricExporterBuilder setConnectTimeout(java.time.Duration)
*** MODIFIED CLASS: PUBLIC FINAL io.opentelemetry.exporter.otlp.http.trace.OtlpHttpSpanExporterBuilder (not serializable)
=== CLASS FILE FORMAT VERSION: 52.0 <- 52.0
+++ NEW METHOD: PUBLIC(+) io.opentelemetry.exporter.otlp.http.trace.OtlpHttpSpanExporterBuilder setConnectTimeout(long, java.util.concurrent.TimeUnit)
+++ NEW METHOD: PUBLIC(+) io.opentelemetry.exporter.otlp.http.trace.OtlpHttpSpanExporterBuilder setConnectTimeout(java.time.Duration)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jack-berg jack-berg merged commit 902d68c into open-telemetry:main Dec 7, 2023
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants