prometheus-community / pro-bing

A library for creating continuous probers
MIT License
351 stars 61 forks source link

Pinger timeout should return an error #70

Open wbollock opened 1 year ago

wbollock commented 1 year ago

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

lllama commented 10 months ago

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.

fkr commented 10 months ago

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?

fuomag9 commented 9 months ago

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)

jmkng commented 3 months ago

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?