Refactor Critical Path code to prevent it from ever throwing exceptions

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

@@ -21,8 +21,9 @@ import test6 from './testCases/test6';
 import test7 from './testCases/test7';
 import test5 from './testCases/test5';
 import test8 from './testCases/test8';
+import test9 from './testCases/test9';
 
-describe.each([[test1], [test2], [test3], [test4], [test5], [test6], [test7], [test8]])(
+describe.each([[test1], [test2], [test3], [test4], [test5], [test6], [test7], [test8], [test9]])(
   'Happy Path',
   testProps => {
     it('should find criticalPathSections correctly', () => {

packages/jaeger-ui/src/components/TracePage/CriticalPath/testCases/test9.js

@@ -0,0 +1,76 @@
+// Copyright (c) 2023 The Jaeger Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+import transformTraceData from '../../../../model/transform-trace-data';
+
+/*
+                    ┌──────────┐        |
+                    │ Span A   │        |                 span A
+                    └──────────┘        |                  /
+                    ++++++++++++        |                 /
+    ┌────────────┐                      |              span B
+    │  Span B    │                      |               
+    └────────────┘                      |      (parent-child tree)
+spanB will be dropped.                  |
+span A is on critical path(+++++)       |
+*/
+
+const trace = {
+  traceID: 'trace-abc',
+  spans: [
+    {
+      spanID: 'span-A',
+      operationName: 'op-A',
+      references: [],
+      startTime: 10,
+      duration: 20,
+      processID: 'p1',
+    },
+    {
+      spanID: 'span-B',
+      operationName: 'op-B',
+      references: [
+        {
+          refType: 'CHILD_OF',
+          spanID: 'span-A',
+        },
+      ],
+      startTime: 1,
+      duration: 4,
+      processID: 'p1',
+    },
+  ],
+  processes: {
+    p1: {
+      serviceName: 'service-one',
+    },
+  },
+};
+
+const transformedTrace = transformTraceData(trace);
+
+const criticalPathSections = [
+  {
+    spanId: 'span-A',
+    section_start: 10,
+    section_end: 30,
+  },
+];
+
+const test9 = {
+  criticalPathSections,
+  trace: transformedTrace,
+};
+
+export default test9;

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

@@ -17,6 +17,7 @@ import test4 from '../testCases/test4';
 import test6 from '../testCases/test6';
 import test7 from '../testCases/test7';
 import test8 from '../testCases/test8';
+import test9 from '../testCases/test9';
 import getChildOfSpans from './getChildOfSpans';
 import sanitizeOverFlowingChildren from './sanitizeOverFlowingChildren';
 
@@ -35,11 +36,12 @@ function getExpectedSanitizedData(spans, test) {
 }
 
 describe.each([
-  [test3, new Map().set(test3.trace.spans[0].spanID, test3.trace.spans[0])],
-  [test4, new Map().set(test4.trace.spans[0].spanID, test4.trace.spans[0])],
+  [test3, new Map().set(test3.trace.spans[0].spanID, { ...test3.trace.spans[0], childSpanIds: [] })],
+  [test4, new Map().set(test4.trace.spans[0].spanID, { ...test4.trace.spans[0], childSpanIds: [] })],
   [test6, getExpectedSanitizedData(test6.trace.spans, 'test6')],
   [test7, getExpectedSanitizedData(test7.trace.spans, 'test7')],
   [test8, getExpectedSanitizedData(test8.trace.spans, 'test8')],
+  [test9, new Map().set(test9.trace.spans[0].spanID, { ...test9.trace.spans[0], childSpanIds: [] })],
 ])('sanitizeOverFlowingChildren', (testProps, expectedSanitizedData) => {
   it('Should sanitize the data(overflowing spans) correctly', () => {
     const refinedSpanData = getChildOfSpans(testProps.trace.spans);

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

@@ -27,80 +27,74 @@ const sanitizeOverFlowingChildren = (spanMap: Map<string, Span>): Map<string, Sp
 
   spanIds.forEach(spanId => {
     const span = spanMap.get(spanId)!;
-    if (span && span.references.length) {
-      // parentSpan will be undefined when its parentSpan is dropped previously
-      const parentSpan = spanMap.get(span.references[0].spanID);
-      if (parentSpan) {
-        const childEndTime = span.startTime + span.duration;
-        const parentEndTime = parentSpan.startTime + parentSpan.duration;
-        switch (true) {
-          case span.startTime >= parentSpan.startTime && childEndTime <= parentEndTime:
-            // case 1: everything looks good
-            // |----parent----|
-            //   |----child--|
-            break;
-
-          case span.startTime < parentSpan.startTime &&
-            childEndTime <= parentEndTime &&
-            childEndTime > parentSpan.startTime:
-            // case 2: child start before parent, truncate is needed
-            //      |----parent----|
-            //   |----child--|
-            spanMap.set(span.spanID, {
-              ...span,
-              startTime: parentSpan.startTime,
-              duration: childEndTime - parentSpan.startTime,
-            });
-            break;
-
-          case span.startTime >= parentSpan.startTime &&
-            childEndTime > parentEndTime &&
-            span.startTime < parentEndTime:
-            // case 3: child end after parent, truncate is needed
-            //      |----parent----|
-            //              |----child--|
-            spanMap.set(span.spanID, {
-              ...span,
-              duration: parentEndTime - span.startTime,
-            });
-            break;
-
-          case span.startTime < parentSpan.startTime && childEndTime > parentEndTime:
-            // case 4: child start before parent and end after parent, truncate is needed
-            //      |----parent----|
-            //  |---------child---------|
-            spanMap.set(span.spanID, {
-              ...span,
-              startTime: parentSpan.startTime,
-              duration: parentEndTime - parentSpan.startTime,
-            });
-            break;
+    if (!(span && span.references.length)) {
+      return;
+    }
+    // parentSpan will be undefined when its parentSpan is dropped previously
+    const parentSpan = spanMap.get(span.references[0].spanID);
 
-          case span.startTime >= parentEndTime || childEndTime <= parentSpan.startTime:
-            // case 5: child outside of parent range => drop the child span
-            //      |----parent----|
-            //                        |----child--|
-            // or
-            //                      |----parent----|
-            //       |----child--|
+    if (!parentSpan) {
+      // Drop the child spans of dropped parent span
+      spanMap.delete(span.spanID);
+      return;
+    }
+    const childEndTime = span.startTime + span.duration;
+    const parentEndTime = parentSpan.startTime + parentSpan.duration;
+    if (span.startTime >= parentSpan.startTime) {
+      if (span.startTime >= parentEndTime) {
+        // child outside of parent range => drop the child span
+        //      |----parent----|
+        //                        |----child--|
+        // Remove the childSpan from spanMap
+        spanMap.delete(span.spanID);
 
-            // Remove the childSpan from spanMap
-            spanMap.delete(span.spanID);
+        // Remove the childSpanId from its parent span
+        parentSpan.childSpanIds = parentSpan.childSpanIds.filter(id => id !== span.spanID);
+        return;
+      }
+      if (childEndTime > parentEndTime) {
+        // child end after parent, truncate is needed
+        //      |----parent----|
+        //              |----child--|
+        spanMap.set(span.spanID, {
+          ...span,
+          duration: parentEndTime - span.startTime,
+        });
+        return;
+      }
+      // everything looks good
+      // |----parent----|
+      //   |----child--|
+      return;
+    }
+    if (childEndTime <= parentSpan.startTime) {
+      // child outside of parent range => drop the child span
+      //                      |----parent----|
+      //       |----child--|
 
-            // Remove the childSpanId from its parent span
-            parentSpan.childSpanIds = parentSpan.childSpanIds.filter(id => id === span.spanID);
-            spanMap.set(parentSpan.spanID, { ...parentSpan });
-            break;
+      // Remove the childSpan from spanMap
+      spanMap.delete(span.spanID);
 
-          default:
-            // Never reaches to default
-            // Something unexpected happened
-            throw RangeError(`Error while computing Critical Path Algorithm.`);
-        }
-      } else {
-        // Drop the child spans of dropped parent span
-        spanMap.delete(span.spanID);
-      }
+      // Remove the childSpanId from its parent span
+      parentSpan.childSpanIds = parentSpan.childSpanIds.filter(id => id !== span.spanID);
+    } else if (childEndTime <= parentEndTime) {
+      // child start before parent, truncate is needed
+      //      |----parent----|
+      //   |----child--|
+      spanMap.set(span.spanID, {
+        ...span,
+        startTime: parentSpan.startTime,
+        duration: childEndTime - parentSpan.startTime,
+      });
+    } else {
+      // child start before parent and end after parent, truncate is needed
+      //      |----parent----|
+      //  |---------child---------|
+      spanMap.set(span.spanID, {
+        ...span,
+        startTime: parentSpan.startTime,
+        duration: parentEndTime - parentSpan.startTime,
+      });
     }
   });