TraceView - Optimization of queries

[TackAdam]

Description

1. Remove the query request for handleTraceViewRequest(), handleServicesPieChartRequest(), handleSpansGanttRequest() by utilizing the primary payload and parsing it on the UI level. This reduces the number of queries when moving from Trace content to a specific trace went from 4 to 1 (Service map still generating 4 calls pending optimization).

• Testing on a OpenSearch cluster with 200gb of spans indicated that the overview query call was causing a 2x delay to retrieve the metrics already available in payload. Average time reduced by 50% for trace view page for non-cached data.

2. Remove handleSpansRequest() from Span List and Tree view to prevent unnecessary queries when switching between Timeline and table views.

• Span_detail_table will maintain a handleSpansRequest() to cover application analytics pending its depreciation as it does not have a payload request to use the new functionality.

3. Fixed a bug where Jaeger overview was not being populated with information through edge case handiling in the helper function getOverviewFields()

4. Fixed Jaeger Tree Span table not working

5. Add sorting to Jaeger Span Table.

6. Fix gantt chart displaying errors. The time text we added to the right was overriding the error field. We now check if it has an error and display that as priority over the time.

Queries Before 8 request + 1 per table change

QueriesBefore.mov

Queries after 5 request + 0 per table change

QueriesAfter.mov

Jaeger Before:

Jaeger After:

Gantt chart error display fix:

Issues Resolved

#2334 - Trace View section

Check List

• New functionality includes testing.
  • All tests pass, including unit test, integration test and doctest
• New functionality has been documented.
  • New functionality has javadoc added
  • New functionality has user manual doc added
• Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[codecov]

Codecov Report

Attention: Patch coverage is 36.50190% with 167 lines in your changes missing coverage. Please review.

Project coverage is 56.13%. Comparing base (5cfbba1) to head (e102bd1).
Report is 6 commits behind head on main.

Files with missing lines	Patch %	Lines
..._analytics/components/traces/span_detail_table.tsx	19.75%	65 Missing ⚠️
..._analytics/components/traces/span_detail_panel.tsx	14.28%	36 Missing ⚠️
...e_analytics/components/common/helper_functions.tsx	5.26%	18 Missing ⚠️
...s/trace_analytics/components/traces/trace_view.tsx	15.00%	17 Missing ⚠️
...analytics/components/traces/trace_view_helpers.tsx	78.26%	13 Missing and 2 partials ⚠️
...mponents/flyout_components/trace_detail_render.tsx	18.18%	9 Missing ⚠️
...trace_analytics/requests/traces_request_handler.ts	50.00%	7 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2349      +/-   ##
==========================================
- Coverage   56.53%   56.13%   -0.40%     
==========================================
  Files         393      395       +2     
  Lines       15574    15807     +233     
  Branches     4284     4390     +106     
==========================================
+ Hits         8804     8873      +69     
- Misses       6705     6863     +158     
- Partials       65       71       +6     

