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

[observiqexporter] Allow Dialer timeout to be configured #5906

Merged

Conversation

BinaryFissionGames
Copy link
Contributor

@BinaryFissionGames BinaryFissionGames commented Oct 25, 2021

Description:

  • Added in configuration options for some more specific timeouts (dialer & TLS handshake timeouts). This allows finer-grained control of request timeouts, in general.
  • Bumped default timeout for overall request from 10 seconds to 20 seconds
  • Modified some comments on the config; They aren't really "optional" in the programmatic API, only for a file-based configuration.

Link to tracking Issue: N/A

Testing:

  • Unit tests for configuration parsing modified to test new configuration values

Documentation:

  • Added/updated README documentation for defaults and new config options.

exporter/observiqexporter/client.go Outdated Show resolved Hide resolved
exporter/observiqexporter/config.go Show resolved Hide resolved
exporter/observiqexporter/config.go Outdated Show resolved Hide resolved
@BinaryFissionGames BinaryFissionGames changed the title [observiqexporter] Allow Dialer and TLS handshake timeout to be configured [observiqexporter] Allow Dialer timeout to be configured Nov 1, 2021
Copy link
Member

@djaglowski djaglowski left a comment

Choose a reason for hiding this comment

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

LGTM - The prometheus test that is failing appears to be unrelated.

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.

Thanks for the contribution.

It looks like the observiqexporter component doesnt curren't have an owner in the CODEOWNERS, any chance you'd be interested in being one @BinaryFissionGames? It appears you have the most contributions to it.

exporter/observiqexporter/config.go Outdated Show resolved Hide resolved
@BinaryFissionGames
Copy link
Contributor Author

Yeah, it's a good idea for me to be in the codeowners for observiqexporter; I'll open a separate PR for that. Thanks @codeboten !

@codeboten codeboten merged commit a934ddc into open-telemetry:main Nov 19, 2021
povilasv referenced this pull request in coralogix/opentelemetry-collector-contrib Dec 19, 2022
Bump go.uber.org/zap from 1.21.0 to 1.22.0 in /cmd/builder
Bump go.uber.org/zap from 1.21.0 to 1.22.0
Bump go.uber.org/atomic from 1.9.0 to 1.10.0

Co-authored-by: bogdandrutu <[email protected]>
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.

None yet

4 participants