Require exporter timeouts to be positive

[jack-berg]

Resolves #6245.

This is a behavior change, which I would consider a bug. The behavior when exporter timeout 0 is at best ambiguous, and at worst non-functional. As discussed in this comment:

There's different interpretations of 0 for different http clients:

• okhttp allows you to set the connect timeout to 0 with "no timeout" semantics.
• jdk http client throws if you set connect timeout to 0

I figured we get around this by only accepting positive values. If a user wants forever semantics, they can se timeout to be Long.MAX_VALUE.

Configuration options impacted:

• OtlpHttpSpanExporterBuilder#setTimeout
• OtlpHttpSpanExporterBuilder#setConnectTimeout
• OtlpHttpMetricExporterBuilder#setTimeout
• OtlpHttpMetricExporterBuilder#setConnectTimeout
• OtlpHttpLogRecordExporterBuilder#setTimeout
• OtlpHttpLogRecordExporterBuilder#setConnectTimeout
• OtlpGrpcSpanExporterBuilder#setTimeout
• OtlpGrpcSpanExporterBuilder#setConnectTimeout
• OtlpGrpcMetricExporterBuilder#setTimeout
• OtlpGrpcMetricExporterBuilder#setConnectTimeout
• OtlpGrpcLogRecordExporterBuilder#setTimeout
• OtlpGrpcLogRecordExporterBuilder#setConnectTimeout
• BatchLogRecordProcessorBuilder#setExporterTimeout
• BatchSpanProcessorBuilder#setExporterTimeout

[codecov]

Codecov Report

Attention: Patch coverage is 80.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 90.28%. Comparing base (a6b3302) to head (5ad8be7).

Files with missing lines	Patch %	Lines
...er/otlp/logs/OtlpGrpcLogRecordExporterBuilder.java	50.00%	0 Missing and 1 partial ⚠️
...er/otlp/metrics/OtlpGrpcMetricExporterBuilder.java	50.00%	0 Missing and 1 partial ⚠️
...porter/otlp/trace/OtlpGrpcSpanExporterBuilder.java	50.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##               main    #6850   +/-   ##
=========================================
  Coverage     90.28%   90.28%           
  Complexity     6588     6588           
=========================================
  Files           729      729           
  Lines         19768    19768           
  Branches       1944     1944           
=========================================
+ Hits          17847    17848    +1     
  Misses         1327     1327           
+ Partials        594      593    -1     

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

[trask]

to make sure we're interpreting "timeout zero" consistently, would you support a similar spec clarification to

• OTEL_EXPORTER_OTLP_TIMEOUT
• OTEL_EXPORTER_ZIPKIN_TIMEOUT
• OTEL_BSP_EXPORT_TIMEOUT
• OTEL_BLRP_EXPORT_TIMEOUT
• OTEL_METRIC_EXPORT_TIMEOUT

[jack-berg]

to make sure we're interpreting "timeout zero" consistently, would you support a similar spec clarification to

Yes, that's a good call. But I wonder if there's a scenario where the outcome of a spec PR would influence what we do.

• If there's not consistency on whether timeout of 0 is valid or invalid and spec rejects any attempt to clarify, then I think we're ok to interpret it as invalid in java.
• If there's consistency that timeout of 0 is invalid and that clarifying is a bug fix, then we're ok with this PR in java.

I think the only situation where we wouldn't want to this PR in java would be: There is spec consensus that timeout of 0 conveys "indefinite". Seems hard to believe that all the implementations interpret it this way.

[jack-berg]

Spec issue open-telemetry/opentelemetry-specification#4283