Skip to content

Commit

Permalink
Refactor Critical Path code to prevent it from ever throwing exceptio…
Browse files Browse the repository at this point in the history 
…ns (#1785)

Resolves #1779 

In this PR switch statement is refactored to if-else block and covered
all of our decision space.

---------

Signed-off-by: GLVS Kiriti <[email protected]>
  • Loading branch information
@GLVSKiriti
GLVSKiriti authored Sep 14, 2023
1 parent 103fef5 commit 7ef6909
Show file tree
Hide file tree
Showing 4 changed files with 146 additions and 73 deletions.
3 changes: 2 additions & 1 deletion packages/jaeger-ui/src/components/TracePage/CriticalPath/index.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down
76 changes: 76 additions & 0 deletions packages/jaeger-ui/src/components/TracePage/CriticalPath/testCases/test9.js
View file
Original file line number Diff line number Diff line change
@@ -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;
6 changes: 4 additions & 2 deletions ...jaeger-ui/src/components/TracePage/CriticalPath/utils/sanitizeOverFlowingChildren.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand All @@ -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);
Expand Down
134 changes: 64 additions & 70 deletions ...ges/jaeger-ui/src/components/TracePage/CriticalPath/utils/sanitizeOverFlowingChildren.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
});
}
});

Expand Down

0 comments on commit 7ef6909

Please sign in to comment.