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

Add TCP checks on top of HTTP checks #47

Open
JoshuaOliphant opened this issue Jan 23, 2019 · 9 comments
Open

Add TCP checks on top of HTTP checks #47

JoshuaOliphant opened this issue Jan 23, 2019 · 9 comments
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed

Comments

@JoshuaOliphant
Copy link

I've been trying Goldpinger out recently, and have been finding it useful already. I was looking through the code, and was curious why you chose to do http monitoring, rather than tcp or icmp pings?

@seeker89
Copy link
Contributor

Hey, glad you are finding it useful !

We started with HTTP, because it was the most common use case at the time, and it tests the underlying layers too. From SRE perspective, it's probably more interesting to know HTTP connectivity goes through, rather than ICMP, but it of course depends on what you're doing.

Someone has suggested doing actual ICMP pings, and it's likely to appear at some point in time.

Feel free to contribute, it should be pretty straightforward to add as an option 👍

@ivkalita
Copy link
Contributor

I see one more advantage of using HTTP here. Goldpinger could be improved such a way to return some custom health-related metrics in the response, and HTTP (comparing to ICMP) makes it easier :)

@JoshuaOliphant
Copy link
Author

That all makes sense, thanks for the quick response. I was going to suggest timing the response when receiving the first byte, but if ICMP pings appear at some point anyway I suppose that won't matter. Plus maybe getting the timing after the whole response gives a more realistic idea of how the network is behaving at the http level?

@seeker89
Copy link
Contributor

Yup, yup and yup, respectively.

@pjperez
Copy link

pjperez commented Jan 29, 2019

Hi folks,

I understand the reasoning behind the HTTP checks, however it is mentioned the intention of supporting ICMPs in the future too. May I humbly suggest making TCP the next supported protocol instead?

TCP is often offloaded to hardware acceleration, whilst ICMP isn't (and sometimes blocked or deprioritised). In my opinion ICMP will not always give you a good view of your network health (it would also have less problems with things like asymmetric routing, giving you a wrong impression!).

TCP being offloaded certainly means HTTP is also offloaded (as it's TCP+ L7 payload), but on an HTTP check there could be a lot of moving parts depending on what you're querying (e.g. the HTTP check might trigger -and wait for- backend DB queries!). I've got the feeling HTTP is good for an overall health check of the infra at the app layer, but it does not help separating issues between network and application layers. TCP is perfect for that.

Disclaimer: I am not a dev by any stretch, but time permitting I might have a look at the code and see if that's something I can add myself. At first sight it does not seem like the product is ready to support more than one type of check though, so it may be a bit daunting for someone like me with limited coding skills.

@seeker89
Copy link
Contributor

OK, so maybe let's do it this way. We'll implement TCP checks as soon as this issue reaches 10 thumbs up. Fair ?

@pjperez
Copy link

pjperez commented Mar 15, 2019

Thumbs up to that! 👍 😄

@dannyk81
Copy link
Contributor

dannyk81 commented Aug 9, 2019

@seeker89 there you go, I'm number 10 😄

@seeker89 seeker89 added enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed labels Jan 10, 2020
@seeker89 seeker89 changed the title Question: Why http? Add TCP checks on top of HTTP checks Jan 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

6 participants
@seeker89 @pjperez @JoshuaOliphant @ivkalita @dannyk81 and others