Prevent trace mutation during critical path computation

packages/jaeger-ui/src/components/TracePage/CriticalPath/index.test.js

@@ -21,3 +21,64 @@ describe.each([[test1], [test2], [test3], [test4], [test5], [test6], [test7], [t
     });
   }
 );
+
+describe('criticalPathForTrace immutability', () => {
+  it('should not modify the original trace spans', () => {
+    // Create a deep copy of the trace for comparison
+    const originalTrace = JSON.parse(JSON.stringify(test2.trace));
+
+    // Run the critical path algorithm
+    TraceCriticalPath(test2.trace);
+
+    // Verify the trace was not modified
+    expect(JSON.stringify(test2.trace)).toBe(JSON.stringify(originalTrace));
+  });
+
+  it('should not modify span childSpanIds arrays', () => {
+    // Store original childSpanIds arrays (and their references)
+    const originalChildSpanIds = test2.trace.spans.map(span => ({
+      array: span.childSpanIds,
+      values: [...(span.childSpanIds || [])],
+    }));
+
+    // Run the critical path algorithm
+    TraceCriticalPath(test2.trace);
+
+    // Verify childSpanIds were not modified (same reference and values)
+    test2.trace.spans.forEach((span, index) => {
+      expect(span.childSpanIds).toBe(originalChildSpanIds[index].array); // Same reference
+      expect(span.childSpanIds).toEqual(originalChildSpanIds[index].values); // Same values
+    });
+  });
+
+  it('should not modify span references arrays', () => {
+    // Store original references (and their references)
+    const originalReferences = test2.trace.spans.map(span => ({
+      array: span.references,
+      values: span.references.map(ref => ({ ...ref })),
+    }));
+
+    // Run the critical path algorithm
+    TraceCriticalPath(test2.trace);
+
+    // Verify references were not modified (same reference and values)
+    test2.trace.spans.forEach((span, index) => {
+      expect(span.references).toBe(originalReferences[index].array); // Same reference
+      expect(span.references.length).toBe(originalReferences[index].values.length);
+    });
+  });
+
+  it('should not modify FOLLOWS_FROM spans parent childSpanIds', () => {
+    // Store original childSpanIds of the parent in test5
+    const parentSpan = test5.trace.spans[0];
+    const originalChildSpanIds = [...parentSpan.childSpanIds];
+    const originalLength = originalChildSpanIds.length;
+
+    // Run the critical path algorithm
+    TraceCriticalPath(test5.trace);
+
+    // Verify parent's childSpanIds was not modified
+    expect(parentSpan.childSpanIds.length).toBe(originalLength);
+    expect(parentSpan.childSpanIds).toEqual(originalChildSpanIds);
+  });
+});

packages/jaeger-ui/src/components/TracePage/CriticalPath/index.tsx

@@ -2,7 +2,7 @@
 // SPDX-License-Identifier: Apache-2.0
 
 import memoizeOne from 'memoize-one';
-import { Span, Trace, criticalPathSection } from '../../../types/trace';
+import { Span, Trace, criticalPathSection, CPSpan, CPSpanReference } from '../../../types/trace';
 import getChildOfSpans from './utils/getChildOfSpans';
 import findLastFinishingChildSpan from './utils/findLastFinishingChildSpan';
 import sanitizeOverFlowingChildren from './utils/sanitizeOverFlowingChildren';
