Skip to content

fix(collector): correct jsonpath alias parsing for rows missing path - #4265

Open
orangeCatDeveloper wants to merge 2 commits into
apache:masterfrom
orangeCatDeveloper:fix/issue-3307-jsonpath-row-alias
Open

fix(collector): correct jsonpath alias parsing for rows missing path#4265
orangeCatDeveloper wants to merge 2 commits into
apache:masterfrom
orangeCatDeveloper:fix/issue-3307-jsonpath-row-alias

Conversation

@orangeCatDeveloper

@orangeCatDeveloper orangeCatDeveloper commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

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:

query  $.items.*.status.containerStatuses[0].restartCount  →  [5, 2]

pod-b has no containerStatuses, and jayway skips it without a placeholder. Distributing [5, 2] by row index:

row pod gets should be
0 pod-a 5 5
1 pod-b 2 (pod-c's value) null
2 pod-c nothing 2

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].restartCount must 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's cpu_delta=$.cpu_stats... - $.precpu_stats... doesn't equal any single aliasField.

Test plan

  • Unit tests reproduce both bugs (red before the fix, green after): HttpCollectImplTest, MetricsCollectTest, JsonPathParserTest. Full collector suites pass.
  • End-to-end on a local instance with a mocked pods API, same monitor, only collector jars swapped: before → rc all null; after → 5 / null / 2.
  • Regression: official spring_gateway (profile=$.activeProfiles[0]) and docker (name=$.Names[0]) monitors return identical values before and after.

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.

@orangeCatDeveloper
orangeCatDeveloper force-pushed the fix/issue-3307-jsonpath-row-alias branch 2 times, most recently from 12f5185 to 4ab65b9 Compare August 13, 2026 19:42
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.
@orangeCatDeveloper
orangeCatDeveloper force-pushed the fix/issue-3307-jsonpath-row-alias branch from 4ab65b9 to 7d030b8 Compare August 14, 2026 06:20

@Aias00 Aias00 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
  • parseContentWithJsonPath is used across the codebase (including the new row() helper in JsonPathParserTest). With SUPPRESS_EXCEPTIONS it will silently return null on a missing path instead of throwing PathNotFoundException, 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

  1. aliasFields.contains(expressionStr) is an exact-string match. A calculates alias reference that differs only in whitespace/casing from the aliasField would 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.
  2. In HttpCollectImpl, when an alias returns multiple values (wildcard), resultValue = aliasValues then String.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#parseResponseByJsonPathKeepsRowAlignmentWhenAliasPathMissing reproduces the 3-row case (pod-a/5, pod-b-pending/NULL, pod-c/2) well.
  • Bug 2 (calculates silently eaten): I traced fieldAliasMap consumption in calculateFields (value = aliasFieldValueMap.get(aliasField)). The new branch routes through the existing alias-mapping path and is mutually exclusive with fieldExpressionMap, so the logic is sound. MetricsCollectTest#calculateFieldsMapsIndexedJsonPathAlias asserts 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] <title>jsonpath parse data error [question] <title>use jsonpath parse k8s api reponse error

3 participants