Skip to content

[Agent Traces] Add Sessions tab - #12846

Open
ps48 wants to merge 9 commits into
opensearch-project:mainfrom
ps48:feat/agent-traces-sessions
Open

ps48 wants to merge 9 commits into
opensearch-project:mainfrom
ps48:feat/agent-traces-sessions

Conversation

@ps48

@ps48 ps48 commented Sep 29, 2026 •

Copy link
Copy Markdown
Member

Description

Adds Sessions to Agent Traces. A session is the set of traces that share gen_ai.conversation.id (OTel GenAI semantic conventions; Data Prepper maps OpenInference/ADOT session.id to it at ingest).

Sessions tab

  • Columns: Time (session start), Session ID, First Message, Last Message, Duration, User ID (hidden when no session has user.id), Total Traces, Total Tokens. Same table markup and typography as Traces/Spans, client-side sort, lazy rendering on scroll.
  • The query and time range select which sessions appear; per-session totals (traces, duration, tokens, first/last message) always cover the whole session, so the list and the session flyout agree for sessions that started before the range.
  • The list loads up to 100 sessions and shows "100 of N sessions" when there are more.
  • Row-level filters apply in order, including filters after eval (where, eval, parse, grok, regex, fillnull). Commands that reshape rows (stats, head, fields, dedup, ...) are not applied here; a notice lists them and points to the Traces or Visualization tab.
  • An empty list under a filter says no sessions match the query (instead of the gen_ai.conversation.id setup hint).
  • Metrics bar: new Total Sessions stat.

Fields panel on Sessions

  • Facets (service, agent name, status) count distinct sessions per value from a stats query under the user's query, instead of spans from a 500-row sample. A session counts toward a value when any of its spans that carry gen_ai.conversation.id has it, which is also how filter-for narrows the list.
  • The Sessions table has fixed columns, so the panel hides "Selected" and the add/remove column actions on this tab (new optional facetBuckets$ / fixedColumns on TabDefinition).

Session flyout

  • Header: full session id (ellipsis only when narrow) with copy, Total Duration, Total Tokens, Total Traces.
  • Trace list with per-trace latency and tokens; clicking a trace focuses it in the conversation, the chevron opens the existing trace flyout.
  • Session Conversation: one turn per trace, labeled with the message roles from the data (User / Assistant, and System / Tool when present), next/previous trace arrows, Trace #n links into the trace flyout. Only the conversation scrolls; headers stay pinned and aligned.
  • View All Traces: Traces and Spans tables scoped to the session.

Message rendering (shared with the trace flyout)

  • Input/Output previews parse gen_ai.input.messages / gen_ai.output.messages per the semconv JSON schema ({role, parts[]}), with placeholders for non-text parts ([tool_call: name], [image], [reasoning]) and a legacy fallback.
  • Formatted (markdown) / JSON toggle with copy buttons.

Side nav

  • Agent monitoring now lists Traces and Sessions (navTicketing icon). Spans stays a tab next to Traces; the agentTraces/spans app is still registered so existing links keep working.

Dashboards

  • Saved searches from the Traces, Spans and Sessions tabs now embed as agent tables instead of raw span documents (previously every span, with Status/Latency/Tokens/Input/Output empty). Traces runs the root-span query, Spans all gen_ai spans, and Sessions the same queries as the Sessions tab. Rows open the trace or session in Agent Traces with the dashboard time range.

Also fixed (affects Traces too)

  • Metrics bar tokens/latency now honor filters placed after eval (same helper as Sessions).
  • Plural forms in the count messages ("1 traces" -> "1 trace").

Issues Resolved

Screenshot

Testing the changes

  1. Ingest agent traces whose spans carry gen_ai.conversation.id (for example the observability-stack multi-agent trip planner with the multi-turn canary).
  2. Use Sessions in the side nav. Check columns, sorting, scroll loading, and "N of M sessions" with a wide time range.
  3. Filter with | where serviceName = 'weather-agent', then | eval x = 1 | where x = 2 (no sessions match), then add | head 5 (notice lists head).
  4. Use a facet filter-for; facet counts are distinct sessions.
  5. Open a session: header totals, trace list focus, arrows, Trace #n into the trace flyout, Formatted/JSON, copy, View All Traces.
  6. Save a search on the Sessions tab and one on the Traces tab, add both to a dashboard, and click a row in each panel.
  7. With no gen_ai.conversation.id data, the tab shows an empty state naming the field.

Unit tests: yarn test:jest src/plugins/agent_traces (189 suites, 1995 tests pass locally).

