fix(collector): correct jsonpath alias parsing for rows missing path - #4265
fix(collector): correct jsonpath alias parsing for rows missing path#4265orangeCatDeveloper wants to merge 2 commits into
Conversation
12f5185 to
4ab65b9
Compare
Http jsonPath collection resolved alias paths with a global "parseScript + alias" query indexed by row number, so rows missing the path (e.g. pending pods without containerStatuses) misaligned every following row. Calculates like rc=$.status.containerStatuses[0].restartCount also compiled as JEXL array access and silently evaluated to null. Alias paths are now evaluated per row, and calculates equal to an aliasField skip JEXL entirely. Fixes apache#3307.
4ab65b9 to
7d030b8
Compare
Aias00
left a comment
There was a problem hiding this comment.
Thanks for the detailed fix and the thorough test plan — both bugs are real and the per-row / alias-reference approach is the right direction. I verified the logic against calculateFields and the tests, and everything checks out except one issue that leaks into the shared parser. Requesting a change before merge.
🔴 Must fix — shared Configuration makes PARSER also suppress exceptions
File: hertzbeat-collector/hertzbeat-collector-common/.../util/JsonPathParser.java
Configuration conf = Configuration.defaultConfiguration()
.addOptions(Option.DEFAULT_PATH_LEAF_TO_NULL)
.addOptions(Option.ALWAYS_RETURN_LIST);
PARSER = JsonPath.using(conf);
// a single row legitimately may not contain the queried path
ROW_PARSER = JsonPath.using(conf.addOptions(Option.SUPPRESS_EXCEPTIONS));In json-path 2.9.0 (confirmed via the repo's pom.xml), Configuration#addOptions(...) mutates the instance in place and returns this. Because both PARSER and ROW_PARSER hold a reference to the same conf, after conf.addOptions(SUPPRESS_EXCEPTIONS) runs, PARSER now also has SUPPRESS_EXCEPTIONS.
Consequences:
- The intended design (only the row-level parser suppresses missing-path exceptions) is defeated.
parseContentWithJsonPathis used across the codebase (including the newrow()helper inJsonPathParserTest). WithSUPPRESS_EXCEPTIONSit will silently returnnullon a missing path instead of throwingPathNotFoundException, which can mask real misconfiguration errors in other collectors and change existing behavior.
It's a silent bug — all tests still pass — so it needs to be fixed explicitly. Suggested fix: give ROW_PARSER its own Configuration instead of mutating the shared one:
ROW_PARSER = JsonPath.using(Configuration.defaultConfiguration()
.addOptions(Option.DEFAULT_PATH_LEAF_TO_NULL)
.addOptions(Option.ALWAYS_RETURN_LIST)
.addOptions(Option.SUPPRESS_EXCEPTIONS));🟡 Minor / optional
aliasFields.contains(expressionStr)is an exact-string match. Acalculatesalias reference that differs only in whitespace/casing from thealiasFieldwould fall through to JEXL and still silently null. Acceptable given the YAML is generated from the same fields, but a one-line comment documenting the "RHS must exactly equal an aliasField" contract would help future maintainers.- In
HttpCollectImpl, when an alias returns multiple values (wildcard),resultValue = aliasValuesthenString.valueOf(...)produces a"[...]"string. Harmless for single-value aliases, but worth a note or an explicit "take first / join" decision.
🟢 Verified correct
- Bug 1 (cross-row misalignment): evaluating
parseRowWithJsonPath(objectValue, alias)per row, with a missing path yielding an empty list → NULL cell, eliminates the row-index alignment entirely.HttpCollectImplTest#parseResponseByJsonPathKeepsRowAlignmentWhenAliasPathMissingreproduces the 3-row case (pod-a/5, pod-b-pending/NULL, pod-c/2) well. - Bug 2 (calculates silently eaten): I traced
fieldAliasMapconsumption incalculateFields(value = aliasFieldValueMap.get(aliasField)). The new branch routes through the existing alias-mapping path and is mutually exclusive withfieldExpressionMap, so the logic is sound.MetricsCollectTest#calculateFieldsMapsIndexedJsonPathAliasasserts the expected result. - Test coverage spans common / basic / collector modules and adds two new test classes — good.
Suggested next step
Fix the ROW_PARSER configuration isolation, and (optional but nice) add a one-line regression test asserting that parseContentWithJsonPath still throws / behaves as before on a missing path. Once that's in, this is good to merge.
What's changed?
Fixes #3307.
Fixes #3260.
A custom alias field
$.status.containerStatuses[0].restartCount(monitor pod restart count, jsonPath parsing) returns an empty or wrong column. Example response,parseScript: $.items.*:{"items": [ {"metadata": {"name": "pod-a"}, "status": {"containerStatuses": [{"restartCount": 5}]}}, {"metadata": {"name": "pod-b"}, "status": {"phase": "Pending"}}, {"metadata": {"name": "pod-c"}, "status": {"containerStatuses": [{"restartCount": 2}]}} ]}Two independent bugs. Both hit this user.
Bug 1: rows receive each other's values (
HttpCollectImpl)Problem. The alias column was filled by one global query, then distributed by row index:
pod-b has no
containerStatuses, and jayway skips it without a placeholder. Distributing[5, 2]by row index:Fix. Ask each row's own object instead of the whole response: new
JsonPathParser.parseRowWithJsonPath(rowObject, alias). A row that lacks the path returns an empty list → NULL cell. No cross-row indexing, nothing to misalign.Bug 2: the calculates step silently eats the value (
MetricsCollect)Problem.
calculates: rc=$.status.containerStatuses[0].restartCountmust be classified: is the right side a column reference or a formula? The old rule was "if it compiles as JEXL, it's a formula". This path does compile — JEXL reads[0]as array access on a variable named$.status.containerStatuses. That variable doesn't exist, so it evaluates to null with no log. The column stays empty even when Bug 1 is fixed.Fix. Before compiling, check: right side exactly equals one of the metric's
aliasFields? Then it is by definition a column reference → map it directly, skip JEXL. Real formulas keep working — docker'scpu_delta=$.cpu_stats... - $.precpu_stats...doesn't equal any single aliasField.Test plan
HttpCollectImplTest,MetricsCollectTest,JsonPathParserTest. Full collector suites pass.rcall null; after →5 / null / 2.spring_gateway(profile=$.activeProfiles[0]) anddocker(name=$.Names[0]) monitors return identical values before and after.Checklist
Add or update API