[Bug]: Prevent Critical Path code from throwing exceptions

[yurishkuro]

What happened?

Critical Path code occasionally throws Range exception here:

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

Line 87 in 17eb291

throw RangeError(`Error while computing Critical Path Algorithm.`);

Steps to reproduce

Unknown, the failures have been happening sporadically in CI

Expected behavior

Code should handle all input trace data, fall back gracefully to not returning CP if impossible to calculate.

Relevant log output

No response

Screenshot

No response

Additional context

We need to rethink the switch statement. The relative positioning of parent/child spans has a fixed number of permutations, they should be covered by the appropriate if-conditions, ideally in a hierarchical manner, e.g.

if child starts after parent
  if child ends before parent
    etc.

The function should also be simplified to do early returns. For example, instead of introducing a nesting level with

if (span && span.references.length) {
   // nested logic
}

replace it with

if (!(span && span.references.length)) {
   return
}

Jaeger backend version

No response

SDK

No response

Pipeline

No response

Stogage backend

No response

Operating system

No response

Deployment model

No response

Deployment configs

No response

[yurishkuro]

cc @GLVSKiriti

[GLVSKiriti]

                |----parent----|
         |------------child------------|

I think the error is due to this undhandled case.
So default case throws the range error

[GLVSKiriti]

I will make a PR !!

[yurishkuro]

Thanks for the extra fix. But I still want to refactor that code to never throw exceptions

[GLVSKiriti]

Thanks for the extra fix. But I still want to refactor that code to never throw exceptions

Can't we remove the default case bcz as already we handled all permutations of the parent and child?

[yurishkuro]

I think the way the code is written now it's very difficult to judge if it covers all of your decision space. If it was structured as nested if statements with one condition in each, there would be easier way to reason.

Separately, even in the current code, instead of raising error from default clause it could simply log a warning with the values of the relevant timestamps. But it's still lazy approach, a proper coverage of decision space is better.

Btw we could also use fuzzing test to verify that default clause can never be reached.