[Refactor]: Enhancement to selfTime calculation logic in TraceStatistics view #1901

GLVSKiriti · 2023-10-21T10:16:59Z

Which problem is this PR solving?

Resolves [Bug]: Negative Self time in Trace Statistics view #1592 & Resolves [Enhancement]: Self time Calculation to be unified across multiple screens #1263
Indirectly Resolves Unstable unit test on TraceStatistics/tableValues.tsx #877

Description of the changes

When calculating selfTime of a span
selfTime is the total time spent in the span when it was not waiting on children.
- So we skip the FOLLOWS_FROM type child spans
selfTime = duration - (total time consumed by child spans between span startTime and endTime)
This is implemented by using DRange similar to the logic in TraceGraph
@yurishkuro

Signed-off-by: GLVS Kiriti <glvskiriti2003369@gmail.com>

GLVSKiriti · 2023-10-21T11:07:01Z

These are three tests which are failing

GLVSKiriti · 2023-10-21T11:19:53Z

traceWithOverlappingChildrenLongerThanParent

and final value of selfTotal is calculated from this

jaeger-ui/packages/jaeger-ui/src/components/TracePage/TraceStatistics/tableValues.tsx

Line 254 in 1f63ee0

oneColumnChange.selfTotal = Math.round((oneColumnChange.selfTotal / 1000) * 100) / 100;

so the final selfTotal = 0.03 but I don't know why it is 10.33 in unit test
traceWithTwoNonOverlappingChildren

similarly selfTotal = 0.03
traceWithSingleChildSpanLongerThanParent

similarly selfTotal = 0.04

GLVSKiriti · 2023-10-21T11:21:44Z

Is this calculations are correct Or am I doing wrong ?

yurishkuro · 2023-10-21T20:37:31Z

Regarding the tests, I would recommend replacing the times / durations in the fixture files with smaller numbers that are easier to grok without a calculator, like start=100000, duration=100 (pick values to retain the overall magnitude, but make the numbers round)

packages/jaeger-ui/src/components/TracePage/TraceStatistics/tableValues.tsx

Signed-off-by: GLVS Kiriti <glvskiriti2003369@gmail.com>

codecov · 2023-10-22T08:01:28Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Files	Coverage Δ
...mponents/TracePage/CriticalPath/testCases/test3.js	`100.00% <100.00%> (ø)`
...mponents/TracePage/TraceStatistics/tableValues.tsx	`98.99% <100.00%> (-0.34%)`	⬇️

📢 Thoughts on this report? Let us know!.

packages/jaeger-ui/src/components/TracePage/TraceStatistics/tableValues.test.js

packages/jaeger-ui/src/components/TracePage/TraceStatistics/tableValues.tsx

Signed-off-by: GLVS Kiriti <glvskiriti2003369@gmail.com>

packages/jaeger-ui/src/components/TracePage/TraceStatistics/tableValues.tsx

...TracePage/TraceStatistics/tableValuesTestTrace/traceWithSingleChildSpanLongerThanParent.json

...nents/TracePage/TraceStatistics/tableValuesTestTrace/traceWithTwoNonOverlappingChildren.json

Signed-off-by: GLVS Kiriti <glvskiriti2003369@gmail.com>

packages/jaeger-ui/src/components/TracePage/TraceStatistics/tableValues.tsx

…bleValues.tsx Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com> Signed-off-by: GLVSKiriti <116095646+GLVSKiriti@users.noreply.github.com>

Signed-off-by: GLVS Kiriti <glvskiriti2003369@gmail.com>

packages/jaeger-ui/src/components/TracePage/TraceStatistics/tableValues.tsx

yurishkuro · 2023-10-23T21:41:29Z

packages/jaeger-ui/src/components/TracePage/TraceStatistics/tableValues.tsx

