Use go-ping to implement ICMP probes

[mem]

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]

[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.

[SuperQ]

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

[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.

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

[outofrange]

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 (/pull/412), while go-ping doesn't support setting the identifier yet: digineo/go-ping/pull/19

[razvancostincalin]

Hi,

I can see that digineo/go-ping#19 has already been merged.
Is there anything else blocking this from moving forward?

I think it will be nice addition to blackbox.

Thanks