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

[confighttp] Add option to disable HTTP keep-alives #8274

Merged

Conversation

kasia-kujawa
Copy link
Contributor

Description:

Add option to disable HTTP keep-alives which helps in better load balancing in k8s clusters when otlphttp exports are used to send logs to otlp receivers

Link to tracking Issue: #8260

@kasia-kujawa kasia-kujawa requested review from a team and bogdandrutu August 23, 2023 14:04
@atoulme
Copy link
Contributor

atoulme commented Aug 23, 2023

Please add a changelog entry.

@codecov
Copy link

codecov bot commented Aug 23, 2023

Codecov Report

Patch coverage is 100.00% of modified lines.

Files Changed Coverage
config/confighttp/confighttp.go 100.00%

📢 Thoughts on this report? Let us know!.

@kasia-kujawa kasia-kujawa force-pushed the kkujawa_disable_keep_alives branch 2 times, most recently from e154a06 to 80f69ad Compare August 25, 2023 09:01
@kasia-kujawa
Copy link
Contributor Author

@bogdandrutu Could you take a look at this pull request?

Copy link
Member

@andrzej-stencel andrzej-stencel left a comment

Choose a reason for hiding this comment

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

This makes sense to me, the change seems pretty straightforward too.

Copy link
Contributor

@atoulme atoulme left a comment

Choose a reason for hiding this comment

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

LGTM. Please rebase off main to fix the conflict.

@kasia-kujawa kasia-kujawa force-pushed the kkujawa_disable_keep_alives branch 2 times, most recently from 6ef9dc4 to bffd17c Compare September 4, 2023 12:58
@jpkrohling
Copy link
Member

@open-telemetry/collector-maintainers, I believe this is ready to be merged.

@jpkrohling jpkrohling added the ready-to-merge Code review completed; ready to merge by maintainers label Sep 5, 2023
@kasia-kujawa kasia-kujawa force-pushed the kkujawa_disable_keep_alives branch from bffd17c to 50d445f Compare September 5, 2023 17:54
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.

@Aneurysm9 please take a look, the warning has been added.

config/confighttp/confighttp.go Show resolved Hide resolved
@Aneurysm9
Copy link
Member

@Aneurysm9 please take a look, the warning has been added.

I would prefer not to add this configuration option at all, but don't feel strongly enough to block this PR, so I'll simply decline to approve it.

I'm not sure that logging a warning when this option is enabled would be more than noise. Hopefully someone enabling this option has a better understanding of their environment than we do and understands what they're buying.

@codeboten codeboten merged commit 8195e46 into open-telemetry:main Sep 7, 2023
@github-actions github-actions bot added this to the next release milestone Sep 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge Code review completed; ready to merge by maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants