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

Fix ICMP count and cleanup. #44

Merged
merged 1 commit into from
Mar 16, 2024
Merged

Conversation

RomainMou
Copy link
Contributor

@RomainMou RomainMou commented Jan 1, 2024

Hello,

While conducting tests with the exporter, I observed something unusual in my packet capture: it appears that the ICMP check doesn't respect the count and always sends three packets:

14:28:45.361446 IP 10.100.3.100 > 10.100.3.1: ICMP echo request, id 1, seq 0, length 13

14:28:50.360774 IP 10.100.3.100 > 10.100.3.1: ICMP echo request, id 2, seq 0, length 13
14:28:50.362169 IP 10.100.3.100 > 10.100.3.1: ICMP echo request, id 1, seq 1, length 13

14:28:55.360262 IP 10.100.3.100 > 10.100.3.1: ICMP echo request, id 3, seq 0, length 13
14:28:55.361605 IP 10.100.3.100 > 10.100.3.1: ICMP echo request, id 2, seq 1, length 13
14:28:55.362916 IP 10.100.3.100 > 10.100.3.1: ICMP echo request, id 1, seq 2, length 13

[...]

14:30:25.380410 IP 10.100.3.100 > 10.100.3.1: ICMP echo request, id 2, seq 19, length 13
14:30:25.380417 IP 10.100.3.100 > 10.100.3.1: ICMP echo request, id 3, seq 18, length 13
14:30:25.380525 IP 10.100.3.100 > 10.100.3.1: ICMP echo request, id 4, seq 0, length 13

In reality, it sends one ICMP packet per interval and initiates a new task at each interval until we reach const MaxConcurrentJobs = 3, as evident from the seq being incremented properly to 19 (count was set to 20).

This pull request addresses that issue, I believe. I also noticed that ICMP and TCP checks differed from the others, and attempted to standardize them/remove what seems to not really be used for interval management. However, this is my first time working with Go, so I'm not entirely sure. I may have missed something or made a mistake.

Thank you.

@syepes syepes merged commit 8ba5f2f into syepes:master Mar 16, 2024
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.

2 participants