Check List

  • All tests pass
    • yarn test:jest
    • yarn test:jest_integration
  • New functionality includes testing.
  • New functionality has been documented.
  • Commits are signed per the DCO using --signoff

Sessions group traces by gen_ai.conversation.id (OTel GenAI semconv):
- Sessions tab with first/last message, duration, trace count, tokens;
  Total Sessions metric
- Session flyout with trace list, conversation navigation, and a
  View All Traces drill-in that opens the existing trace flyout
- Input/Output previews parse gen_ai.*.messages per the semconv JSON
  schema (last user message, assistant text, part placeholders), with a
  gen_ai.tool.call.arguments/result fallback, across all tables

Signed-off-by: Shenoy Pratik Gurudatt <4348487+ps48@users.noreply.github.com>
- Sessions list and View All Traces use the Traces/Spans DataTable
  markup: same header, typography, sort and infinite scroll (no pager),
  with the open session highlighted
- DataTableInfoBar supports a session entity
- Session flyout: only the conversation scrolls; the trace list and the
  conversation header with prev/next arrows stay pinned
- Focusing a trace highlights its turn; the list chevron opens the trace

Signed-off-by: Shenoy Pratik Gurudatt <4348487+ps48@users.noreply.github.com>
- TabDefinition gains optional facetFields; groupFields/DiscoverSidebar
  take the active tab's list (defaults unchanged for Traces/Spans)
- Sessions shares the Traces root-span prepareQuery (same cache key) so
  the fields panel has data, with service/agent/status facets
- Facet filters select which sessions are listed; each session's stats
  are computed unfiltered so trace counts and durations stay complete

Signed-off-by: Shenoy Pratik Gurudatt <4348487+ps48@users.noreply.github.com>
- Shared MessageContent view: Formatted renders semconv messages as
  role-labeled markdown (EuiMarkdownFormat); JSON shows the attribute
  pretty-printed; copy follows the active view
- Trace flyout Input/Output gets the toggle and per-section copy, with a
  gen_ai.tool.call.* fallback for execute_tool spans
- Session conversation gets a global toggle and per-message copy

Signed-off-by: Shenoy Pratik Gurudatt <4348487+ps48@users.noreply.github.com>
…d panels

- Title shows the full session id (ellipsis only when the flyout is narrow).
- Label conversation messages with their OTel GenAI roles (User/Assistant,
  System/Tool when present) instead of Human/AI.
- Remove horizontal scroll in the conversation (flex gutter overflow).
- Scroll only the conversation column and land each turn below the sticky
  header, so the Trace #n link stays visible.
- Align the Trace list and Session Conversation headers.

Signed-off-by: Shenoy Pratik Gurudatt <4348487+ps48@users.noreply.github.com>
Signed-off-by: Shenoy Pratik Gurudatt <4348487+ps48@users.noreply.github.com>
@github-actions

github-actions Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

(Review updated until commit 0e43880)

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 No security concerns identified
✅ No TODO sections
🔀 Multiple PR themes

Sub-PR theme: GenAI message preview + shared message content view

Relevant files:

  • src/plugins/agent_traces/public/application/pages/traces/flyout/message_content_view.tsx
  • src/plugins/agent_traces/public/application/pages/traces/flyout/message_content_view.scss
  • src/plugins/agent_traces/public/application/pages/traces/flyout/message_content_view.test.tsx
  • src/plugins/agent_traces/public/application/pages/traces/flyout/flyout_detail_panel.tsx
  • src/plugins/agent_traces/public/application/pages/traces/hooks/genai_message_preview.ts
  • src/plugins/agent_traces/public/application/pages/traces/hooks/genai_message_preview.test.ts
  • src/plugins/agent_traces/public/components/data_table/table_cell/trace_utils/trace_utils.tsx

Sub-PR theme: Fields panel tab customization (facetFields, facetBuckets$, fixedColumns)

Relevant files:

  • src/plugins/agent_traces/public/components/fields_selector/discover_field.tsx
  • src/plugins/agent_traces/public/components/fields_selector/discover_sidebar.tsx
  • src/plugins/agent_traces/public/components/fields_selector/discover_sidebar.test.tsx
  • src/plugins/agent_traces/public/components/fields_selector/field_list.tsx
  • src/plugins/agent_traces/public/components/fields_selector/fields_selector_panel.tsx
  • src/plugins/agent_traces/public/components/fields_selector/lib/group_fields.ts
  • src/plugins/agent_traces/public/components/fields_selector/lib/group_fields.test.ts
  • src/plugins/agent_traces/public/services/tab_registry/tab_registry_service.ts