Flag	Coverage Δ
dashboards-observability	56.13% <36.50%> (-0.40%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[ps48]

Why do we need this function to normalizePayload? Since we are using just 1 request for all trace related details shouldn't we normalize this at source where we receive the request?

[TackAdam]

Moved to traces_request_handler
and used in handlePayloadRequest

const normalizedData = normalizePayload(response);
setPayloadData(JSON.stringify(normalizedData, null, 2));

[ps48]

In general, since we are moving lot of computation from OpenSearch to browser we need to manually do some sanity checks, to see if the calculation before and after is the same. May be use our sanity data and check the values in 2.18 (playground) and now with this PR in your local.

[ps48]

same here

[joshuali925]

is there a reason that this is different from

dashboards-observability/public/components/trace_analytics/components/traces/span_detail_panel.tsx

Lines 144 to 149 in 0fe0710

	const parsed = JSON.parse(payloadData);
	let hits: any[] = [];
	if (parsed.hits && Array.isArray(parsed.hits.hits)) {
	hits = parsed.hits.hits;
	} else if (Array.isArray(parsed)) {
	hits = parsed;

? and maybe extract the parsing into a util function?

[ps48]

Usually, we'd want to keep useEffect lean and put the logic inside a function.

[TackAdam]

Moved logic to helper above it

const parseAndFilterHits = (payloadData: string, mode: string, spanFilters: any[]) => {
  try {
    const parsed = JSON.parse(payloadData);
    let hits: any[] = [];
    if (parsed.hits && Array.isArray(parsed.hits.hits)) {
      hits = parsed.hits.hits;
    } else if (Array.isArray(parsed)) {
      hits = parsed;
    } else {
      console.warn('Unexpected payload format:', parsed);
      return [];
    }

    hits = hits.map((hit) => {
      if (mode === 'jaeger') {
        if (!hit.sort || !hit.sort[0]) {
          return {
            ...hit,
            sort: [Number(hit._source.startTime) * 1000], // Jaeger: startTime is in microseconds
          };
        }
      } else {
        if (!hit.sort || !hit.sort[0]) {
          return {
            ...hit,
            sort: [parseIsoToNano(hit._source.startTime)],
          };
        }
      }
      return hit;
    });

    hits.sort((a, b) => b.sort[0] - a.sort[0]);

    if (spanFilters.length > 0) {
      hits = hits.filter((hit) => {
        return spanFilters.every(({ field, value }) => {
          let fieldVal;
          if (mode === 'jaeger' && field.startsWith('process.')) {
            fieldVal = hit._source?.process?.[field.split('.')[1]];
          } else {
            fieldVal = hit._source?.[field];
          }
          return fieldVal === value;
        });
      });
    }

    hits = hits.filter((hit) => {
      if (mode === 'jaeger') {
        return Boolean(hit._source?.process?.serviceName);
      } else {
        return Boolean(hit._source?.serviceName);
      }
    });
    
    return hits;

  } catch (error) {
    console.error('Error processing payloadData in parseAndFilterHits:', error);
    return [];
  }
};

useEffect(() => {
  if (!props.payloadData) {
    console.warn('No payloadData provided to SpanDetailPanel');
    return;
  }

  const hits = parseAndFilterHits(props.payloadData, mode, spanFilters);

  if (hits.length === 0) {
    return;
  }

  hitsToSpanDetailData(hits, props.colorMap, mode)
    .then((transformedData) => {
      setData(transformedData);
    })
    .catch((error) => {
      console.error('Error in hitsToSpanDetailData:', error);
    })
    .finally(() => {
      if (props.setGanttChartLoading) {
        props.setGanttChartLoading(false);
      }
    });
}, [props.payloadData, props.colorMap, mode, spanFilters]);

[ps48]

Why are we doing this check? hits won't have sort field by default?

[TackAdam]

In general, since we are moving lot of computation from OpenSearch to browser we need to manually do some sanity checks, to see if the calculation before and after is the same. May be use our sanity data and check the values in 2.18 (playground) and now with this PR in your local.

Comparing results

NewCode	Playground 2.18

[ps48]

Can we add a small check here, that first span doesn't have a parentTraceId? To account for cases we the OTel data might miss parent traces.

[TackAdam]

Updated too

    let computedLatency: number | null = null;
    const tgDuration = get(firstSpan, 'traceGroupFields.durationInNanos');
    if (firstSpan.traceGroupFields && firstSpan.traceGroupFields.durationInNanos != null) {
      computedLatency = Number(firstSpan.traceGroupFields.durationInNanos) / NANOS_TO_MS;
    } else if (tgDuration != null) {
      computedLatency = Number(tgDuration) / NANOS_TO_MS;
    } else {
      const startTimes = parsed.map((span: any) => moment(span._source.startTime));
      const endTimes = parsed.map((span: any) => moment(span._source.endTime));
      
      const minStartTime = moment.min(startTimes);
      const maxEndTime = moment.max(endTimes);
      
      if (minStartTime && maxEndTime) {
        computedLatency = maxEndTime.diff(minStartTime, 'milliseconds', true);
        fallbackValueUsed = true;
      }
    }

So if the first span is not present we get a more accurate latency instead of 0 or a smaller number

Before - invalid date:

After - invalid date:

Latency is now much more accurate when handling missing data

[joshuali925]

nit. since this if is the same for both modes, it might be better to check traceMode inside this if

[TackAdam]

Updated too

      hits = hits.map((hit) => {
        if (!hit.sort || !hit.sort[0]) {
          const time = traceMode === 'jaeger' 
            ? Number(hit._source.startTime) * MILI_TO_SEC
            : parseIsoToNano(hit._source.startTime);
          
          return {
            ...hit,
            sort: [time],
          };
        }
        return hit;
      });

[joshuali925]

do we use hit.sort anywhere else? if not then probably just do hits.sort((a, b) => b._source.startTime - a._source.startTime); here and remove the inserting sort logic above

[TackAdam]

I believe the inserting sort logic is needed for handling jaeger vrs data prepper. Using just the hits.sort you show fails to render the gantt visualization.

[joshuali925]

lodash can handle the nested and flattened fields when reading value

Suggested change

	let fieldVal;
	if (traceMode === 'jaeger' && field.startsWith('process.')) {
	fieldVal = hit._source?.process?.[field.split('.')[1]];
	} else {
	fieldVal = hit._source?.[field];
	}
	return fieldVal === value;
	return _.get(hit._source, field) === value;

[TackAdam]

Updated too

      if (payloadSpanFilters.length > 0) {
        hits = hits.filter((hit) => {
          return payloadSpanFilters.every(({ field, value }) => {
            return _.get(hit._source, field) === value;
          });
        });
      }

[joshuali925]

is there a reason that this is different from

dashboards-observability/public/components/trace_analytics/components/traces/span_detail_panel.tsx

Lines 144 to 149 in 0fe0710

	const parsed = JSON.parse(payloadData);
	let hits: any[] = [];
	if (parsed.hits && Array.isArray(parsed.hits.hits)) {
	hits = parsed.hits.hits;
	} else if (Array.isArray(parsed)) {
	hits = parsed;

? and maybe extract the parsing into a util function?

[joshuali925]

will there be negate filters like field a is not b?

[TackAdam]

Not currently supported the flyout only allows filters to be added as such:

Where the user can only clear. Intended use is for investigating based on the flyout

[joshuali925]

this might need some unit tests

[TackAdam]

Added cypress test for jaeger tree view functionality

 it('Checks tree view for specific traceId in Jaeger mode', () => {
   cy.contains('15b0b4004a651c4c').click();
   cy.get('[data-test-subj="globalLoadingIndicator"]').should('not.exist');

   cy.get('.euiButtonGroup').contains('Tree view').click();
   cy.get("[data-test-subj='treeExpandAll']").should('exist');
   cy.get("[data-test-subj='treeCollapseAll']").should('exist');

   // Waiting time for render to complete
   cy.get("[data-test-subj='treeExpandAll']").click();
   cy.get("[data-test-subj='treeCollapseAll']").click();
   
   cy.get("[data-test-subj='treeViewExpandArrow']").should('have.length', 1);
   cy.get("[data-test-subj='treeExpandAll']").click();
   cy.get("[data-test-subj='treeViewExpandArrow']").should('have.length', 6);
 });

[mengweieric]

Question: Do we have a reset mechanism for these setters? If we previously loaded data with an overview, but a new payload arrives without an overview for some reason, it seems like the overview wouldn't be updated here, potentially leaving it with stale data. Should we handle this case explicitly?

[TackAdam]

That one specifically is for application analytics and the useEffect triggers on payload change. As long as it is a valid payload the overview will get the new information from it. If the payload is successful from handlePayloadRequest causing new data the overview will always be generated from that information.

[mengweieric]

nit. Should we consider moving this common folder one level up? It seems like each logical component should have its own common folder.

[TackAdam]

I think having it on the same level as our helper_functions makes sense as this would be for constants shared between trace_analytics including both traces and services as we continue to refactor.

[mengweieric]

type?

[TackAdam]

Updated too

interface Span {
  traceId: string;
  spanId: string;
  traceState: string;
  parentSpanId: string;
  name: string;
  kind: string;
  startTime: string;
  endTime: string;
  durationInNanos: number;
  serviceName: string;
  events: any[];
  links: any[];
  droppedAttributesCount: number;
  droppedEventsCount: number;
  droppedLinksCount: number;
  traceGroup: string;
  traceGroupFields: {
    endTime: string;
    durationInNanos: number;
    statusCode: number;
  };
  status: {
    code: number;
  };
  instrumentationLibrary: {
    name: string;
    version: string;
  };
}

interface ParsedHit {
  _index: string;
  _id: string;
  _score: number;
  _source: Span;
  sort?: any[];
}

export const parseHits = (payloadData: string): ParsedHit[] => {
  try {
    const parsed = JSON.parse(payloadData);
    let hits: ParsedHit[] = [];

    if (parsed.hits && Array.isArray(parsed.hits.hits)) {
      hits = parsed.hits.hits;
    } else if (Array.isArray(parsed)) {
      hits = parsed;
    }

    return hits;
  } catch (error) {
    console.error('Error processing payloadData:', error);
    return [];
  }
};

[mengweieric]

I thought we are avoiding using lodash?

[TackAdam]

Removed, replaced with

      if (payloadSpanFilters.length > 0) {
        hits = hits.filter((hit) => {
          return payloadSpanFilters.every(({ field, value }) => {
            const fieldValue = field.split('.').reduce((acc, part) => acc?.[part], hit._source);
            return fieldValue === value;
          });
        });
      }

[mengweieric]

we should be able to refactor these a little bit for example like extracting these duplicated root span handing to a function:

rootSpans.push(spanMap[span[spanIdKey]]);
alreadyAddedRootSpans.add(span[spanIdKey]);

and also for these nested if conditions, I don't see you are having any else if or else then why not to merge them like

if (ref.refType === 'FOLLOWS_FROM') {
            if (!alreadyAddedRootSpans.has(span[spanIdKey])) {
...

to

if (ref.refType === 'FOLLOWS_FROM' && !alreadyAddedRootSpans.has(span[spanIdKey])) {
...

[TackAdam]

Updated too

interface SpanReference {
    refType: 'CHILD_OF' | 'FOLLOWS_FROM';
    spanID: string;
  }
  
  const addRootSpan = (
    spanId: string,
    spanMap: SpanMap,
    rootSpans: Span[],
    alreadyAddedRootSpans: Set<string>
  ) => {
    if (!alreadyAddedRootSpans.has(spanId)) {
      rootSpans.push(spanMap[spanId]);
      alreadyAddedRootSpans.add(spanId);
    }
  };
  
  const buildHierarchy = (spans: Span[]): Span[] => {
    const spanMap: SpanMap = {};
  
    spans.forEach((span) => {
      const spanIdKey = props.mode === 'jaeger' ? 'spanID' : 'spanId';
      spanMap[span[spanIdKey]] = { ...span, children: [] };
    });
  
    const rootSpans: Span[] = [];
    const alreadyAddedRootSpans: Set<string> = new Set(); // Track added root spans
  
    spans.forEach((span) => {
      const spanIdKey = props.mode === 'jaeger' ? 'spanID' : 'spanId';
      const references: SpanReference[] = span.references || [];
  
      if (props.mode === 'jaeger') {
        references.forEach((ref: SpanReference) => {
          if (ref.refType === 'CHILD_OF') {
            const parentSpan = spanMap[ref.spanID];
            if (parentSpan) {
              parentSpan.children.push(spanMap[span[spanIdKey]]);
            }
          }
  
          if (ref.refType === 'FOLLOWS_FROM' && !alreadyAddedRootSpans.has(span[spanIdKey])) {
            addRootSpan(span[spanIdKey], spanMap, rootSpans, alreadyAddedRootSpans);
          }
        });
  
        if (references.length === 0 || references.every((ref) => ref.refType === 'FOLLOWS_FROM')) {
          addRootSpan(span[spanIdKey], spanMap, rootSpans, alreadyAddedRootSpans);
        }
      } else {
        // Data Prepper handling
        if (span.parentSpanId && spanMap[span.parentSpanId]) {
          spanMap[span.parentSpanId].children.push(spanMap[span[spanIdKey]]);
        } else {
          addRootSpan(span[spanIdKey], spanMap, rootSpans, alreadyAddedRootSpans);
        }
      }
    });
  
    return rootSpans;
  };

[mengweieric]

i18n?

[TackAdam]

Updated too

                        <EuiIconTip
                          aria-label={i18n.translate('tracesOverview.iconTip.ariaLabel', {
                            defaultMessage: 'Warning',
                          })}
                          size="m"
                          type="alert"
                          color="warning"
                          content={i18n.translate('tracesOverview.iconTip.content', {
                            defaultMessage: 'Latency may not be accurate due to missing or unexpected duration data.',
                          })}
                        />

[mengweieric]

type?

[TackAdam]

Updated too

interface Hit {
  _index: string;
  _id: string;
  _score: number;
  _source: any;
  sort?: any[];
}

interface ParsedResponse {
  hits?: {
    hits: Hit[];
  };
  [key: string]: any;
}

export function normalizePayload(parsed: ParsedResponse): Hit[] {
  if (Array.isArray(parsed)) {
    return parsed;
  }
  if (parsed.hits && Array.isArray(parsed.hits.hits)) {
    return parsed.hits.hits;
  }
  return [];
}

[mengweieric]

type?

[mengweieric]

avoid using lodash?

[TackAdam]

Removed and refactored to work without it.

[joshuali925]

do we have a replacement for lodash? some of them like get are useful and not trivial in plain js
https://github.com/you-dont-need/You-Dont-Need-Lodash-Underscore?tab=readme-ov-file#_get

[ps48]

Why do we still need this? If we know one check is for Jaeger and other is data prepper can we just use mode based if conditions instead?

[TackAdam]

Updated too

export function normalizePayload(parsed: ParsedResponse): Hit[] {
  if (parsed.hits && Array.isArray(parsed.hits.hits)) {
    return parsed.hits.hits;
  }
  return [];
}

Jaeger and data prepper should be under parsed.hits

[ps48]

Can we please move these to common constants?

[TackAdam]

Moved to constants