Trace server improvements

[nforro]

• Use enum values of supported OTEL types directly
  This fixes span rendering in trace-server.
• Make spans linkable
  With these changes clicking on a span updates the URL and when accessing an URL with a span ID the corresponding span is scrolled to and focused. Sidebar navigation has been updated to use span IDs as well,
  for consistency.

[qodo-for-packit]

PR Summary by Qodo

Trace server: fix OTEL enum rendering and add deep-linkable spans

🐞 Bug fix ✨ Enhancement 🕐 20-40 Minutes

AI Description

• Convert Enum-valued OTEL attributes to raw values to prevent span rendering issues.
• Add span deep-linking: clicking a span updates the URL and supports load-and-scroll by span ID.
• Harden async view rendering with route guards to avoid stale updates during navigation.

Diagram

graph TD
  U([User]) --> R["Router (#hash)"] --> TD["Trace detail view"] --> API["Trace API"] --> SP[(Spans data)]
  TD --> UI["Span list + sidebar"] --> H["URL w/ spanId"]
  UI --> CSS["Highlight styles"]

  subgraph Legend
    direction LR
    _user([User]) ~~~ _ui["UI module"] ~~~ _api["API call"] ~~~ _data[(Data)]
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Use query params for span selection

• ➕ Avoids brittle path parsing/pop logic for optional spanId
• ➕ Easier to extend later (e.g., tab, time range, filters)
• ➖ Would require changing the existing hash route format and router logic
• ➖ Potentially breaking bookmarked URLs unless backward compatibility is added

2. Use history.pushState instead of replaceState

• ➕ Allows browser Back/Forward to traverse span selections naturally
• ➖ Could create excessive history entries when exploring many spans
• ➖ May be annoying unless throttled/debounced or only used for deliberate actions

3. Centralize URL update + highlight into a single helper

• ➕ Reduces duplicated hash parsing and highlight clearing across span row/sidebar/jump button
• ➕ Lowers risk of subtle inconsistencies in future updates
• ➖ Primarily a maintainability refactor; current changes are already functional
• ➖ May slightly increase up-front complexity for a small codebase

Recommendation: Current approach (optional spanId in the hash path + replaceState) is pragmatic and keeps routing consistent with existing URL structure. If this pattern grows, consider either (a) migrating span selection to a query parameter for robustness, or (b) extracting a small helper to update the trace URL and manage highlight state to avoid further duplication.

Files changed (3) +120 / -9

Enhancement (2) +110 / -4

app.jsAdd span deep-linking, focus/highlight behavior, and async route guards +100/-4

Add span deep-linking, focus/highlight behavior, and async route guards

• Extends trace routing to accept an optional spanId and passes it into trace rendering so the target span can be scrolled into view and highlighted on load. Clicking span rows/sidebar items updates the URL to include the selected span ID and briefly highlights the selected element. Adds view/route guards after async API calls to avoid updating the DOM when the user has navigated away.

trace_server/static/app.js

style.cssIntroduce theme highlight color and hover affordance for span headers +10/-0

Introduce theme highlight color and hover affordance for span headers

• Adds a '--highlight-bg' theme variable for both light/dark modes. Styles span row headers with hover background and minor spacing/transition to reinforce clickability and highlight behavior.

trace_server/static/style.css

Bug fix (1) +10 / -5

openinference-streaming.patchNormalize Enum-valued attributes before OTEL type checks +10/-5

Normalize Enum-valued attributes before OTEL type checks

• Adds Enum handling so span attribute values that are Enums are converted to their underlying '.value'. This prevents non-OTEL attribute types from leaking into span attributes and fixes downstream span rendering expectations.

openinference-streaming.patch

[qodo-for-packit]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📜 Skill insights (0)

1. Enum arrays stringify attributes 🐞 Bug ≡ Correctness

Description

In BeeAIInstrumentor’s OTEL attribute normalization, only top-level Enum values are unwrapped;
lists/tuples containing Enum elements fail the OTEL-type check and are coerced to a single string.
This changes attribute shape (array -> string) and can break downstream rendering/querying that
expects an array of primitives.

Code

openinference-streaming.patch[R85-88]

++            # Extract enum value if it's an enum
++            if isinstance(value, Enum):
++                value = value.value
++            # Convert to string if not an OTEL type

Relevance

●●● Strong

Likely real bug: Enum elements in arrays get coerced to string; team accepts shape-preserving
correctness fixes.

