pro-bing icon indicating copy to clipboard operation
pro-bing copied to clipboard

Pinger timeout should return an error

Open wbollock opened this issue 2 years ago • 4 comments

If the pinger reaches timeout, no error is returned from OnFinish() and it makes it seem like the probe finished successfully, even if nothing was returned from the ping job.

https://github.com/prometheus-community/pro-bing/blob/0999adf48b00a72b9ce152a562e2585ac986a6f6/ping.go#L538-L539

I believe this should return an error indicating the timeout was reached before the ping job finished fully. Happy to make the PR if this sounds good

wbollock avatar Oct 12 '23 15:10 wbollock

Adding a +1 for this. Given that an unreachable host will block on .Run() without a timeout, it would be great to have some way to determine whether the pinger timed out.

lllama avatar Jan 25 '24 01:01 lllama

indeed. Especially since one specifically has to set the timeout (which I initially did not do and wondered why my pinger go routine sometimes would not properly return). Does it make sense to also set a timeout by default when the pinger is initialized?

fkr avatar Jan 25 '24 09:01 fkr

indeed. Especially since one specifically has to set the timeout (which I initially did not do and wondered why my pinger go routine sometimes would not properly return). Does it make sense to also set a timeout by default when the pinger is initialized?

I tried running with pinger.Timeout = 1000 but the result is that it exits and returns like it responded to the ping (which is not true)

fuomag9 avatar Mar 03 '24 17:03 fuomag9

I may not understand, but a RunWithContext function exists, I'm using that to check for a timeout.

ctx, _ := context.WithTimeout(context.Background(), deadline)
err = client.RunWithContext(ctx)
	if err != nil {
		if errors.Is(err, context.DeadlineExceeded) {
			// Timeout
		}
		...
	}

Does this not work for your situation?

jmkng avatar Aug 23 '24 16:08 jmkng