Enhance ping_function for server and port handling #63
Conversation
Refactor ping_function to handle both server and port for pinging. Added compatibility for checking TCP connections using netcat.
|
@Slyker thanks for the PR and apologies for the delay in response, overall I think this seems like a good addition, but I don't think we would want to capture ping responses in seconds, lets keep it at ms as earlier Also, can you please update the ping plugin docs to mention that servers with ports are supported as well |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the ping plugin to add support for TCP port connectivity checks in addition to standard ICMP ping, and standardizes the output format to seconds across both methods.
- Adds TCP port connectivity checking using
ncwhen server is specified inhost:portformat - Standardizes output format to seconds (with
.2fsformat) for both ICMP ping and TCP checks - Removes OS-specific case statements in favor of a unified approach
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
|
@2KAbhishek I think it should be good as it is, I've updated the readme and changed some part to be more accurate |
| avg_time=$(echo "$ping_output" | awk -F'=' '/min\/avg\/max/ {split($2, a, "/"); print a[2]}' | tr -d ' ') | ||
| # Detect float and unit | ||
| if [[ "$avg_time" == *.* ]]; then | ||
| # Assume ms if less than 50, s if >= 1 | ||
| val="$avg_time" | ||
| unit="ms" | ||
| # Some pings can output >1, consider <10 as s, >10 as ms | ||
| if (( $(echo "$val >= 1" | bc) )) && (( $(echo "$val < 50" | bc) )); then | ||
| unit="ms" | ||
| elif (( $(echo "$val >= 50" | bc) )); then | ||
| unit="ms" | ||
| elif (( $(echo "$val < 1" | bc) )); then | ||
| unit="s" | ||
| fi | ||
| normalize_time "$val" "$unit" | ||
| else | ||
| normalize_time "$avg_time" ms | ||
| fi | ||
| ;; |
There was a problem hiding this comment.
can we please simply return ms instead, I am not planning to have ping time displayed in seconds right now
also lets get rid of the comments
| normalize_time() { | ||
| value="$1" | ||
| unit="$2" | ||
| # Convert to ms if needed | ||
| case "$unit" in | ||
| ms) printf "%.2f ms" "$value" ;; | ||
| s) printf "%.2f ms" "$(echo "$value * 1000" | bc -l)" ;; | ||
| *) printf "%s" "$value" ;; |
| CYGWIN* | MINGW32* | MSYS* | MINGW*) | ||
| # Windows compatibility stub |
There was a problem hiding this comment.
You can remove the placeholder for windows as well as well
There was a problem hiding this comment.
Left some cleanup suggestions @Slyker
Can you confirm if you tested the changes in mac / linux or both
2 participants
Refactor ping_function to handle both server and port for pinging. Added compatibility for checking TCP connections using netcat.
This pull request refactors the
ping_functioninplugins/ping.shto improve its flexibility and output consistency. The main change is the addition of support for pinging servers specified with a port (host:port), measuring response time usingncinstead ofping, and standardizing the output format to seconds.Functionality and compatibility improvements:
host:port, usingncto measure connection time and outputting the duration in seconds.-.--swhen a ping or connection fails, making the output clearer.