Correct trace name resolution #541

swapster · 2020-03-09T16:58:00Z

Related to #461.

There is a problem with resolution of a trace name for traces without spans with empty refs (when a "root span" is a span with remote parent).

This PR fixes this problem as suggested in a discussion. To detect a "root span" we:

Find all spans that either have no parent reference, or a parent reference that is not found among all trace's spans.
Sort found spans by start time.
Take a first ("the earliest") span and make a trace name from this span's data.

Additionally, there are some unit tests for a different cases.

codecov · 2020-03-09T17:08:00Z

Codecov Report

Merging #541 into master will decrease coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #541      +/-   ##
==========================================
- Coverage   92.97%   92.94%   -0.03%     
==========================================
  Files         197      197              
  Lines        4840     4848       +8     
  Branches     1174     1177       +3     
==========================================
+ Hits         4500     4506       +6     
- Misses        300      302       +2     
  Partials       40       40

Impacted Files	Coverage Δ
packages/jaeger-ui/src/model/trace-viewer.tsx	`100.00% <100.00%> (ø)`
...eViewer/TimelineHeaderRow/TimelineViewingLayer.tsx	`88.13% <0.00%> (-3.39%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 68e282d...cd8d04c. Read the comment docs.

1. Find all spans without parent reference or with a parent reference that is not found in the trace 2. Sort all found spans by start time (in case when there are more than one span found) 3. Take first of the found spans and get a trace name Signed-off-by: Valerii Varankin <[email protected]>

* Test for trace without root span * Test for trace with exactly one root span (with remote ref) * Test for trace with exactly one root span (with no refs) * Test for trace with more than one root span Signed-off-by: Valerii Varankin <[email protected]>

* File extension changed to .tsx * Path to changed file removed from tsconfig.lint.json * Added type for spans argument in getTraceName() Signed-off-by: Valerii Varankin <[email protected]>

yurishkuro

thanks for the PR

yurishkuro · 2020-03-09T19:54:09Z

packages/jaeger-ui/src/model/find-trace-name.test.js

@@ -0,0 +1,172 @@
+// Copyright (c) 2018 Uber Technologies, Inc.


yurishkuro · 2020-03-09T19:56:51Z

packages/jaeger-ui/src/model/trace-viewer.tsx

+      const parentIDs = sp.references.filter(r => r.refType === 'CHILD_OF').map(r => r.spanID);
+
+      // no parent from trace
+      return !parentIDs.some(pID => allSpanIDs.includes(pID));


Doesn't allSpanIDs.includes do a linear search? Why not construct allSpanIDs as a dictionary {spanID->span}?

Just didn't think about it. But yeap, dictionary makes sense here.
Changed an array to a dict.

yurishkuro · 2020-03-09T19:58:13Z

packages/jaeger-ui/src/model/trace-viewer.tsx

+      // no parent from trace
+      return !parentIDs.some(pID => allSpanIDs.includes(pID));
+    })
+    .sort((sp1, sp2) => sp1.startTime - sp2.startTime)[0];


I don't think this is correct, as it does not give preference to the true root span (that one that has NO parent). It's only correct in the absence of clock skews.

I missed it.

Added a parents check to a sort() method (also, left untouched comparing by a startTime since, I think, it's better than not to sort multiple "root" spans at all if there are several of them).

Also, changed a couple of tests.

yurishkuro · 2020-03-09T19:59:02Z

packages/jaeger-ui/tsconfig.lint.json

@@ -30,7 +30,6 @@
    "src/api/jaeger.js",
    "src/components/SearchTracePage/SearchResults/ResultItem.markers.js",
    "src/model/order-by.js",
-    "src/model/trace-viewer.js",


should the tsx file be added to some other linter config, or will it be automatically linted?

I will be linted since this config file inherits another config with include option.
Please correct me if I'm wrong.

packages/jaeger-ui/src/model/find-trace-name.test.js

yurishkuro · 2020-03-09T20:01:10Z

packages/jaeger-ui/src/model/find-trace-name.test.js

+  const spansWithNoRoots = [
+    {
+      spanID: firstSpanId,
+      startTime: 1583758690000,


I recommend defining a constant t and using deltas like t+10 to vary the timestamps.

Thanks, fixed.

yurishkuro · 2020-03-09T20:02:12Z

packages/jaeger-ui/src/model/find-trace-name.test.js

+      },
+      references: [
+        {
+          spanID: remoteSpanId,


nit: this looks more like missingSpanId rather than remote.

Yes, its' better, fixed.

Fixed a year in a copyright text Signed-off-by: Valerii Varankin <[email protected]>

Renamed variable from "remoteSpanId" to "missingSpanId" Signed-off-by: Valerii Varankin <[email protected]>

Made a constant with base span's timestamp in a tests Signed-off-by: Valerii Varankin <[email protected]>

Usage of dictionary of all spans instead of id's array to check an inclusion Signed-off-by: Valerii Varankin <[email protected]>

Correction of a spans sorting to such order: 1. First, sort spans by a presence of a parent 2. Second, sort spans by a start time Signed-off-by: Valerii Varankin <[email protected]>

* Test for a trace with multiple root spans different by parents * Test for a trace with multiple root spans different by start time Signed-off-by: Valerii Varankin <[email protected]>

Added a process key with an empty object to all test spans to avoid reading property of undefined Signed-off-by: Valerii Varankin <[email protected]>

packages/jaeger-ui/src/model/find-trace-name.test.js

packages/jaeger-ui/src/model/trace-viewer.tsx

packages/jaeger-ui/src/model/find-trace-name.test.js

yurishkuro · 2020-03-11T03:20:52Z

packages/jaeger-ui/src/model/trace-viewer.tsx

+      if (!sp.references || !sp.references.length) {
+        return true;
+      }
+      const parentIDs = sp.references.filter(r => r.refType === 'CHILD_OF').map(r => r.spanID);


What if you dropped filters for CHILD_OF? It's possible to have an async trace using FOLLOWS_FROM. I don't think the algorithm is actually sensitive to the type of reference. If anything, I would filter on r.spanContext.traceID being the same as the current span, because a reference to an external trace is definitely not a parent.

Hm. Did I understand correctly, span.references can't contain refs to a children? I mean, if some span listed as a reference, it can be either a parent or a some missing ref, but not a child?
If so, I suggest to change this logic by this way. There is no need to calculate a dict of all trace spans anymore, since some() can check traceID by itself. If there are no situations when spans (which is an argument to this function) contains more data then all .references, this approach looks more correct.

P. S. "I didn't find spanContext in a types, think it should be r.traceID or r.span.traceID :)"

Sorry, I was wrong.

There may be situations when there are references with same traceID but with incorrect (missing) spanID. It this case such span won't define as a root but it can be a root.

Updated a logic and a tests. Now it should work correct.

Co-Authored-By: Yuri Shkuro <[email protected]> Signed-off-by: Valerii Varankin <[email protected]>

….test.js Co-Authored-By: Yuri Shkuro <[email protected]> Signed-off-by: Valerii Varankin <[email protected]>

Co-Authored-By: Yuri Shkuro <[email protected]> Signed-off-by: Valerii Varankin <[email protected]>

Method of parents definition changed to comparing traceIDs of spans Signed-off-by: Valerii Varankin <[email protected]>

Tests traces data was changed to fit parents definition method by a traceIDs Signed-off-by: Valerii Varankin <[email protected]>

Added a checking of parent span presence among all trace's spans Signed-off-by: Valerii Varankin <[email protected]>

Tests traces data was changed according to changed logic Signed-off-by: Valerii Varankin <[email protected]>

yurishkuro

LGTM !

* Trace name resolution by root span with remote ref 1. Find all spans without parent reference or with a parent reference that is not found in the trace 2. Sort all found spans by start time (in case when there are more than one span found) 3. Take first of the found spans and get a trace name Signed-off-by: Valerii Varankin <[email protected]> * Unit tests for getTraceName() method * Test for trace without root span * Test for trace with exactly one root span (with remote ref) * Test for trace with exactly one root span (with no refs) * Test for trace with more than one root span Signed-off-by: Valerii Varankin <[email protected]> * trace-viewer to .tsx * File extension changed to .tsx * Path to changed file removed from tsconfig.lint.json * Added type for spans argument in getTraceName() Signed-off-by: Valerii Varankin <[email protected]> * Year fix Fixed a year in a copyright text Signed-off-by: Valerii Varankin <[email protected]> * Variable rename Renamed variable from "remoteSpanId" to "missingSpanId" Signed-off-by: Valerii Varankin <[email protected]> * Timestamp constant Made a constant with base span's timestamp in a tests Signed-off-by: Valerii Varankin <[email protected]> * Spans id array to spans dictionary Usage of dictionary of all spans instead of id's array to check an inclusion Signed-off-by: Valerii Varankin <[email protected]> * Correct sort logic Correction of a spans sorting to such order: 1. First, sort spans by a presence of a parent 2. Second, sort spans by a start time Signed-off-by: Valerii Varankin <[email protected]> * Tests for a new sorting logic * Test for a trace with multiple root spans different by parents * Test for a trace with multiple root spans different by start time Signed-off-by: Valerii Varankin <[email protected]> * Empty process object to all test spans Added a process key with an empty object to all test spans to avoid reading property of undefined Signed-off-by: Valerii Varankin <[email protected]> * Update Copyright text in packages/jaeger-ui/src/model/trace-viewer.tsx Co-Authored-By: Yuri Shkuro <[email protected]> Signed-off-by: Valerii Varankin <[email protected]> * Update Copyright text in packages/jaeger-ui/src/model/find-trace-name.test.js Co-Authored-By: Yuri Shkuro <[email protected]> Signed-off-by: Valerii Varankin <[email protected]> * More detailed comment for root span Co-Authored-By: Yuri Shkuro <[email protected]> Signed-off-by: Valerii Varankin <[email protected]> * Note about a loop Co-Authored-By: Yuri Shkuro <[email protected]> Signed-off-by: Valerii Varankin <[email protected]> * Change of parents definition logic Method of parents definition changed to comparing traceIDs of spans Signed-off-by: Valerii Varankin <[email protected]> * Tests correction according to changed logic Tests traces data was changed to fit parents definition method by a traceIDs Signed-off-by: Valerii Varankin <[email protected]> * Checking of parent span presence Added a checking of parent span presence among all trace's spans Signed-off-by: Valerii Varankin <[email protected]> * Tests correction according to changed logic Tests traces data was changed according to changed logic Signed-off-by: Valerii Varankin <[email protected]> Co-authored-by: Yuri Shkuro <[email protected]> Signed-off-by: vvvprabhakar <[email protected]>

swapster requested review from everett980 and tiffon as code owners March 9, 2020 16:58

swapster added 3 commits March 9, 2020 20:56

trace-viewer to .tsx

e968cdb

* File extension changed to .tsx * Path to changed file removed from tsconfig.lint.json * Added type for spans argument in getTraceName() Signed-off-by: Valerii Varankin <[email protected]>

swapster force-pushed the correct-trace-name-resolution branch from 79a7e39 to e968cdb Compare March 9, 2020 18:03

yurishkuro reviewed Mar 9, 2020

View reviewed changes

swapster added 7 commits March 10, 2020 12:28

Year fix

5c633c2

Fixed a year in a copyright text Signed-off-by: Valerii Varankin <[email protected]>

Variable rename

08d8dbb

Renamed variable from "remoteSpanId" to "missingSpanId" Signed-off-by: Valerii Varankin <[email protected]>

Timestamp constant

5793005

Made a constant with base span's timestamp in a tests Signed-off-by: Valerii Varankin <[email protected]>

Spans id array to spans dictionary

52e3794

Usage of dictionary of all spans instead of id's array to check an inclusion Signed-off-by: Valerii Varankin <[email protected]>

Correct sort logic

c914bcf

Correction of a spans sorting to such order: 1. First, sort spans by a presence of a parent 2. Second, sort spans by a start time Signed-off-by: Valerii Varankin <[email protected]>

Tests for a new sorting logic

a6f67ba

* Test for a trace with multiple root spans different by parents * Test for a trace with multiple root spans different by start time Signed-off-by: Valerii Varankin <[email protected]>

Empty process object to all test spans

270a762

Added a process key with an empty object to all test spans to avoid reading property of undefined Signed-off-by: Valerii Varankin <[email protected]>

swapster requested a review from yurishkuro March 10, 2020 10:34

Merge branch 'master' into correct-trace-name-resolution

75552d8

yurishkuro reviewed Mar 11, 2020

View reviewed changes

swapster and others added 4 commits March 11, 2020 11:19

Update Copyright text in packages/jaeger-ui/src/model/trace-viewer.tsx

3944f05

Co-Authored-By: Yuri Shkuro <[email protected]> Signed-off-by: Valerii Varankin <[email protected]>

Update Copyright text in packages/jaeger-ui/src/model/find-trace-name…

f65375f

….test.js Co-Authored-By: Yuri Shkuro <[email protected]> Signed-off-by: Valerii Varankin <[email protected]>

More detailed comment for root span

98b7eb7

Co-Authored-By: Yuri Shkuro <[email protected]> Signed-off-by: Valerii Varankin <[email protected]>

Note about a loop

bfbce5b

Co-Authored-By: Yuri Shkuro <[email protected]> Signed-off-by: Valerii Varankin <[email protected]>

swapster force-pushed the correct-trace-name-resolution branch from 207ec09 to bfbce5b Compare March 11, 2020 08:20

swapster added 2 commits March 11, 2020 12:31

Change of parents definition logic

9aa5b4b

Method of parents definition changed to comparing traceIDs of spans Signed-off-by: Valerii Varankin <[email protected]>

Tests correction according to changed logic

0c49d43

Tests traces data was changed to fit parents definition method by a traceIDs Signed-off-by: Valerii Varankin <[email protected]>

swapster requested a review from yurishkuro March 11, 2020 09:57

swapster added 2 commits March 11, 2020 14:19

Checking of parent span presence

8d3eb81

Added a checking of parent span presence among all trace's spans Signed-off-by: Valerii Varankin <[email protected]>

Tests correction according to changed logic

3b2cbae

Tests traces data was changed according to changed logic Signed-off-by: Valerii Varankin <[email protected]>

yurishkuro approved these changes Mar 12, 2020

View reviewed changes

Merge branch 'master' into correct-trace-name-resolution

cd8d04c

yurishkuro merged commit a9dce8b into jaegertracing:master Mar 12, 2020

swapster mentioned this pull request Mar 13, 2020

Usage of a helper for trace name when preparing trace data #544

Merged

tmthrgd mentioned this pull request Apr 12, 2021

Fix trace name resolution for spans with remote parents #461

Closed

This was referenced Jul 30, 2021

Sync relevant jaeger-ui PRs grafana/grafana#37404

Closed

Jaeger: Sync getTraceName improvements from upstream grafana/grafana#37408

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correct trace name resolution #541

Correct trace name resolution #541

swapster commented Mar 9, 2020

codecov bot commented Mar 9, 2020 •

edited

Loading

yurishkuro left a comment

yurishkuro Mar 9, 2020

swapster Mar 10, 2020

yurishkuro Mar 9, 2020

swapster Mar 10, 2020

yurishkuro Mar 9, 2020

swapster Mar 10, 2020

yurishkuro Mar 9, 2020

swapster Mar 10, 2020

yurishkuro Mar 9, 2020

swapster Mar 10, 2020

yurishkuro Mar 9, 2020

swapster Mar 10, 2020

yurishkuro Mar 11, 2020

swapster Mar 11, 2020

swapster Mar 11, 2020

yurishkuro left a comment

		@@ -0,0 +1,172 @@
		// Copyright (c) 2018 Uber Technologies, Inc.

Correct trace name resolution #541

Correct trace name resolution #541

Conversation

swapster commented Mar 9, 2020

codecov bot commented Mar 9, 2020 • edited Loading

Codecov Report

yurishkuro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro left a comment

Choose a reason for hiding this comment

codecov bot commented Mar 9, 2020 •

edited

Loading