-
Notifications
You must be signed in to change notification settings - Fork 471
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Bug]: Prevent Critical Path code from throwing exceptions #1779
Comments
cc @GLVSKiriti |
I think the error is due to this undhandled case. |
I will make a PR !! |
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? |
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. |
…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]>
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
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.
The function should also be simplified to do early returns. For example, instead of introducing a nesting level with
replace it with
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
The text was updated successfully, but these errors were encountered: