goldpinger icon indicating copy to clipboard operation
goldpinger copied to clipboard

Add TCP checks on top of HTTP checks

Open JoshuaOliphant opened this issue 6 years ago • 9 comments

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?

JoshuaOliphant avatar Jan 23 '19 17:01 JoshuaOliphant

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 👍

seeker89 avatar Jan 23 '19 18:01 seeker89

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 :)

ivkalita avatar Jan 23 '19 18:01 ivkalita

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?

JoshuaOliphant avatar Jan 23 '19 21:01 JoshuaOliphant

Yup, yup and yup, respectively.

seeker89 avatar Jan 24 '19 15:01 seeker89

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.

pjperez avatar Jan 29 '19 10:01 pjperez

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

seeker89 avatar Mar 14 '19 23:03 seeker89

Thumbs up to that! 👍 😄

pjperez avatar Mar 15 '19 12:03 pjperez

@seeker89 there you go, I'm number 10 :smile:

dannyk81 avatar Aug 09 '19 16:08 dannyk81

I'm working for it. Use icmp instead http /ping.But you still need to use the http api /check ans /check_all. Maybe I should launch a pr as soon as possible.

ZYao123 avatar Oct 20 '22 02:10 ZYao123