Fix Ping check on non-English Windows by decoding with the OEM code page - #3092
Fix Ping check on non-English Windows by decoding with the OEM code page#3092ddog-nasirthomas wants to merge 2 commits into
Conversation
|
🎯 Code Coverage (details) 🔗 Commit SHA: 59d5861 | Docs | Datadog PR Page | Give us feedback! |
|
|
||
| def _exec_ping_windows(self, command): # pragma: nocover | ||
| # ping emits localized output in the OEM code page; get_subprocess_output would hardcode a UTF-8 decode. | ||
| result = subprocess.run(command, capture_output=True) |
There was a problem hiding this comment.
Dropping the cmd /c wrapper means subprocess.run no longer goes through a shell, so failures that used to come back as a non-zero exit code (caught by retcode != 0 at line 75 → CheckException → DOWN) can now raise Python exceptions that check()'s except CheckException at line 106 doesn't catch. Concretely: an 8-block IPv6 host builds cmd = "ping6" (line 41), and Windows ships no ping6.exe, so subprocess.run(["ping6", ...]) raises FileNotFoundError — an unhandled traceback instead of a CRITICAL service check. While you're touching this call, it's also worth adding timeout= (currently only -w bounds ping's own wait, not the subprocess itself) so a wedged process can't block the collector.
|
|
||
| return lines | ||
|
|
||
| def _exec_ping_windows(self, command): # pragma: nocover |
There was a problem hiding this comment.
This method is the entire fix for a bug that has already escaped once (the 1.0.3 chcp 65001 attempt), yet it's # pragma: nocover with no tests, and test-all-windows.yml is jobs: {} — nothing in CI exercises this path. The "oem" codec itself can't be tested off Windows (codecs.lookup("oem") raises LookupError), but the branching logic (empty-output raise, retcode/err propagation) can be, if the codec is injectable so a test can substitute a real single-byte codec like cp850 and feed it the exact 0x81 byte from the reported crash. Worth confirming in the PR description too: was this validated against a real non-English Windows host? GetOEMCP() can diverge from GetConsoleOutputCP() — the same class of encoding-API mismatch that made the 1.0.3 fix ineffective.
| if not out: | ||
| raise SubprocessOutputEmptyError("get_subprocess_output expected output but had none.") |
There was a problem hiding this comment.
The error message here still says "get_subprocess_output expected output but had none.", but this method exists specifically to bypass get_subprocess_output. Since SubprocessOutputEmptyError isn't caught by check()'s except CheckException, this message is exactly what surfaces in logs when this path fails, and it will misdirect debugging.
|
|
||
| ***Fixed***: | ||
|
|
||
| * Fix the Ping check still crashing on non-English Windows. The ``chcp 65001`` approach in 1.0.3 did not change the encoding of ping's output, so localized output still failed with ``'utf-8' codec can't decode byte 0x81``. On Windows the check now decodes ping's output using the OEM code page instead of UTF-8 ([#<PR>](https://github.com/DataDog/integrations-extras/pull/<PR>)). |
There was a problem hiding this comment.
let's add the PR number to the link
| @@ -1,5 +1,11 @@ | |||
| # CHANGELOG - Ping | |||
There was a problem hiding this comment.
probably worth updating the README as well, as it says "Versions prior to 1.0.3 may not work correctly if the installed Windows language is not set to English."
What does this PR do?
Fixes the
pingcheck crashing on non-English Windows. On Windows the check now runspingviasubprocess.runand decodes its output with the OEM code page (decode("oem", errors="replace")) instead of going throughget_subprocess_output, which hardcodes a UTF-8 decode. Thechcp 65001approach from 1.0.3 is removed.Motivation
1.0.3 tried to fix this by switching the console to UTF-8 (
chcp 65001), but that turned out not to change the encoding ping actually uses , so a customer on German Windows still hit the same crash ('utf-8' codec can't decode byte 0x81) after updating.Review checklist
no-changeloglabel attachedAdditional Notes
Anything else we should know when reviewing?