@@ -315,18 +147,18 @@ function valueFirstDropdown(selectedTagKey: string, trace: Trace) {
    for (let j = 0; j < allSpans.length; j++) {


Just to illustrate my earlier point about linear searches: with this loop in place we're doing O(N^2) work.

Signed-off-by: GLVS Kiriti <glvskiriti2003369@gmail.com>

Signed-off-by: GLVSKiriti <116095646+GLVSKiriti@users.noreply.github.com>

yurishkuro · 2023-10-24T04:55:55Z

packages/jaeger-ui/src/components/TracePage/TraceStatistics/tableValues.tsx

-  tempSelfChange += span.duration - duration;
-
-  return tempSelfChange;
+  return Math.round(spanRange.length / 10);


btw, I don't think rounding is necessary, length should always be divisible by 10.

yurishkuro · 2023-10-24T04:56:31Z

Thanks!

maxgaponov · 2023-11-28T17:31:23Z

Hi, @GLVSKiriti and @yurishkuro . Sorry, I don't quite understand why we need this *10 logic in computeSelfTime function. Can you please explain it for me or give a test case?

We have the comment in the function:

//       range(1, 10).subtract(4, 8) => range([1, 3], [9-10])
//       length=(3-1)+(10-9)=2+1=3

But in my understanding if we have spanA(1, 10) and spanB(4, 8) then we can represent them as range(1, 9) and range(4, 7) accordingly. And range(1, 9).subtract(range(4, 7)) => range([1, 3], [8, 9]) (length=3+2=5 as DRange treats all intervals as inclusive).

yurishkuro · 2023-11-28T20:39:23Z

@maxgaponov you may be right

Enhancement to selfTime calculation logic

b02cde1

Signed-off-by: GLVS Kiriti <glvskiriti2003369@gmail.com>

GLVSKiriti requested a review from a team as a code owner October 21, 2023 10:16

GLVSKiriti requested review from yurishkuro and removed request for a team October 21, 2023 10:16

GLVSKiriti changed the title ~~Enhancement to selfTime calculation logic in TraceStatistics view~~ [Refactor]: Enhancement to selfTime calculation logic in TraceStatistics view Oct 21, 2023

yurishkuro reviewed Oct 21, 2023

View reviewed changes

GLVSKiriti added 2 commits October 22, 2023 12:18

Minimized the code complexity

3db728f

Signed-off-by: GLVS Kiriti <glvskiriti2003369@gmail.com>

Edited test fixtures

53dd0df

Signed-off-by: GLVS Kiriti <glvskiriti2003369@gmail.com>

GLVSKiriti requested a review from yurishkuro October 22, 2023 08:04

yurishkuro reviewed Oct 22, 2023

View reviewed changes

packages/jaeger-ui/src/components/TracePage/TraceStatistics/tableValues.test.js Show resolved Hide resolved

yurishkuro reviewed Oct 22, 2023

View reviewed changes

packages/jaeger-ui/src/components/TracePage/TraceStatistics/tableValues.tsx Outdated Show resolved Hide resolved

Multiplied with 10 to Drange intervals to avoid improper overlaps

79991df

Signed-off-by: GLVS Kiriti <glvskiriti2003369@gmail.com>

GLVSKiriti requested a review from yurishkuro October 22, 2023 20:35

yurishkuro added the changelog:bugfix-or-minor-feature 🐞 Bug fixes, Minor Improvements label Oct 22, 2023

yurishkuro reviewed Oct 23, 2023

View reviewed changes

GLVSKiriti added 3 commits October 23, 2023 09:46

Added comments to explain the logic of the code

ac620db

Signed-off-by: GLVS Kiriti <glvskiriti2003369@gmail.com>

Renamed a trace file name

108829c

Signed-off-by: GLVS Kiriti <glvskiriti2003369@gmail.com>

Modified the code

a1e3b4a

Signed-off-by: GLVS Kiriti <glvskiriti2003369@gmail.com>

GLVSKiriti requested a review from yurishkuro October 23, 2023 04:58

yurishkuro reviewed Oct 23, 2023

View reviewed changes

GLVSKiriti and others added 3 commits October 24, 2023 00:11

Update packages/jaeger-ui/src/components/TracePage/TraceStatistics/ta…

50e0a39

…bleValues.tsx Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com> Signed-off-by: GLVSKiriti <116095646+GLVSKiriti@users.noreply.github.com>

Updated to suggested changes

df59065

Signed-off-by: GLVS Kiriti <glvskiriti2003369@gmail.com>

computeSelfTime function is introduced

c91f7d7

Signed-off-by: GLVS Kiriti <glvskiriti2003369@gmail.com>

GLVSKiriti requested a review from yurishkuro October 23, 2023 19:23

yurishkuro reviewed Oct 23, 2023

View reviewed changes

packages/jaeger-ui/src/components/TracePage/TraceStatistics/tableValues.tsx Outdated Show resolved Hide resolved

yurishkuro reviewed Oct 23, 2023

View reviewed changes

packages/jaeger-ui/src/components/TracePage/TraceStatistics/tableValues.tsx Outdated Show resolved Hide resolved

yurishkuro reviewed Oct 23, 2023

View reviewed changes

Updated code and comments to suggested changes

44a1da7

Signed-off-by: GLVS Kiriti <glvskiriti2003369@gmail.com>

GLVSKiriti requested a review from yurishkuro October 24, 2023 04:42

Merge branch 'main' into selfTimeEnhancementInTraceStatistics

ce1f573

Signed-off-by: GLVSKiriti <116095646+GLVSKiriti@users.noreply.github.com>

yurishkuro approved these changes Oct 24, 2023

View reviewed changes

yurishkuro merged commit 614bfc9 into jaegertracing:main Oct 24, 2023
10 checks passed

GLVSKiriti deleted the selfTimeEnhancementInTraceStatistics branch October 24, 2023 05:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Refactor]: Enhancement to selfTime calculation logic in TraceStatistics view #1901

[Refactor]: Enhancement to selfTime calculation logic in TraceStatistics view #1901

GLVSKiriti commented Oct 21, 2023 •

edited

GLVSKiriti commented Oct 21, 2023

GLVSKiriti commented Oct 21, 2023

GLVSKiriti commented Oct 21, 2023

yurishkuro commented Oct 21, 2023

codecov bot commented Oct 22, 2023 •

edited

yurishkuro Oct 23, 2023

yurishkuro Oct 24, 2023

yurishkuro commented Oct 24, 2023

maxgaponov commented Nov 28, 2023

yurishkuro commented Nov 28, 2023

		@@ -315,18 +147,18 @@ function valueFirstDropdown(selectedTagKey: string, trace: Trace) {
		for (let j = 0; j < allSpans.length; j++) {

[Refactor]: Enhancement to selfTime calculation logic in TraceStatistics view #1901

[Refactor]: Enhancement to selfTime calculation logic in TraceStatistics view #1901

Conversation

GLVSKiriti commented Oct 21, 2023 • edited

Which problem is this PR solving?

Description of the changes

GLVSKiriti commented Oct 21, 2023

GLVSKiriti commented Oct 21, 2023

GLVSKiriti commented Oct 21, 2023

yurishkuro commented Oct 21, 2023

codecov bot commented Oct 22, 2023 • edited

Codecov Report

yurishkuro Oct 23, 2023

Choose a reason for hiding this comment

yurishkuro Oct 24, 2023

Choose a reason for hiding this comment

yurishkuro commented Oct 24, 2023

maxgaponov commented Nov 28, 2023

yurishkuro commented Nov 28, 2023

GLVSKiriti commented Oct 21, 2023 •

edited

codecov bot commented Oct 22, 2023 •

edited