Sub-PR theme: Sessions tab implementation and metrics

Relevant files:

  • src/plugins/agent_traces/common/index.ts
  • src/plugins/agent_traces/public/application/pages/sessions/**
  • src/plugins/agent_traces/public/application/register_tabs.ts
  • src/plugins/agent_traces/public/application/register_tabs.test.ts
  • src/plugins/agent_traces/public/application/pages/traces/hooks/use_trace_metrics.ts
  • src/plugins/agent_traces/public/application/pages/traces/trace_metrics_bar.tsx
  • src/plugins/agent_traces/public/application/pages/traces/trace_metrics_bar.test.tsx
  • src/plugins/agent_traces/public/application/pages/traces/table_shared.tsx

⚡ Recommended focus areas for review

Stale facet updates

The facet fetch Promise.all uses requestId === requestIdRef.current before publishing to sessionFacetBuckets$, but requestId is captured at call time and requestIdRef.current is only bumped on subsequent calls to fetchSessions. If the tab unmounts while facets are in-flight, the cleanup effect sets the subject to null, then the resolved facet promise will overwrite it because the requestId still matches. Consider tracking a mounted flag or checking against a cancellation token that flips on unmount.

void Promise.all(
  SESSION_FACET_FIELDS.map(async (field) => {
    try {
      const response = await pplService.executeQuery(
        datasetParam,
        buildSessionFacetQuery(whereQuery, field)
      );
      return [field, parseFacetBuckets(pplResponseToRecords(response), field)] as const;
    } catch {
      return [field, []] as const; // e.g. the field is not mapped in this index
    }
  })
).then((entries) => {
  if (requestId === requestIdRef.current) {
    sessionFacetBuckets$.next(Object.fromEntries(entries));
  }
});
Timezone bug in duration

toMs uses moment.utc(ts) on PPL timestamps like '2026-09-28 22:17:18.659', which have no timezone. If ingested timestamps are actually local (or in the cluster tz), interpreting them as UTC will still produce a valid start/end difference because both bounds are parsed the same way — but any comparison against wall-clock times or the row startTime display (which uses the user's tz via formatTs) will be inconsistent. Confirm ingest timestamps are UTC; otherwise duration/time-link semantics may diverge.

const toMs = (ts: string): number => {
  const m = moment.utc(ts);
  return m.isValid() ? m.valueOf() : NaN;
};
Unbounded IN list

buildSessionStatsQuery, buildTraceSessionMapQuery, buildRootSpansQuery, and buildSessionSpansQuery embed all ids into a single PPL in (...) list. With SESSIONS_PAGE_LIMIT=100 sessions each having many traces, buildRootSpansQuery can receive up to SESSION_ROOTS_LIMIT=2000 trace ids in one string, producing very large queries that may exceed PPL/backend query length limits. Consider chunking or capping the input.

/** Full (unfiltered) stats for the given sessions: trace count and time bounds. */
export const buildSessionStatsQuery = (source: string, sessionIds: string[]): string =>
  `${source} | where ${SESSION_FIELD_PPL} in (${inList(
    sessionIds
  )}) | stats distinct_count(traceId) as total_traces, min(startTime) as start_time, max(endTime) as end_time by ${SESSION_FIELD_PPL} | sort - start_time`;

/** Maps each trace id to its session id. Any span in a trace may carry the session id. */
export const buildTraceSessionMapQuery = (source: string, sessionIds: string[]): string =>
  `${source} | where ${SESSION_FIELD_PPL} in (${inList(
    sessionIds
  )}) | dedup traceId | fields traceId, ${SESSION_FIELD_PPL} | head ${SESSION_ROOTS_LIMIT}`;

/** Root spans of the given traces (they carry per-trace input, output and token totals). */
export const buildRootSpansQuery = (source: string, traceIds: string[]): string =>
  `${source} | where parentSpanId = "" and traceId in (${inList(
    traceIds
  )}) | sort startTime | head ${SESSION_ROOTS_LIMIT}`;

/** Every span in the given traces, for the session detail flyout. */
export const buildSessionSpansQuery = (
  source: string,
  traceIds: string[],
  limit = SESSION_SPANS_LIMIT
): string => `${source} | where traceId in (${inList(traceIds)}) | head ${limit}`;
Unstable turn ref callback

The turnRef prop passed to ConversationTurn is an inline arrow (el) => { turnRefs.current[i] = el; } recreated every render, so React will invoke it with null and then the element on every parent re-render (e.g., when focusedIndex changes). This is generally acceptable but can cause the ref array to be transiently emptied right when focusTrace reads turnRefs.current[index]. Consider using a stable ref-setter factory to avoid intermittent scroll-into-view failures.

