Skip to content

[Bug] [UI] Fix NaN display in job detail Overview table when tablePaths does not match metrics keys#10965

Open
77amyfly wants to merge 2 commits into
apache:devfrom
77amyfly:fix/UIbug
Open

[Bug] [UI] Fix NaN display in job detail Overview table when tablePaths does not match metrics keys#10965
77amyfly wants to merge 2 commits into
apache:devfrom
77amyfly:fix/UIbug

Conversation

@77amyfly
Copy link
Copy Markdown

Purpose of this pull request

Fix #10547

Fix NaN display in the job detail Overview table. The sourceCell and sinkCell functions use tablePaths to look up values in metrics, but the tablePaths format does not match the metrics keys returned by the backend. For example:

  • tablePaths returns: ["fake"]
  • metrics key is: "Source[0].fake"

This mismatch causes Number(undefined) to be evaluated as NaN. Fixed by using endsWith to match tablePaths against metrics keys instead of direct lookup.

Does this PR introduce any user-facing change?

Yes. The job detail Overview table now correctly displays metrics values (Received Bytes, Write Bytes, Received Count, Write Count, etc.) instead of NaN when tablePaths does not match metrics keys.
before fix:
Screenshot 2026-05-27 at 12 35 00 PM
after fix:
Screenshot 2026-05-27 at 12 35 53 PM

How was this patch tested?

  1. Added unit test in seatunnel-engine-ui/src/tests/detail.spec.ts to verify that no NaN is displayed when tablePaths does not match metrics keys.
  2. All existing unit tests pass after the fix (6 test files, 12 tests passed).

Check list

  • If any new Jar binary package adding in your PR, please add License Notice according New License Guide
  • If necessary, please update the documentation to describe the new feature.
  • If necessary, please update incompatible-changes.md to describe the incompatibility caused by this PR.

… and sinkCell

Add unit test to verify no NaN is displayed when tablePaths does not match metrics keys
@77amyfly 77amyfly marked this pull request as draft May 27, 2026 19:37
@github-actions github-actions Bot added the Zeta label May 27, 2026
@77amyfly 77amyfly marked this pull request as ready for review May 27, 2026 22:15
Copy link
Copy Markdown
Contributor

@DanielLeens DanielLeens left a comment

Choose a reason for hiding this comment

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

Thanks for working on this. I went through the full current diff and traced the actual metric path from the engine backend into the UI table rendering.

What this PR fixes

  • User pain: the Job Detail Overview table can render NaN when the table path shown on a vertex does not exactly match the metric map key produced by the backend.
  • Fix approach: the UI now falls back from exact lookup to suffix matching, so fake can match a backend key like Source[0].fake.
  • One-line value: this removes the NaN symptom for the simple single-vertex case.

Runtime path I checked

Engine metric aggregation
  -> BaseService.processMetric(...) [BaseService.java:441-586]
      -> for single vertex: key can become "Source[0].table" / "Sink[0].table" [BaseService.java:463-472]
      -> for tagged array metrics: key becomes "identifier.table" [BaseService.java:494-523]

UI Overview table
  -> detail.tsx sourceCell/sinkCell [detail.tsx:94-128]
      -> iterate row.tablePaths
      -> find first metric key where key.endsWith(path) [detail.tsx:103-106, 121-124]
      -> sum that value into the displayed cell

Review findings

Problem 1: Wrong metric attribution when multiple vertices share the same table path

  • Location: seatunnel-engine/seatunnel-engine-ui/src/views/jobs/detail.tsx:103-106, seatunnel-engine/seatunnel-engine-ui/src/views/jobs/detail.tsx:121-124
  • Severity: High
  • Raised before by others: No
  • Why this is a real bug:
    The backend intentionally emits per-vertex metric keys such as Source[0].fake and Sink[0].fake instead of the bare table path when there is vertex-level attribution on the same table [BaseService.java:463-472, 494-523]. The new UI code picks the first key whose suffix matches fake. That works only if there is exactly one matching vertex. As soon as two sources or two sinks share the same tablePaths entry, the first suffix match can belong to the wrong vertex, so the Overview table silently shows another vertex's numbers.
  • Risk:
    This is not just a cosmetic mismatch. On normal multi-source or fan-out jobs, the per-row metrics in Overview become incorrect while still looking valid, which is worse than NaN because it hides the attribution error.
  • Better fix:
    Match on the current vertex identifier as well, not only on the suffix. The UI already has vertexName, so it can derive Source[0] / Sink[0] and prefer identifier + "." + path. An even cleaner option would be to expose a backend map keyed by vertex id or vertex identifier directly so the UI does not need to reverse-engineer the key shape.

Problem 2: The new regression test only covers the single-match happy path

  • Location: seatunnel-engine/seatunnel-engine-ui/src/tests/detail.spec.ts:49-111
  • Severity: Medium
  • Raised before by others: No
  • Why this matters:
    The new test uses one source and one sink, both on fake, so there is only one possible suffix match for each metric group. That verifies the NaN symptom is gone, but it does not protect against the real ambiguity introduced by endsWith(path) when multiple vertices share the same table path.
  • Risk:
    The current implementation can regress into wrong attribution without any test failure.
  • Better fix:
    Add a regression case with at least two source vertices or two sink vertices sharing the same table path and assert each row reads its own metric bucket.

Test stability

  • Rating: Stable
  • Why:
    The new frontend test is deterministic, uses mocked service data, and does not depend on timing, ports, or external state.

Merge conclusion

Conclusion: fix required before merge

Blocking items:

  1. Problem 1

Non-blocking suggestions:

  1. Problem 2

The current direction is good for the original NaN symptom, but I would not merge it until the per-vertex attribution stays correct on the real multi-vertex path as well.

@nzw921rx
Copy link
Copy Markdown
Collaborator

+1 good job

@DanielLeens
Copy link
Copy Markdown
Contributor

Thanks for taking a look here as well.

On my side, the current blocker is still the same one from the latest Daniel review: on the current head 262278b65888f0da43900e2bdb86d8360f2ae928, the endsWith(path) fallback can still attribute metrics to the wrong vertex when multiple sources or sinks share the same table path.

Since there is still no new commit after that reviewable head, I am keeping this as a reply-only follow-up rather than starting another full re-review in this batch. Once a code update lands for that attribution path, please ping me again and I will recheck the new head from scratch.

Copy link
Copy Markdown
Contributor

@davidzollo davidzollo left a comment

Choose a reason for hiding this comment

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

Good job.
+1 if CI passes

@DanielLeens
Copy link
Copy Markdown
Contributor

Thanks @davidzollo. I saw the approval, but on my side the blocker from the latest Daniel review still stands on the unchanged head 262278b65888f0da43900e2bdb86d8360f2ae928: the endsWith(path) fallback can still misattribute metrics when multiple vertices share the same table path. Since there is still no new commit after that head, I am keeping this as a reply-only follow-up and waiting for a code update before re-reviewing from scratch.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] [UI] The task home page does not display the rate indicators, but they are clearly shown.

4 participants