PR-#115

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new logic unwraps Enum only for scalar values. For list/tuple attributes, it validates
elements against _OTEL_TYPES without unwrapping element Enums, so any Enum element causes the
whole collection to be stringified via str(value).

openinference-streaming.patch[82-93]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`openinference-streaming.patch` adds Enum handling during attribute normalization, but it only unwraps when the attribute value itself is an `Enum`. If the value is a list/tuple containing Enums (e.g. `[MyEnum.A, MyEnum.B]`), the code will fall through to `str(value)`, converting an array attribute into a string.

### Issue Context
OpenTelemetry attributes support primitive scalars and arrays of primitives. The new Enum logic should also unwrap Enum elements inside arrays/tuples before validating/coercing.

### Fix Focus Areas
- openinference-streaming.patch[82-93]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Uppercase spanId won't scroll ✓ Resolved 🐞 Bug ≡ Correctness

Description

parseTraceHash() accepts uppercase hex span/trace IDs (case-insensitive regex) but returns them
without normalization, and the deep-link scroll uses `document.getElementById('span-' +
targetSpanId)`. If rendered span row IDs use a different casing (commonly lowercase from API/span
data), URLs containing uppercase IDs won’t match and the scroll/highlight will fail.

Code

trace_server/static/app.js[R95-97]

+  const isTraceId = s => /^[0-9a-f]{32}$/i.test(s);
+  const isSpanId = s => /^[0-9a-f]{16}$/i.test(s);
+

Relevance

●●● Strong

Small deterministic fix (normalize hex IDs) to prevent deep-link lookup mismatch; similar
case-insensitive handling accepted before.

PR-#655

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The parser explicitly accepts mixed-case IDs (/i) and returns them as-is. The deep-linking logic
then uses targetSpanId directly in getElementById('span-' + targetSpanId), while span row
elements are identified by 'span-' + span.span_id (coming from span data), so a casing mismatch
causes lookup failure.

trace_server/static/app.js[88-125]
trace_server/static/app.js[586-602]
trace_server/static/app.js[856-860]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`parseTraceHash()` validates span/trace IDs using case-insensitive regexes but does not canonicalize the returned IDs. Deep-link lookup uses the raw `targetSpanId` as part of a DOM element ID, which is typically constructed from `span.span_id` values.

### Issue Context
To make deep links robust, normalize `traceId` and `spanId` to a consistent casing (e.g. lowercase) when parsing (and/or before DOM lookup / API calls).

### Fix Focus Areas
- trace_server/static/app.js[88-125]
- trace_server/static/app.js[586-602]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Guard ignores issue ✓ Resolved 🐞 Bug ≡ Correctness

Description

The new stale-response guards in renderTraceDetail()/pollNewSpans() only compare
state.currentTraceId, but the underlying request is also scoped by issue, so a response for a
different issue can still be applied if it shares the same traceId. This can overwrite state.spans
and render the wrong spans after fast navigation between issues that reference the same trace_id.

Code

trace_server/static/app.js[R527-528]

    const data = await api.spans(issue, {traceId: traceId});
+    if (state.view !== 'trace' || state.currentTraceId !== traceId) return;

Relevance

●●● Strong

Correctness race: team commonly accepts tightening guards to prevent cross-context async/concurrency
state corruption.

PR-#700
PR-#657

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The code added a stale-response guard but it only checks currentTraceId; however, the request is
made for a specific issue, and the backend data model explicitly allows a single trace_id to be
linked to multiple issues, so traceId alone is insufficient to disambiguate navigation races.

trace_server/static/app.js[312-318]
trace_server/static/app.js[523-531]
trace_server/static/app.js[617-621]
trace_server/server.py[12-14]
trace_server/server.py[146-151]
trace_server/server.py[527-540]
trace_server/server.py[681-689]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The trace view’s new async “stale response” guards only check `state.view` and `state.currentTraceId`, but the fetch is performed against `/traces/<issue>?trace_id=...`, so `issue` must also match to safely apply results.

### Issue Context
A single `trace_id` can be associated with multiple Jira issues (see `span_issues` and `query_recent_traces()` returning multiple issues per trace). If a user navigates quickly between two issues that both reference the same trace, an older response can still pass the current guard and overwrite the UI.

### Fix Focus Areas
- trace_server/static/app.js[523-531]
- trace_server/static/app.js[611-614]
- trace_server/static/app.js[617-621]
- trace_server/static/app.js[588-600]