{traces.map((trace, i) => (
  <ConversationTurn
    key={trace.traceId}
    index={i + 1}
    trace={trace}
    focused={i === focusedIndex}
    mode={messageMode}
    turnRef={(el) => {
      turnRefs.current[i] = el;
    }}
    onOpenTrace={() => openTrace(trace.root)}
  />
))}

@github-actions

github-actions Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Latest suggestions up to 5346cb9

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Parse PPL timestamps with explicit formats

moment.utc('') returns an invalid moment but moment.utc(undefined) returns "now";
more importantly, passing a plain string without a format uses the deprecated
fallback parser and may misinterpret non-ISO timestamps. Since PPL emits timestamps
like '2026-09-28 22:17:18.659' (space separator, no timezone), pass an explicit
format to avoid deprecation warnings and cross-locale ambiguity.

src/plugins/agent_traces/public/application/pages/sessions/session_utils.ts [153-156]

 const toMs = (ts: string): number => {
-  const m = moment.utc(ts);
+  if (!ts) return NaN;
+  const m = moment.utc(ts, ['YYYY-MM-DD HH:mm:ss.SSS', 'YYYY-MM-DD HH:mm:ss', moment.ISO_8601], true);
   return m.isValid() ? m.valueOf() : NaN;
 };
Suggestion importance[1-10]: 4

__

Why: Using an explicit format avoids moment's deprecated fallback parser warnings and improves robustness for non-ISO timestamps, though the current code works for the tested PPL formats.

Low
Distinguish unavailable sessions from zero

When the session field is unmapped and the catch runs, sessionStats.total_sessions
becomes undefined, so sessionStats.total_sessions ?? null yields null — this is the
intended "unavailable" signal. However when the query succeeds but returns no
matches, parseStatsResponse may return { total_sessions: 0 } (or similar). Confirm
the failure path returns something that yields null (not 0) so UI can distinguish
"no sessions" from "not supported"; consider returning { total_sessions: null }
explicitly on error.

src/plugins/agent_traces/public/application/pages/traces/hooks/use_trace_metrics.ts [106-115]

 (async () => {
   try {
     const sessionField = `\`${AGENT_TRACES_SESSION_ID_FIELD}\``;
     const query = `${sourceOnlyQuery} | where isnotnull(${sessionField}) | stats distinct_count(${sessionField}) as total_sessions`;
     const response = await pplService.executeQuery(datasetParam, query);
     return parseStatsResponse(response);
   } catch {
-    return {} as Record<string, any>;
+    return { total_sessions: null } as Record<string, any>;
   }
 })(),
Suggestion importance[1-10]: 3

__

Why: The existing code already returns {} on error, which yields null via ?? null. On success with zero matches, showing 0 is arguably correct. The suggestion offers a minor semantic clarification but limited practical impact.

Low
Possible issue
Ensure IN-list values are quoted correctly

