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

clarify interval and timeout for http health check #5071

Closed
wants to merge 3 commits into from

Conversation

hannahhearth
Copy link
Contributor

Fixes #2977

See source code.

@hannahhearth hannahhearth requested a review from a team December 7, 2018 18:27
Copy link
Member

@banks banks left a comment

Choose a reason for hiding this comment

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

Slight wording change, but thanks this is clearer

website/source/docs/agent/checks.html.md Outdated Show resolved Hide resolved
website/source/docs/agent/checks.html.md Outdated Show resolved Hide resolved
banks and others added 2 commits December 7, 2018 15:50
Co-Authored-By: opihana <hana378@gmail.com>
@hannahhearth
Copy link
Contributor Author

Thanks @banks, how does it look now?

Copy link
Member

@banks banks left a comment

Choose a reason for hiding this comment

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

Great, thanks!

Copy link
Contributor

@pierresouchay pierresouchay left a comment

Choose a reason for hiding this comment

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

LGTM

@pearkes
Copy link
Contributor

pearkes commented Apr 4, 2019

I feel like we can get this merged, @opihana?

@valentin-krasontovitsch

Sorry to give my 2 cents, but I feel like the comment in the source code does not reflect the logic in the source code.

Let me quote the block in question (which conveniently is also linked above):

// For long (>10s) interval checks the http timeout is 10s, otherwise the
// timeout is the interval. This means that a check *should* return
// before the next check begins.
if c.Timeout > 0 && c.Timeout < c.Interval {
	c.httpClient.Timeout = c.Timeout
} else if c.Interval < 10*time.Second {
	c.httpClient.Timeout = c.Interval
}

The improved docs describe the source code, so I think it would make sense to correct the comment as well (or remove it - some people might say that that's why we have documentation in separate files, so we don't need [to maintain] comments like this) - I personally think it should just be scratched though, the logic is clear from the code.

@freddygv
Copy link
Contributor

We're going to change the behavior so that we use the user provided timeout rather than restricting it based on the interval.

See #6094

@freddygv freddygv closed this Jul 10, 2019
@kisunji kisunji deleted the docs-interval-http-check branch September 13, 2021 13:39
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.

interval and timeout for http health check
6 participants