fix(collector): keep partial telnet metrics, surface raw reply

[orangeCatDeveloper]

What's changed?

Fixes #1693: zookeeper monitors fail with "The cmd execution results do not match the expected number of metrics". One blunt check caused two different failures reported in the issue:

if (resultMap.size() < aliasFields.size()) {
    builder.setMsg("The cmd execution results do not match the expected number of metrics.");
    return;
}

Case 1: valid partial reply thrown away (reporter ZachariahHu)

conf on standalone zookeeper 3.4.14 returns 8 keys; the template defines 10 (dataDirSize/dataLogSize only exist in newer versions). Nothing is misconfigured, yet the whole group is discarded every cycle.

Fix: fail only when the reply contains none of the expected fields. A partial reply keeps its parsed values; missing fields fall through to the pre-existing null-padding path.

Before — 9 parsed entries thrown away, conf shows no data:

ERROR TelnetCollectImpl - telnet response data not enough: {maxSessionTimeout=40000, dataDir=/data/version-2, responseTime=7, clientPort=2181, tickTime=2000, minSessionTimeout=4000, dataLogDir=/datalog/version-2, serverId=0, maxClientCnxns=60}

After — same server, collected without errors:

{"clientPort": "2181", "dataDir": "/data/version-2", "dataDirSize": null,
 "dataLogDir": "/datalog/version-2", "dataLogSize": null, "tickTime": "2000",
 "maxClientCnxns": "60", "minSessionTimeout": "4000",
 "maxSessionTimeout": "40000", "serverId": "0"}

Case 2: whitelist refusal reason swallowed (reporter leim)

When the 4lw whitelist blocks conf (bitnami image default), zookeeper replies with an explanation instead of data — but parsing dropped the raw reply, so the user only saw the generic message above.

Fix: parsing returns the raw reply alongside the parsed map (CmdResult record); the failure message now carries it, in logs and in the UI popup.

Before — the refusal reason is nowhere:

ERROR TelnetCollectImpl - telnet response data not enough: {responseTime=18}

After — zookeeper's own explanation reaches the user:

WARN TelnetCollectImpl - telnet cmd [conf] returned no expected metrics: conf is not executed because it is not in the whitelist.

Failure is judged by "no expected field present", not "map is empty", so refusal text that parses as key/value (error=conf disabled) still fails.

Parsing hardening

Values split at the first separator only (split(sep, 2)) so dataDir=/data/zk=a survives; duplicate keys keep the first value; header-only/newline-only replies no longer throw on lines[0]. Tradeoff: a truncated reply with some expected fields present now yields visible null cells instead of a hard failure. zookeeper 3.9 with an open whitelist still collects all 10 columns.

UI popup before/after (case 2):

Checklist

• I have read the Contributing Guide
• I have written the necessary doc or comment.
• I have added the necessary unit tests and all cases have passed.

Add or update API

• I have added the necessary e2e tests and all cases have passed.

[Aias00]

Review: fix(collector): keep partial telnet metrics, surface raw reply — APPROVED

Fixes #1693 (zookeeper 4lw command refused → whole collection failed). Clean, well-tested improvement.

What's correct

• execCmdAndParseResult now returns a CmdResult(values, rawResponse) record, decoupling the parsed map from the raw bytes.
• Partial metrics preserved: the old guard was resultMap.size() < aliasFields.size() → FAIL if any field was missing. New logic only FAILs when none of the expected non-responseTime fields were returned (expectsCmdMetrics && !hasExpectedMetric), and missing fields are padded with NULL_VALUE (confirmed by testCollectPadsMissingMetrics: a=1 present, b/c → NULL). This is the right "keep what we got" semantics.
• Raw reply surfaced: on total failure the FAIL message now includes sanitizeReply(cmdResult.rawResponse()), and sanitizeReply strips control chars and abbreviates to 300 chars — good for diagnosing whitelist refusals without log injection. Test testCollectFailsWithRawReplyWhenNothingParsed asserts the reply text propagates.
• Robustness fixes in the parser:
  • if (lines.length > 0 && ...) guards lines[0] access when the response is empty.
  • if (lines.length == 0) return new CmdResult(new HashMap<>(16), result); handles empty responses (still exposes raw for the error message).
  • Collectors.toMap(..., (first, second) -> first, HashMap::new) adds a merge function so duplicate keys no longer throw IllegalStateException.
  • separator derived from lines[0] once — functionally same as before but cleaner.

Non-blocking suggestions

• The separator is still keyed off lines[0]: if a data line uses a different separator than the header, it would misparse. Same limitation as before, just noting it.
• A test for the duplicate-key merge path (two lines with same key) would lock that fix in, but the existing tests already cover the main cases.

Verdict: APPROVED. Good partial-success semantics, safer logging, and solid tests.