escapePPLValue likely returns an escaped string without surrounding quotes, but the
query builders use it directly inside an in (...) clause where PPL requires quoted
string literals (e.g. in ("a"b", "c") as asserted by the tests). If escapePPLValue
doesn't add the surrounding double quotes, the generated PPL will be syntactically
invalid. Verify the helper's behavior and wrap the escaped value in double quotes if
needed.

src/plugins/agent_traces/public/application/pages/sessions/session_utils.ts [81]

-const inList = (values: string[]): string => values.map((v) => escapePPLValue(v)).join(', ');
+const inList = (values: string[]): string =>
+  values.map((v) => `"${escapePPLValue(v)}"`).join(', ');
Suggestion importance[1-10]: 2

__

Why: The suggestion is speculative: escapePPLValue is imported from an existing helper, and the tests explicitly assert the generated queries include quoted values (in ("a\"b", "c")), which implies the helper already adds quotes. The suggestion could double-quote strings if applied.

Low

Previous suggestions

Suggestions up to commit 0e43880
CategorySuggestion                                                                                                                                    Impact
Possible issue
Extract source command from unfiltered base query

getSourceCommand is applied to whereQuery (which already excludes tail commands but
retains where filters), so source will actually be the full source = ... command
only when there are no where clauses. When filters exist, source will still contain
them, causing buildSessionStatsQuery, buildTraceSessionMapQuery, and
buildRootSpansQuery to be filtered by the user's where clauses — contradicting the
documented intent that stats be "unfiltered." Call getSourceCommand(baseQueryString)
instead.

src/plugins/agent_traces/public/application/pages/sessions/hooks/use_sessions.ts [66-67]

 const { whereQuery } = splitPplWhereAndTail(baseQueryString);
-const source = getSourceCommand(whereQuery);
+const source = getSourceCommand(baseQueryString);
Suggestion importance[1-10]: 8

__

Why: Valid concern: getSourceCommand extracts everything before the first |, but whereQuery from splitPplWhereAndTail typically already contains source | where ..., so the extracted source would exclude filters correctly. However, if whereQuery contains no pipe, the whole where clause becomes the "source", potentially causing issues. Using baseQueryString is safer and matches the documented intent.

Medium
General
Avoid refetching on every render via formatTs

The effect depends on traceIds via the derived key, but the eslint-disabled deps
array uses key while the effect body reads traceIds directly. If two different
traceIds arrays produce the same joined string in a different order, key will differ
and re-run correctly, but stale closures over traceIds are still safe. However,
formatTs is included as a dep — if the parent passes a new function each render,
this effect will refetch on every render. Consider memoizing formatTs at the call
site or omitting it from deps.

src/plugins/agent_traces/public/application/pages/sessions/hooks/use_session_detail.ts [45-48]

+const key = traceIds ? traceIds.join(',') : '';
 
+useEffect(() => {
+  if (!traceIds || traceIds.length === 0 || !pplService || !datasetParam || !baseQueryString) {
Suggestion importance[1-10]: 4

__

Why: Valid concern about formatTs potentially causing re-fetches on every render, but the parent (sessions_tab.tsx) already wraps formatTs in useCallback. The improved_code is identical to the existing_code, providing no actionable change.

Low
Guard trace map query against null traceIds

dedup traceId before head can drop trace-to-session mappings if the same traceId
appears earlier without a session id (e.g., some spans of the trace lack the
attribute). Since the query already filters where ${SESSION_FIELD_PPL} in (...),
this is safe, but consider adding | where isnotnull(traceId) to protect against null
traceIds and place head after dedup as done. Also, SESSION_ROOTS_LIMIT (2000) may be
too low if a session contains many traces across many sessions being listed
simultaneously.

src/plugins/agent_traces/public/application/pages/sessions/session_utils.ts [129-132]

 export const buildTraceSessionMapQuery = (source: string, sessionIds: string[]): string =>
   `${source} | where ${SESSION_FIELD_PPL} in (${inList(
     sessionIds
-  )}) | dedup traceId | fields traceId, ${SESSION_FIELD_PPL} | head ${SESSION_ROOTS_LIMIT}`;
+  )}) | where isnotnull(traceId) | dedup traceId | fields traceId, ${SESSION_FIELD_PPL} | head ${SESSION_ROOTS_LIMIT}`;
Suggestion importance[1-10]: 3

__

Why: Minor defensive improvement. Since spans in OTel traces should always have a traceId, the null check is unlikely to matter in practice.

Low
Verify PPL value quoting for IN lists

escapePPLValue likely returns an already-quoted value (the test expects "a"b",
"c"), but if it does not wrap in quotes, the generated in (...) clause will be
invalid PPL. Verify that escapePPLValue returns quoted strings; otherwise wrap the
result in double quotes explicitly to avoid producing malformed queries for session
ids and trace ids.

src/plugins/agent_traces/public/application/pages/sessions/session_utils.ts [81]

+const inList = (values: string[]): string => values.map((v) => escapePPLValue(v)).join(', ');
 
-
Suggestion importance[1-10]: 2

__

Why: The suggestion only asks to verify existing behavior, and the improved_code is identical to the existing_code. Tests already confirm the escaping works correctly.

Low
Suggestions up to commit f7c44e0
CategorySuggestion                                                                                                                                    Impact
Possible issue
Extract source from base query, not filtered

getSourceCommand is being called on whereQuery (already stripped of tail commands),
but downstream queries like buildSessionStatsQuery use it as a plain source. The
variable naming suggests source should be extracted from baseQueryString (which
contains the source), not from whereQuery. If whereQuery still contains where
clauses, they will incorrectly narrow the "unfiltered" stats query, defeating the
documented intent that "their stats are computed unfiltered".

src/plugins/agent_traces/public/application/pages/sessions/hooks/use_sessions.ts [62-63]

 const { whereQuery } = splitPplWhereAndTail(baseQueryString);
-const source = getSourceCommand(whereQuery);
+const source = getSourceCommand(baseQueryString);
Suggestion importance[1-10]: 8

__

Why: Valid concern: getSourceCommand on whereQuery (which still contains where clauses) means the "unfiltered" stats queries will actually be narrowed by the user's filter, contradicting the documented intent. However, getSourceCommand slices at the first pipe, so it would still return just the source portion — reducing the severity somewhat.

Medium
Ensure IN-list values are quoted strings

The test expect(buildTraceSessionMapQuery('source = x', ['a"b', 'c'])).toContain('in
("a\"b", "c")') asserts that values are wrapped in quotes and escaped. Verify that
escapePPLValue returns a quoted, escaped literal (e.g. "a"b"). If it only escapes
without quoting, the generated PPL will be syntactically invalid (unquoted
identifiers where string literals are expected), producing runtime PPL errors.

src/plugins/agent_traces/public/application/pages/sessions/session_utils.ts [80]

-const inList = (values: string[]): string => values.map((v) => escapePPLValue(v)).join(', ');
+const inList = (values: string[]): string =>
+  values.map((v) => `"${escapePPLValue(v).replace(/"/g, '\\"')}"`).join(', ');
Suggestion importance[1-10]: 3

