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

feat: make collector health check timeout configurable #1371

Merged
merged 4 commits into from
Oct 10, 2024

Conversation

VinozzZ
Copy link
Contributor

@VinozzZ VinozzZ commented Oct 9, 2024

Which problem is this PR solving?

When there's a big volume of traffic, collector can take longer to respond to the health check

Short description of the changes

  • add a new config option HealthCheckTimeout in Collection

@VinozzZ VinozzZ requested a review from a team as a code owner October 9, 2024 22:05
@VinozzZ VinozzZ force-pushed the yingrong.make_collect_healthcheck_configurable branch from 9507863 to 656ee72 Compare October 9, 2024 22:08
@VinozzZ VinozzZ self-assigned this Oct 9, 2024
@VinozzZ VinozzZ changed the base branch from main to 2.8.3-release October 9, 2024 22:09
Copy link
Contributor

@TylerHelmuth TylerHelmuth left a comment

Choose a reason for hiding this comment

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

Left suggestions to update wording to specify this only applies to the collection health check subsystems.

config/metadata/configMeta.yaml Outdated Show resolved Hide resolved
config/metadata/configMeta.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

@MikeGoldsmith MikeGoldsmith left a comment

Choose a reason for hiding this comment

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

Making the health check timeout configurable is good. I've left a query regarding how the new metric is recorded.

collect/collect.go Outdated Show resolved Hide resolved
@MikeGoldsmith
Copy link
Contributor

I'd also suggest updating the target branch to main, then we can cherry-pick it back into the release branch.

@VinozzZ VinozzZ changed the base branch from 2.8.3-release to main October 10, 2024 14:19
@VinozzZ VinozzZ force-pushed the yingrong.make_collect_healthcheck_configurable branch from cc6c60c to 752475c Compare October 10, 2024 14:22
@VinozzZ VinozzZ requested a review from MikeGoldsmith October 10, 2024 14:24
Copy link
Contributor

@MikeGoldsmith MikeGoldsmith left a comment

Choose a reason for hiding this comment

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

Looks great, thanks @VinozzZ 🎉

@VinozzZ VinozzZ merged commit 7bdc0a7 into main Oct 10, 2024
5 checks passed
@VinozzZ VinozzZ deleted the yingrong.make_collect_healthcheck_configurable branch October 10, 2024 14:45
MikeGoldsmith pushed a commit that referenced this pull request Oct 10, 2024
## Which problem is this PR solving?

When there's a big volume of traffic, collector can take longer to
respond to the health check

## Short description of the changes

- add a new config option `HealthCheckTimeout` in `Collection`

---------

Co-authored-by: Tyler Helmuth <[email protected]>
# Conflicts:
#	config/metadata/configMeta.yaml
TylerHelmuth added a commit that referenced this pull request Oct 16, 2024
When there's a big volume of traffic, collector can take longer to
respond to the health check

- add a new config option `HealthCheckTimeout` in `Collection`

---------

Co-authored-by: Tyler Helmuth <[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.

3 participants