blackbox_exporter icon indicating copy to clipboard operation
blackbox_exporter copied to clipboard

Use go-ping to implement ICMP probes

Open mem opened this issue 4 years ago • 5 comments

While working on a change to the ICMP prober, I got a little frustrated at the structure of that code. I tried a small rewrite a couple of times before realizing that there's actually a nice library out there that BBE can use, nameping go-ping. It has several good things going for it, but the most notable is that the code is very well organized.

I would like to propose that we swap the current implementation for this one.

Signed-off-by: Marcelo E. Magallon [email protected]

mem avatar Apr 19 '21 22:04 mem

Hmm, I'm not sure I would like to swap this out. There are some nice things about the go-ping library, but it's maybe not appropriate for use here. We would need to be very careful about testing and making sure this doesn't break the existing behavior.

SuperQ avatar Apr 20 '21 07:04 SuperQ

I've marked this as draft since we need Don't Fragment support first.

SuperQ avatar Apr 20 '21 09:04 SuperQ

Hmm, I'm not sure I would like to swap this out. There are some nice things about the go-ping library, but it's maybe not appropriate for use here. We would need to be very careful about testing and making sure this doesn't break the existing behavior.

I get you. Part of the motivation is precisely that we don't have tests for the code in icmp.go, and that in turn is due partly to the structure of that code. go-ping on the other hand does have tests for important parts of the code, and some of the recent changes enable testing of the more complicated parts (even if I haven't had the chance to write those tests).

mem avatar Apr 20 '21 12:04 mem

If I understand previously merge PRs around this topic correctly, there is another difference between the ICMP implementation of blackbox_exporter and go-ping: You seem to be using randomized ICMP identifiers to avoid NAT issues (prometheus/blackbox_exporter/pull/412), while go-ping doesn't support setting the identifier yet: digineo/go-ping/pull/19

outofrange avatar Jul 20 '23 16:07 outofrange