__

Why: The suggestion is speculative about escapePPLValue's behavior. The tests pass with the current implementation, suggesting escapePPLValue already returns quoted, escaped literals. Low confidence and unverified.

Low
General
Avoid refetching on every parent render

The effect depends on key (a comma-joined string) but reads traceIds inside. If a
parent recomputes traceIds with the same contents but different array identity, key
stays the same and the effect does not re-run — that's fine. However, since formatTs
is included in deps but usually a new function each render, the effect will refetch
on every parent render, causing unnecessary re-requests. Consider memoizing formatTs
at the call site or removing it from the dependency array and reading it via a ref.

src/plugins/agent_traces/public/application/pages/sessions/hooks/use_session_detail.ts [45]

+const formatTsRef = useRef(formatTs);
+formatTsRef.current = formatTs;
+// ...use formatTsRef.current inside the effect; drop formatTs from deps
 const key = traceIds ? traceIds.join(',') : '';
Suggestion importance[1-10]: 5

__

Why: Valid point that formatTs in deps can cause unnecessary refetches if not memoized by the caller. In sessions_tab.tsx, formatTs is wrapped in useCallback, so this is likely not an issue in practice, but the suggestion offers reasonable defensive improvement.

Low
Prevent stale turn refs across sessions

turnRefs.current is never cleaned up when traces shrinks (e.g., switching sessions
with fewer traces). Stale refs from a previous, longer session remain in the array
and could be indexed by focusTrace. Reset the ref array when traces change to avoid
scrolling to a stale/detached element.

src/plugins/agent_traces/public/application/pages/sessions/session_details_flyout.tsx [453-465]

+// near the other hooks:
+useEffect(() => {
+  turnRefs.current = turnRefs.current.slice(0, traces.length);
+}, [traces]);
+// ...
 {traces.map((trace, i) => (
   <ConversationTurn
     key={trace.traceId}
     index={i + 1}
     trace={trace}
     focused={i === focusedIndex}
     mode={messageMode}
     turnRef={(el) => {
       turnRefs.current[i] = el;
     }}
     onOpenTrace={() => openTrace(trace.root)}
   />
 ))}
Suggestion importance[1-10]: 4

__

Why: The flyout is only mounted for one session at a time (unmounted on close), so stale refs across sessions are unlikely. Minor defensive improvement with low real-world impact.

Low
Suggestions up to commit f7c44e0
CategorySuggestion                                                                                                                                    Impact
General
Handle missing roots in trace id list

assembleSessionRows derives traceIds from the roots list, which is bounded by
SESSION_ROOTS_LIMIT and filtered to parentSpanId = "". Sessions whose root spans
exceed the limit — or traces whose root wasn't captured — will have traceIds that
don't match totalTraces from the stats query, causing the flyout to load only a
partial subset of the session's traces without any warning. Consider falling back to
the full trace list from traceToSession when roots are missing.

src/plugins/agent_traces/public/application/pages/sessions/session_utils.ts [199]

+// In assembleSessionRows, build traceIds from traceToSession as a fallback when
+// no root row was returned for a trace, so the flyout still loads all spans.
+const sessionTraceIds = [...traceToSession.entries()]
+  .filter(([, sid]) => sid === s.sessionId)
+  .map(([tid]) => tid);
+const orderedTraceIds = roots.length ? roots.map((r) => r.traceId) : sessionTraceIds;
 
-
Suggestion importance[1-10]: 6