### Suggested fix
1. In `renderTraceDetail()`, change the post-fetch guard to also require `state.currentIssue === issue` (capture `expectedIssue = issue` similarly to `expectedTraceId` if you prefer).
2. Apply the same issue+trace guard in the `catch` block and in `pollNewSpans()`.
3. In the `requestAnimationFrame` callback for `targetSpanId`, also ensure `state.currentIssue` matches before scrolling/highlighting.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

View more (5)

4. Jump links wrong span ✓ Resolved 🐞 Bug ≡ Correctness

Description

The jump-to-bottom button writes a spanId to the URL using state.spans[state.spans.length - 1],
but state.spans is assigned from the API response without sorting on initial load, so the last
array element is not guaranteed to be the bottom-most rendered span. This can produce a valid URL
that, on reload/share, scrolls/highlights a different span than the one at the bottom of the page.

Code

trace_server/static/app.js[R1285-1288]

+          if (parsed && state.spans.length > 0) {
+            const lastSpanId = state.spans[state.spans.length - 1].span_id;
+            const newHash = '#/trace/' + encodeURIComponent(parsed.issue) + '/' + parsed.traceId + '/' + lastSpanId;
+            history.replaceState(null, '', newHash);

Relevance

●●● Strong

Correctness bug in new linkable-span feature; team likely fixes deep-link target deterministically.

PR-#699
PR-#718

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new jump button logic selects the last element of state.spans, but on initial load
state.spans is taken directly from the API without sorting; meanwhile the UI order is driven by
buildSpanTree, which explicitly sorts nodes by start_time. Therefore, the array's last element
can differ from the last rendered row, leading to incorrect deep links.

trace_server/static/app.js[1278-1293]
trace_server/static/app.js[523-531]
trace_server/static/app.js[700-784]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The jump-to-bottom handler updates the URL using `state.spans[state.spans.length - 1].span_id`, but `state.spans` is not guaranteed to be ordered the same as the rendered span list. This can generate deep links that scroll to the wrong span on page load.

### Issue Context
- On initial trace load, `state.spans` is assigned directly from the API response (no sort).
- Rendering uses `buildSpanTree(...)` which sorts roots/children by `start_time`, so visual order can differ from the raw API array.
- The new feature specifically aims to make spans linkable; writing the wrong spanId undermines this.

### Fix Focus Areas
- trace_server/static/app.js[1282-1290]

### Suggested fix
In the jump button `onClick`, derive the target spanId from the rendered DOM (or from the same ordering used for rendering), e.g.:
- Find the last rendered `.span-row` (e.g., `document.querySelector('.span-list .span-row:last-child')`), parse its id (`span-<id>`), and write that id into the URL.
- Keep the existing guards (`parsed` and `state.spans.length > 0`) and add a guard if the DOM query returns null.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

5. decodeURIComponent can crash routing ✓ Resolved 🐞 Bug ☼ Reliability

Description

parseTraceHash() calls decodeURIComponent() on the hash-derived issue segment without guarding
against URIError. A malformed percent-encoded hash can throw and break routing (and span/sidebar
click handlers that call parseTraceHash(location.hash)).

Code

trace_server/static/app.js[R107-110]

+    spanId = parts.pop();
+    traceId = parts.pop();
+    issue = decodeURIComponent(parts.join('/'));
+  } else if (isTraceId(lastPart)) {

Relevance

●●● Strong

Team often accepts defensive parsing/error-guards to prevent crashes from malformed inputs.

PR-#450
PR-#488

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new helper decodes URL components directly and is called from route(); without a try/catch,
malformed hashes can throw and abort route().

trace_server/static/app.js[88-119]
trace_server/static/app.js[293-313]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`decodeURIComponent()` can throw `URIError` on malformed percent-encoding, and `parseTraceHash()` currently does not catch it.

### Issue Context
`parseTraceHash(location.hash)` is invoked from routing and multiple click handlers; a thrown exception can prevent navigation/highlighting and leave the app in a broken state until reload.

### Fix Focus Areas
- trace_server/static/app.js[88-119]
- trace_server/static/app.js[306-313]

### Suggested fix
- Wrap the `decodeURIComponent(...)` calls in a `try/catch` inside `parseTraceHash()`.
- On `URIError`, return `null` (or a structured error) so callers can fall back gracefully.
- Optionally, log to console for debugging (without throwing).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

6. Blank screen on invalid trace ✓ Resolved 🐞 Bug ≡ Correctness

Description

route() clears the app container, but when the hash starts with "#/trace/" and parseTraceHash()
returns null, it renders nothing and leaves state.view/currentTraceId potentially stale. This can
strand users on an empty page for malformed/stale trace links.

Code

trace_server/static/app.js[R307-310]

+    const parsed = parseTraceHash(hash);
+    if (parsed) {
+      state.view = 'trace';
+      state.currentIssue = parsed.issue;

Relevance

●●● Strong

Graceful fallback on invalid/partial inputs aligns with prior accepted robustness fixes in
routing/workflow paths.

PR-#555
PR-#450

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

route() clears the UI (app.innerHTML = '') before the trace branch, but only sets view/renders when
parseTraceHash succeeds; otherwise it does nothing, leaving an empty app and potentially stale
state.

trace_server/static/app.js[293-325]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
When `location.hash` begins with `#/trace/` but `parseTraceHash()` returns `null`, the router has already cleared `#app` and stopped polling, but does not render any view or error.

### Issue Context
This is a regression introduced by the new conditional `if (parsed) { ... }` block.

### Fix Focus Areas
- trace_server/static/app.js[293-325]

### Suggested fix
- Add an `else` branch for the `#/trace/` route to either:
 - render an error banner (e.g., “Invalid trace URL”), and/or
 - fall back to the default `recent` view (`state.view='recent'` + `renderRecent(app)`), and clear `state.currentIssue/currentTraceId`.
- Ensure `updateNav()` reflects the chosen fallback view.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

7. Ambiguous spanId parsing ✓ Resolved 🐞 Bug ≡ Correctness

Description

route() (and the span/sidebar/jump handlers) infer spanId presence purely from parts.length >= 3,
which mis-parses hashes where the issue contains an unencoded '/' and there is no spanId (e.g.
#/trace/org/repo/<traceId> becomes issue=org, traceId=repo, spanId=<traceId>). This is a
regression versus the prior behavior and can break manual/legacy deep links and also cause click
handlers to rewrite the URL to an incorrect trace.

Code

trace_server/static/app.js[R274-277]

    const parts = hash.slice(8).split('/');
+    const spanId = parts.length >= 3 ? parts.pop() : null;
    const traceId = parts.pop();
    const issue = decodeURIComponent(parts.join('/'));

Relevance

●●● Strong

Correctness regression in URL parsing can break legacy/manual deep links; likely they’ll tighten
spanId detection.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new logic pops a spanId whenever there are 3+ segments, which is correct only if the issue is
encoded into a single path segment. The codebase itself indicates issues may contain '/', since it
encodes issues when generating trace URLs; unencoded slash-containing issues are therefore a
realistic input for manual/legacy URLs and become ambiguous under the new heuristic.

trace_server/static/app.js[273-281]
trace_server/static/app.js[432-440]
trace_server/static/app.js[816-830]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The trace router and several click handlers decide whether a spanId is present by checking `parts.length >= 3` and popping the last segment as `spanId`. This breaks hashes where the issue portion contains one or more unencoded `/` characters and the URL has no spanId (3+ segments).

### Issue Context
The app generally generates trace links with `encodeURIComponent(issue)`, but users may still have old/manual bookmarks or external links with unencoded slashes in the issue. With the new optional `/<spanId>` suffix, segment-count parsing becomes ambiguous and can mis-route.

### Fix Focus Areas
- trace_server/static/app.js[273-281]
- trace_server/static/app.js[824-833]
- trace_server/static/app.js[1157-1164]
- trace_server/static/app.js[1245-1253]

### Implementation notes
Use an unambiguous rule to detect presence of spanId/traceId, e.g.:
- Treat the last segment as `spanId` only if it matches OTEL span-id format (typically 16 lowercase hex) *and* the previous segment matches trace-id format (typically 32 lowercase hex).
- Otherwise, treat the last segment as `traceId` and `spanId = null`.
Apply the same parsing helper in route() and in the onClick handlers to avoid divergence.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

8. Unguarded span scroll callback ✓ Resolved 🐞 Bug ☼ Reliability

Description

renderTraceDetail() schedules a requestAnimationFrame scroll/highlight for targetSpanId
without re-checking state.view/state.currentTraceId inside the callback. If the user navigates
to another trace/view before the callback runs, it can scroll/highlight the wrong element (or leave
global autoScroll disabled) in the new view.

Code

trace_server/static/app.js[R541-544]

+    if (targetSpanId) {
+      autoScroll = false;
+      requestAnimationFrame(() => {
+        const target = document.getElementById('span-' + targetSpanId);

Relevance

●●● Strong

Team often accepts async/race-condition hardening; adding state/view guard in deferred callback is
low-risk reliability fix.

PR-#675
PR-#540

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The function guards the async fetch result and error path with view/trace checks, but the new RAF
block that scrolls/highlights lacks those checks and will run even if the route changes after
scheduling.

trace_server/static/app.js[482-484]
trace_server/static/app.js[541-555]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The new `requestAnimationFrame` callback in `renderTraceDetail()` performs DOM operations and updates global state (`autoScroll`, `currentHighlightedSpan`) without confirming the app is still on the same trace.

### Issue Context
Async fetch paths already guard against stale updates (`if (state.view !== 'trace' || state.currentTraceId !== traceId) return;`), but that guard does not apply to the later RAF callback.

### Fix Focus Areas
- trace_server/static/app.js[541-555]

### Suggested fix
- Capture `const expectedTraceId = traceId;` (and optionally `expectedView = 'trace'`) and add a guard inside the RAF callback:
 - `if (state.view !== 'trace' || state.currentTraceId !== expectedTraceId) return;`
- Optionally defer `autoScroll = false` until after the guard passes (inside the callback) to avoid leaking stale state across navigation.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

9. Invalid trace URL unstyled ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

When parseTraceHash() fails, route() renders an "Invalid trace URL" message with `className:
'error', but the stylesheet defines .error-banner and no .error` rule. The new invalid-trace
message is therefore likely to appear unstyled/inconsistent with other errors.

Code

trace_server/static/app.js[324]

+      app.appendChild(el('div', {className: 'error'}, 'Invalid trace URL'));

Relevance

●●● Strong

Trivial UX/style consistency fix: use existing error styling class; usually accepted for user-facing
polish.

PR-#651

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The invalid-trace route branch uses className: 'error'. The CSS file defines styling for
.error-banner but does not define .error, so this message won’t get the intended error styling.

trace_server/static/app.js[312-326]
trace_server/static/style.css[492-499]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The invalid-trace URL path renders an error message with a CSS class (`error`) that is not defined in `style.css`, unlike other error messages which use `error-banner`.

### Issue Context
This is an error path added to avoid a blank screen; using the consistent class keeps error presentation uniform.

### Fix Focus Areas
- trace_server/static/app.js[319-326]
- trace_server/static/style.css[492-499]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

10. Auto-scroll disabled without target ✓ Resolved 🐞 Bug ≡ Correctness

Description

When a spanId is provided in the hash but no element matches it, renderTraceDetail() still sets
autoScroll=false before checking whether the target exists. This can unexpectedly disable live
auto-follow behavior for deep links with stale/invalid span IDs.

Code

trace_server/static/app.js[R576-579]

+        if (state.view !== 'trace' || state.currentTraceId !== expectedTraceId) return;
+        autoScroll = false;
+        const target = document.getElementById('span-' + targetSpanId);
+        if (target) {

Relevance

●●● Strong

Small, local UX correctness fix; avoids disabling behavior on invalid target, low risk.

PR-#584
PR-#555

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

autoScroll is set to false before the code checks whether the target element exists, so an invalid
spanId still turns off auto-follow.

trace_server/static/app.js[571-589]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The deep-link highlight code disables `autoScroll` unconditionally, even if the target span element isn’t found.

### Issue Context
This affects `#/trace/<issue>/<traceId>/<spanId>` links where the spanId is stale/incorrect.

### Fix Focus Areas
- trace_server/static/app.js[571-589]

### Suggested fix
- Move `autoScroll = false` inside the `if (target) { ... }` block, or
- If `target` is missing, keep `autoScroll` unchanged (or explicitly restore it) and consider clearing the spanId from the URL or showing a small “span not found” notice.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Context used

✅ Compliance rules (platform): 7 rules

_{To customize comments, go to the Qodo configuration screen, or learn more in the docs.}

[nforro]

/agentic_review

[qodo-for-packit]

Code review by qodo was updated up to the latest commit 34623dd

[nforro]

/agentic_review

[qodo-for-packit]

Code review by qodo was updated up to the latest commit 67653ce

[nforro]

/agentic_review

[qodo-for-packit]

Code review by qodo was updated up to the latest commit 20b2c10

[nforro]

/agentic_review

[qodo-for-packit]

Code review by qodo was updated up to the latest commit 3629362

[nforro]

/agentic_review

[qodo-for-packit]

Code review by qodo was updated up to the latest commit b207808

[nforro]

/agentic_review

[qodo-for-packit]

Code review by qodo was updated up to the latest commit 8924301