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

(exporterhelper) Configuration package for timeout sender #11387

Closed
wants to merge 17 commits into from

Conversation

jmacd
Copy link
Contributor

@jmacd jmacd commented Oct 7, 2024

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.

Copy link

codecov bot commented Oct 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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     

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

Copy link
Member

@mwear mwear left a comment

Choose a reason for hiding this comment

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

Any chance we can add a few test cases for Validate to appease codecov?

@jmacd jmacd changed the title Configuration package for timeout sender (exporterhelper) Configuration package for timeout sender Oct 7, 2024
Copy link
Member

@mwear mwear left a comment

Choose a reason for hiding this comment

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

LGTM. Noting that the contrib test failure is unrelated to this change (TestFilterMetricProcessorTelemetry).

@mx-psi mx-psi requested a review from dmitryax October 10, 2024 15:56
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

The code looks OK to me.

Is this package configuration that will apply to components outside of exporters?

  • If yes, the documentation for this package could be made more generic to clarify how it applies to different component types.
  • If not I would expect it to live in the exporter helper instead of its own config package.

I took a brief look through other uses of timeout through the core codebase and I'm not sure if the Policy concept would apply in other places.

@jmacd
Copy link
Contributor Author

jmacd commented Oct 10, 2024

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!

@codeboten
Copy link
Contributor

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.

@bogdandrutu
Copy link
Member

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.

@codeboten
Copy link
Contributor

@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

@jmacd
Copy link
Contributor Author

jmacd commented Oct 30, 2024

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

@jmacd jmacd closed this Oct 30, 2024
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.

5 participants