__

Why: Identifies a real edge case where traceIds may be incomplete due to SESSION_ROOTS_LIMIT capping, potentially leading to partial session detail loads. The fallback is reasonable.

Low
Reset loading state on early return

When dependencies are missing the function returns early without clearing loading.
If a prior fetch left loading=true (e.g. dataset was reset), the UI will remain
stuck in the loading state. Reset loading/error state and clear results in this
early-return path.

src/plugins/agent_traces/public/application/pages/sessions/hooks/use_sessions.ts [54-56]

 const fetchSessions = useCallback(async () => {
-  if (!pplService || !datasetParam || !baseQueryString) return;
+  if (!pplService || !datasetParam || !baseQueryString) {
+    setLoading(false);
+    setSessions([]);
+    return;
+  }
   const requestId = ++requestIdRef.current;
Suggestion importance[1-10]: 5

__

Why: Valid minor concern about potentially stuck loading state, but loading is initialized to false and only set true after the guard, so the practical risk is limited.

Low
Clear stale loading and error state

This early-return path doesn't clear a stale loading or error state from a previous
run. If the flyout was previously loading and the effect re-runs with empty inputs,
the UI will still show the spinner or error. Reset those states here.

src/plugins/agent_traces/public/application/pages/sessions/hooks/use_session_detail.ts [48-51]

 if (!traceIds || traceIds.length === 0 || !pplService || !datasetParam || !baseQueryString) {
   setTraces([]);
+  setLoading(false);
+  setError(null);
   return;
 }
Suggestion importance[1-10]: 5

__

Why: Reasonable robustness improvement to reset stale states on early return, though the initial state is already false/null so real-world impact is minor.

Low
Verify session list page size is intentional

The test asserts this function is called as buildMatchingSessionIdsQuery(query, 50),
but use_sessions.ts calls it as buildMatchingSessionIdsQuery(whereQuery) with only
one argument. The default SESSIONS_PAGE_LIMIT (100) will apply there, which is fine,
but confirm the intended limit for the production call — otherwise the sessions list
may silently cap at 100 while tests validate a 50-row cap that never runs.

src/plugins/agent_traces/public/application/pages/sessions/session_utils.ts [86-90]

 export const buildMatchingSessionIdsQuery = (
   whereQuery: string,
-  limit = SESSIONS_PAGE_LIMIT
+  limit: number = SESSIONS_PAGE_LIMIT
 ): string =>
   `${whereQuery} | where isnotnull(${SESSION_FIELD_PPL}) | stats max(endTime) as last_seen by ${SESSION_FIELD_PPL} | sort - last_seen | head ${limit}`;
Suggestion importance[1-10]: 3

__

Why: The suggestion only asks to verify intent; the improved_code is functionally identical to existing_code (only an explicit type annotation added). Low impact.

Low

@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit f7c44e0

…ons fields panel

- Sessions facets count distinct sessions per value (stats query under the
  user's query) instead of fetched root spans from a 500-row sample.
- Tabs can supply facet buckets (facetBuckets$) and declare fixed columns;
  the Sessions tab hides "Selected" and add/remove column actions.
- Keep short Sessions cells (time, ids, counts) on one line in wrap mode.

Signed-off-by: Shenoy Pratik Gurudatt <4348487+ps48@users.noreply.github.com>
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 0e43880

@github-actions

github-actions Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

🔗 Workflow run · commit 5346cb974e66618bcda70a52e6b883ffffe71734

❌ 3 Jest Test Failure(s)

📄 junit-jest-integration-Windows/TEST-Jest Integration Tests.xml

❌ Options authRequired optional User has access to a route if auth mechanism not registered (0.113s)

Jest Integration Tests.src\core\server\http\integration_tests

Error: expect(received).rejects.toThrow(expected)

Expected substring: "socket hang up"
Received message:   "read ECONNRESET"

    at Object.toThrow (D:\a\OpenSearch-Dashboards\OpenSearch-Dashboards\node_modules\expect\build\index.js:2155:20)
    at Object.toThrow (D:\a\OpenSearch-Dashboards\OpenSearch-Dashboards\src\core\server\http\integration_tests\router.test.ts:446:38)
    at processTicksAndRejections (node:internal/process/task_queues:103:5)

❌ applies filter function specified (0.010s)

Jest Integration Tests.src\dev\build\lib\integration_tests

Error: thrown: "Exceeded timeout of 30000 ms for a hook.
Add a timeout value to this test to increase the timeout, if this is a long-running test. See https://jestjs.io/docs/api#testname-fn-timeout."
    at beforeAll (D:\a\OpenSearch-Dashboards\OpenSearch-Dashboards\src\plugins\workspace\server\saved_objects\integration_tests\workspace_ui_settings_wrapper.test.ts:21:3)
    at _dispatchDescribe (D:\a\OpenSearch-Dashboards\OpenSearch-Dashboards\node_modules\jest-circus\build\jestAdapterInit.js:608:26)
    at describe (D:\a\OpenSearch-Dashboards\OpenSearch-Dashboards\node_modules\jest-circus\build\jestAdapterInit.js:576:44)
    at Object.describe (D:\a\OpenSearch-Dashboards\OpenSearch-Dashboards\src\plugins\workspace\server\saved_objects\integration_tests\workspace_ui_settings_wrapper.test.ts:12:1)
    at ModuleExecutor.exec (D:\a\OpenSearch-Dashboards\OpenSearch-Dashboards\node_modules\jest-runtime\build\index.js:2078:26)
    at CjsLoader.loadModule (D:\a\OpenSearch-Dashboards\OpenSearch-Dashboards\node_modules\jest-runtime\build\index.js:323:36)
    at CjsLoader.requireModule (D:\a\OpenSearch-Dashboards\OpenSearch-Dashboards\node_modules\jest-runtime\build\index.js:289:12)
    at Runtime.requireModule (D:\a\OpenSearch-Dashboards\OpenSearch-Dashboards\node_modules\jest-runtime\build\index.js:3657:27)
  … (3 more lines)

❌ workspace ui settings saved object client wrapper should get and update global ui settings when currently not in a workspace (0.000s)

Jest Integration Tests.src\plugins\workspace\server\saved_objects\integration_tests

Error: thrown: "Exceeded timeout of 30000 ms for a hook.
Add a timeout value to this test to increase the timeout, if this is a long-running test. See https://jestjs.io/docs/api#testname-fn-timeout."
    at beforeAll (D:\a\OpenSearch-Dashboards\OpenSearch-Dashboards\src\plugins\workspace\server\saved_objects\integration_tests\workspace_ui_settings_wrapper.test.ts:21:3)
    at _dispatchDescribe (D:\a\OpenSearch-Dashboards\OpenSearch-Dashboards\node_modules\jest-circus\build\jestAdapterInit.js:608:26)
    at describe (D:\a\OpenSearch-Dashboards\OpenSearch-Dashboards\node_modules\jest-circus\build\jestAdapterInit.js:576:44)
    at Object.describe (D:\a\OpenSearch-Dashboards\OpenSearch-Dashboards\src\plugins\workspace\server\saved_objects\integration_tests\workspace_ui_settings_wrapper.test.ts:12:1)
    at ModuleExecutor.exec (D:\a\OpenSearch-Dashboards\OpenSearch-Dashboards\node_modules\jest-runtime\build\index.js:2078:26)
    at CjsLoader.loadModule (D:\a\OpenSearch-Dashboards\OpenSearch-Dashboards\node_modules\jest-runtime\build\index.js:323:36)
    at CjsLoader.requireModule (D:\a\OpenSearch-Dashboards\OpenSearch-Dashboards\node_modules\jest-runtime\build\index.js:289:12)
    at Runtime.requireModule (D:\a\OpenSearch-Dashboards\OpenSearch-Dashboards\node_modules\jest-runtime\build\index.js:3657:27)
  … (3 more lines)

3 failure(s) across 1 suite(s). Full XML reports are in the junit-jest-* artifacts.

Agent monitoring nav now lists Traces and Sessions (navTicketing icon), with the
same new-search / browse-saved popover. Spans stays a tab next to Traces, and the
agentTraces/spans app is still registered so existing links keep working.

Signed-off-by: Shenoy Pratik Gurudatt <4348487+ps48@users.noreply.github.com>
…explain ignored commands

- New quote-aware extractSpanFilterQuery keeps every row-level command (where,
  eval, parse, grok, regex, fillnull) in order. The old split stopped at the first
  non-where command, so '| eval x = 1 | where x = 2' silently dropped the filter
  in the Sessions list, its facets, and the metrics bar.
- Sessions shows a notice listing commands it does not apply (stats, head,
  fields, ...) and points to the Traces or Visualization tab.
- An empty Sessions list under a filter now says no sessions match the query,
  instead of the setup hint about gen_ai.conversation.id.

Signed-off-by: Shenoy Pratik Gurudatt <4348487+ps48@users.noreply.github.com>

This branch has not been deployed

No deployments
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.

1 participant