@@ -28,12 +28,12 @@ import sanitizeOverFlowingChildren from './utils/sanitizeOverFlowingChildren';
  * immediately before the LFC's start.
  */
 const computeCriticalPath = (
-  spanMap: Map<string, Span>,
+  spanMap: Map<string, CPSpan>,
   spanId: string,
   criticalPath: criticalPathSection[],
   returningChildStartTime?: number
 ): criticalPathSection[] => {
-  const currentSpan: Span = spanMap.get(spanId)!;
+  const currentSpan: CPSpan = spanMap.get(spanId)!;
 
   const lastFinishingChildSpan = findLastFinishingChildSpan(spanMap, currentSpan, returningChildStartTime);
   let spanCriticalSection: criticalPathSection;
@@ -78,10 +78,29 @@ function criticalPathForTrace(trace: Trace) {
   const rootSpanId = trace.spans[0].spanID;
   // If there is root span then algorithm implements
   if (rootSpanId) {
+    // Create a map of CPSpan objects to avoid modifying the original trace
     const spanMap = trace.spans.reduce((map, span) => {
-      map.set(span.spanID, span);
+      const cpSpan: CPSpan = {
+        spanID: span.spanID,
+        startTime: span.startTime,
+        duration: span.duration,
+        // Create a shallow copy of references array to avoid modifying original
+        // and convert to CPSpanReference
+        references: span.references.map(
+          (ref): CPSpanReference => ({
+            refType: ref.refType,
+            spanID: ref.spanID,
+            traceID: ref.traceID,
+            span: undefined, // Will be set later if needed
+          })
+        ),
+        // Create a shallow copy of childSpanIds array to avoid modifying original
+        childSpanIds: [...span.childSpanIds],
+        hasChildren: span.hasChildren,
+      };
+      map.set(span.spanID, cpSpan);
       return map;
-    }, new Map<string, Span>());
+    }, new Map<string, CPSpan>());
     try {
       const refinedSpanMap = getChildOfSpans(spanMap);
       const sanitizedSpanMap = sanitizeOverFlowingChildren(refinedSpanMap);

packages/jaeger-ui/src/components/TracePage/CriticalPath/utils/findLastFinishingChildSpan.test.js

@@ -7,14 +7,36 @@ import getChildOfSpans from './getChildOfSpans';
 import findLastFinishingChildSpanId from './findLastFinishingChildSpan';
 import sanitizeOverFlowingChildren from './sanitizeOverFlowingChildren';
 
+// Helper function to create CPSpan objects from Span objects
+function createCPSpan(span) {
+  return {
+    spanID: span.spanID,
+    startTime: span.startTime,
+    duration: span.duration,
+    references: span.references.map(ref => ({
+      refType: ref.refType,
+      spanID: ref.spanID,
+      traceID: ref.traceID,
+      span: undefined,
+    })),
+    childSpanIds: [...span.childSpanIds],
+    hasChildren: span.hasChildren,
+  };
+}
+
+// Helper function to create CPSpan map from trace spans
+function createCPSpanMap(spans) {
+  return spans.reduce((map, span) => {
+    map.set(span.spanID, createCPSpan(span));
+    return map;
+  }, new Map());
+}
+
 describe('findLastFinishingChildSpanId', () => {
   it('Should find lfc of a span correctly', () => {
-    const refinedSpanData = getChildOfSpans(test1.trace.spans);
-    const spanMap = refinedSpanData.reduce((map, span) => {
-      map.set(span.spanID, span);
-      return map;
-    }, new Map());
-    const sanitizedSpanMap = sanitizeOverFlowingChildren(spanMap);
+    const spanMap = createCPSpanMap(test1.trace.spans);
+    const refinedSpanMap = getChildOfSpans(spanMap);
+    const sanitizedSpanMap = sanitizeOverFlowingChildren(refinedSpanMap);
 
     const currentSpan = sanitizedSpanMap.get('span-C');
     let lastFinishingChildSpan = findLastFinishingChildSpanId(sanitizedSpanMap, currentSpan);
@@ -26,12 +48,9 @@ describe('findLastFinishingChildSpanId', () => {
   });
 
   it('Should find lfc of a span correctly with test2', () => {
-    const refinedSpanData = getChildOfSpans(test2.trace.spans);
-    const spanMap = refinedSpanData.reduce((map, span) => {
-      map.set(span.spanID, span);
-      return map;
-    }, new Map());
-    const sanitizedSpanMap = sanitizeOverFlowingChildren(spanMap);
+    const spanMap = createCPSpanMap(test2.trace.spans);
+    const refinedSpanMap = getChildOfSpans(spanMap);
+    const sanitizedSpanMap = sanitizeOverFlowingChildren(refinedSpanMap);
 
     const currentSpan = sanitizedSpanMap.get('span-X');
     let lastFinishingChildSpanId = findLastFinishingChildSpanId(sanitizedSpanMap, currentSpan);

packages/jaeger-ui/src/components/TracePage/CriticalPath/utils/findLastFinishingChildSpan.tsx

@@ -1,18 +1,18 @@
 // Copyright (c) 2023 The Jaeger Authors
 // SPDX-License-Identifier: Apache-2.0
 
-import { Span } from '../../../../types/trace';
+import { CPSpan } from '../../../../types/trace';
 
 /**
  * @returns - Returns the span that finished last among the remaining child spans.
  * If a `returningChildStartTime` is provided as a parameter, it returns the child span that finishes
  * just before the specified `returningChildStartTime`.
  */
 const findLastFinishingChildSpan = (
-  spanMap: Map<string, Span>,
-  currentSpan: Span,
+  spanMap: Map<string, CPSpan>,
+  currentSpan: CPSpan,
   returningChildStartTime?: number
-): Span | undefined => {
+): CPSpan | undefined => {
   let lastFinishingChildSpanId: string | undefined;
   if (returningChildStartTime) {
     lastFinishingChildSpanId = currentSpan.childSpanIds.find(

packages/jaeger-ui/src/components/TracePage/CriticalPath/utils/getChildOfSpans.test.js

@@ -5,30 +5,65 @@ import test2 from '../testCases/test2';
 import test5 from '../testCases/test5';
 import getChildOfSpans from './getChildOfSpans';
 
+// Helper function to create CPSpan objects from Span objects
+function createCPSpan(span) {
+  return {
+    spanID: span.spanID,
+    startTime: span.startTime,
+    duration: span.duration,
+    references: span.references.map(ref => ({
+      refType: ref.refType,
+      spanID: ref.spanID,
+      traceID: ref.traceID,
+      span: undefined,
+    })),
+    childSpanIds: [...span.childSpanIds],
+    hasChildren: span.hasChildren,
+  };
+}
+
 describe('getChildOfSpans', () => {
   it('Should not remove CHILD_OF child spans if there are any', () => {
+    // Create CPSpan objects from the original spans
     const spanMap = test2.trace.spans.reduce((map, span) => {
-      map.set(span.spanID, span);
+      map.set(span.spanID, createCPSpan(span));
       return map;
     }, new Map());
     const refinedSpanMap = getChildOfSpans(spanMap);
-    const expectedRefinedSpanMap = spanMap;
 
     expect(refinedSpanMap.size).toBe(3);
-    expect(refinedSpanMap).toStrictEqual(expectedRefinedSpanMap);
   });
   it('Should remove FOLLOWS_FROM child spans if there are any', () => {
+    // Create CPSpan objects from the original spans
     const spanMap = test5.trace.spans.reduce((map, span) => {
-      map.set(span.spanID, span);
+      map.set(span.spanID, createCPSpan(span));
       return map;
     }, new Map());
     const refinedSpanMap = getChildOfSpans(spanMap);
-    const expectedRefinedSpanMap = new Map().set(test5.trace.spans[0].spanID, {
-      ...test5.trace.spans[0],
-      childSpanIds: [],
-    });
 
     expect(refinedSpanMap.size).toBe(1);
-    expect(refinedSpanMap).toStrictEqual(expectedRefinedSpanMap);
+    // Check that the parent span has no children
+    const parentSpan = refinedSpanMap.get(test5.trace.spans[0].spanID);
+    expect(parentSpan?.childSpanIds).toEqual([]);
+  });
+
+  it('Should not modify the original trace spans', () => {
+    // Store the original childSpanIds reference and value
+    const originalParentSpan = test5.trace.spans[0];
+    const originalChildSpanIdsRef = originalParentSpan.childSpanIds;
+    const originalChildSpanIdsValue = [...originalParentSpan.childSpanIds];
+
+    // Create CPSpan objects (copies) from the original spans
+    const spanMap = test5.trace.spans.reduce((map, span) => {
+      map.set(span.spanID, createCPSpan(span));
+      return map;
+    }, new Map());
+
+    // Run the function on CPSpan copies
+    getChildOfSpans(spanMap);
+
+    // Verify the original span was not modified
+    expect(originalParentSpan.childSpanIds).toBe(originalChildSpanIdsRef); // Same reference
+    expect(originalParentSpan.childSpanIds).toEqual(originalChildSpanIdsValue); // Same values
   });
 });

packages/jaeger-ui/src/components/TracePage/CriticalPath/utils/getChildOfSpans.tsx

@@ -1,13 +1,13 @@
 // Copyright (c) 2023 The Jaeger Authors
 // SPDX-License-Identifier: Apache-2.0
 
-import { Span } from '../../../../types/trace';
+import { CPSpan } from '../../../../types/trace';
 /**
  * Removes child spans whose refType is FOLLOWS_FROM and their descendants.
  * @param spanMap - The map containing spans.
  * @returns - A map with spans whose refType is CHILD_OF.
  */
-const getChildOfSpans = (spanMap: Map<string, Span>): Map<string, Span> => {
+const getChildOfSpans = (spanMap: Map<string, CPSpan>): Map<string, CPSpan> => {
   const followFromSpanIds: string[] = [];
   const followFromSpansDescendantIds: string[] = [];