opentelemetry-collector icon indicating copy to clipboard operation
opentelemetry-collector copied to clipboard

(exporterhelper) Configuration package for timeout sender

Open jmacd opened this issue 1 year ago • 5 comments

Description

Adds a timeout configuration struct, consisting of the current setting (timeout) and a proposed-new setting short_timeout_policy with three defined values: "sustain" (default), "ignore" (override), and "abort" (fail fast).

Link to tracking issue

Part of #11183.

Testing

See #11385 for integration with exporterhelper for more testing.

Documentation

See #11385 for integration with exporterhelper for proposed documentation.

jmacd avatar Oct 07 '24 20:10 jmacd

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 91.91%. Comparing base (9f9b86c) to head (d58a283).

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #11387      +/-   ##
==========================================
- Coverage   91.92%   91.91%   -0.01%     
==========================================
  Files         430      431       +1     
  Lines       20311    20325      +14     
==========================================
+ Hits        18671    18682      +11     
- Misses       1267     1269       +2     
- Partials      373      374       +1     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Oct 07 '24 20:10 codecov[bot]

If not I would expect it to live in the exporter helper instead of its own config package.

I was following configretry, which appears to be exporterhelper-specific but resides in /config. For one thing, I would like to be able to experiment with completely replacing exporterhelper. I should be able to replace exporterhelper w/ exporterhelper2 and re-use its configuration.

Say you're sure, and I'll move it to /exporter/exporterhelper!

jmacd avatar Oct 10 '24 20:10 jmacd

Say you're sure, and I'll move it to /exporter/exporterhelper!

I am not sure. configretry is a good example of this type of functionality existing in its own package.

codeboten avatar Oct 17 '24 22:10 codeboten

I was following configretry, which appears to be exporterhelper-specific but resides in /config. For one thing, I would like to be able to experiment with completely replacing exporterhelper. I should be able to replace exporterhelper w/ exporterhelper2 and re-use its configuration.

The "config" package is a more generic, why would I experiment in a more generic package than in a more specific. You are incorrect about configretry, we use that in few other places in contrib, and that was the reason to "promote" it to a generic config, and not for experimentation.

bogdandrutu avatar Oct 18 '24 15:10 bogdandrutu

@jmacd the configuration in TimeoutConfig is used in both exporters and receivers in contrib, can the first step to make it more usable generally be to move the existing functionality into configtimeout and then add the new setting? The first step would allow the packages that depend on it to move to using configtimeout

then the follow up PR can be to add short_timeout_policy

codeboten avatar Oct 18 '24 16:10 codeboten

If a processor -- say batch processor -- wants to respect timeouts, won't it need a configuration? I will submit an RFC.

jmacd avatar Oct 30 '24 21:10 jmacd