DDG: Remove kind.server filter and validate the case of service calling itself #557

rubenvp8510 · 2020-04-04T15:36:15Z

Which problem is this PR solving?

Fixes DDG for search results is still wrong #554

Short description of the changes

I removed the span.kind == server filter, instead validate when the service calls itself multiple times and remove all of those recursive calls and only preserve one of them.

Attached an image.

rubenvp8510 · 2020-04-04T15:37:03Z

Still need to fix some tests.

codecov · 2020-04-14T18:03:25Z

Codecov Report

Merging #557 into master will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #557      +/-   ##
==========================================
- Coverage   92.90%   92.88%   -0.02%     
==========================================
  Files         200      200              
  Lines        4917     4935      +18     
  Branches     1213     1219       +6     
==========================================
+ Hits         4568     4584      +16     
- Misses        309      311       +2     
  Partials       40       40

Impacted Files	Coverage Δ
...jaeger-ui/src/model/ddg/transformTracesToPaths.tsx	`100.00% <100.00%> (ø)`
packages/jaeger-ui/src/utils/TreeNode.js	`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 4c8fc98...0b3bf0c. Read the comment docs.

everett980 · 2020-04-16T17:20:12Z

packages/jaeger-ui/src/model/ddg/transformTracesToPaths.tsx

+            if (reducedSpans.length > 0) {
+              const headSpan = reducedSpans[reducedSpans.length - 1];
+              // Transition inside the same service ServiceA -> ServiceA
+              if (headSpan.processID === span.processID) {


@rubenvp8510 sorry I couldn't get to this sooner

If I'm reading this correctly, it captures every change in processID. Then, if there isn't a change in processID and the current span is of kind server:

a parent client span that was a different process than its parent span gets replaced with the server span

a parent server span is kept and the current span is added.

I'm wondering if we need to do the replacement in case 1, or if it would be better to simply add any span of kind server or any span with a different processID than the previous reduced span.
i.e.:

if (headSpan.processID !== span.processID || isKindServer(span) { reducedSpans.push(span); }

The only case this is different is if process a calls process b which emits a client span as its first span, which is the parent of a server span in the same process. IMO this case is strange, possibly caused by bad instrumentation, and I'd prefer to surface the scenario to potentially fix it rather than bury it.

@yurishkuro @vprithvi I'd like to hear your opinions on the two approaches to handling this case.

@rubenvp8510 Update: I have discussed the two approaches with Yuri and Prithvi and we agree to not replace the ingress client spans.

Here's an example, which, for brevity, I will represent a single span as processID,operationName,[s]erver/[c]lient
e.g.:

{ processID: 0, operationName: 'a', span.kind: 'client', }

would be 0,a,c

original:
0,a,c 1,a,s 1,b,c 1,c,s, 2,a,c, 2,b,s 2,c,c
would be reduced to
0,a,c 1,a,s 1,c,s 2,a,c 2,b,s by my proposed method
versus
0,a,c 1,a,s 1,c,s 2,b,s in the PR

The differences is that the client ingress span (2,a,c) is not dropped.

Hi @everett980 I think it is OK not loses the ingress client span. it just wasn't clear to me what would be the correct way of derivative the paths on those cases, but now it makes more sense. I'll do the pertinent changes.

I did the changes, this is ready for another review.

Thank you!

Signed-off-by: Ruben Vargas <[email protected]>

rubenvp8510 · 2020-04-17T18:59:09Z

This also fixes #546 I added a test for that case.

packages/jaeger-ui/src/model/ddg/transformTracesToPaths.test.js

packages/jaeger-ui/src/model/ddg/transformTracesToPaths.tsx

Signed-off-by: Ruben Vargas <[email protected]>

packages/jaeger-ui/src/model/ddg/transformTracesToPaths.tsx

yurishkuro · 2020-04-23T02:08:22Z

packages/jaeger-ui/src/model/ddg/transformTracesToPaths.tsx

+            reducedSpans.push(span);
+          } else if (
+            reducedSpans[reducedSpans.length - 1].processID !== span.processID ||
+            isKindServer(span)


the whole algorithm expressed in these two lines, awesome!

cc @vprithvi

Signed-off-by: Ruben Vargas <[email protected]>

everett980

@rubenvp8510 looks good! One minor nit and then it's good to go.

everett980 · 2020-04-23T21:32:38Z

packages/jaeger-ui/src/model/ddg/transformTracesToPaths.test.js

@@ -52,7 +60,7 @@ describe('transform traces to ddg paths', () => {
        (result, span) => ({
          ...result,
          [span.processID]: {
-            serviceName: `${span.spanID.split(' ')[0]} service`,
+            serviceName: span.processID,


This doesn't fully test that the serviceNames are populated. change this to

serviceName: ${span.processID}-name,

Done.

Thanks for the review!

Signed-off-by: Ruben Vargas <[email protected]>

everett980

Looks good, thanks for tackling this!

…ng itself (jaegertracing#557) * DDG: Remove kind.server filter and handle service calling itself * Improve tests on transformTracesToPath * Preserve (do not replace) client ingress spans * Add test for root client span * Improve DDG paths algorithm * Use TREE_ROOT_ID on transformTracesToPaths function * Change makeTrace to set service name to [spanID]-name Signed-off-by: Ruben Vargas <[email protected]> Signed-off-by: vvvprabhakar <[email protected]>

rubenvp8510 requested review from everett980 and tiffon as code owners April 4, 2020 15:36

rubenvp8510 force-pushed the Issue-554 branch from 53ab0bc to c9bb9ea Compare April 4, 2020 15:36

rubenvp8510 changed the title ~~DDG: Remove kind.server filter and handle service calling itself~~ DDG: Remove kind.server filter and validate the case of service calling itself Apr 4, 2020

rubenvp8510 changed the title ~~DDG: Remove kind.server filter and validate the case of service calling itself~~ [WIP] DDG: Remove kind.server filter and validate the case of service calling itself Apr 5, 2020

rubenvp8510 force-pushed the Issue-554 branch from c9bb9ea to 1065106 Compare April 6, 2020 01:07

rubenvp8510 changed the title ~~[WIP] DDG: Remove kind.server filter and validate the case of service calling itself~~ DDG: Remove kind.server filter and validate the case of service calling itself Apr 6, 2020

rubenvp8510 force-pushed the Issue-554 branch from 1065106 to ac02294 Compare April 8, 2020 19:57

rubenvp8510 mentioned this pull request Apr 8, 2020

Dedupled search results DDG paths #558

Merged

rubenvp8510 force-pushed the Issue-554 branch 4 times, most recently from 7f0ca32 to 0de01ca Compare April 14, 2020 17:53

rubenvp8510 force-pushed the Issue-554 branch 3 times, most recently from dfd4838 to 468a9d7 Compare April 15, 2020 18:31

everett980 reviewed Apr 16, 2020

View reviewed changes

rubenvp8510 added 4 commits April 17, 2020 13:57

DDG: Remove kind.server filter and handle service calling itself

cd773f5

Signed-off-by: Ruben Vargas <[email protected]>

Improve tests on transformTracesToPath

f6a686a

Signed-off-by: Ruben Vargas <[email protected]>

Preserve (do not replace) client ingress spans

341953a

Signed-off-by: Ruben Vargas <[email protected]>

Add test for root client span

82821d8

Signed-off-by: Ruben Vargas <[email protected]>

rubenvp8510 force-pushed the Issue-554 branch from e762010 to 82821d8 Compare April 17, 2020 18:58

rubenvp8510 requested a review from everett980 April 17, 2020 19:02

yurishkuro reviewed Apr 17, 2020

View reviewed changes

packages/jaeger-ui/src/model/ddg/transformTracesToPaths.test.js Show resolved Hide resolved

yurishkuro reviewed Apr 17, 2020

View reviewed changes

packages/jaeger-ui/src/model/ddg/transformTracesToPaths.test.js Show resolved Hide resolved

yurishkuro reviewed Apr 17, 2020

View reviewed changes

packages/jaeger-ui/src/model/ddg/transformTracesToPaths.tsx Outdated Show resolved Hide resolved

Improve DDG paths algorithm

5c094b6

Signed-off-by: Ruben Vargas <[email protected]>

rubenvp8510 requested a review from yurishkuro April 20, 2020 20:24

yurishkuro reviewed Apr 22, 2020

View reviewed changes

packages/jaeger-ui/src/model/ddg/transformTracesToPaths.tsx Show resolved Hide resolved

yurishkuro approved these changes Apr 23, 2020

View reviewed changes

Use TREE_ROOT_ID on transformTracesToPaths function

5d1e3e4

Signed-off-by: Ruben Vargas <[email protected]>

everett980 requested changes Apr 23, 2020

View reviewed changes

Change makeTrace to set service name to [spanID]-name

0b3bf0c

Signed-off-by: Ruben Vargas <[email protected]>

everett980 approved these changes Apr 24, 2020

View reviewed changes

everett980 merged commit 2a0321e into jaegertracing:master Apr 24, 2020

everett980 mentioned this pull request Apr 27, 2020

DDG should support client root span #546

Closed

cykl mentioned this pull request Apr 28, 2020

DDG should support messaging scenario #547

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DDG: Remove kind.server filter and validate the case of service calling itself #557

DDG: Remove kind.server filter and validate the case of service calling itself #557

rubenvp8510 commented Apr 4, 2020

rubenvp8510 commented Apr 4, 2020

codecov bot commented Apr 14, 2020 •

edited

Loading

everett980 Apr 16, 2020

everett980 Apr 16, 2020

rubenvp8510 Apr 16, 2020

rubenvp8510 Apr 17, 2020

rubenvp8510 commented Apr 17, 2020

yurishkuro Apr 23, 2020

everett980 left a comment

everett980 Apr 23, 2020

rubenvp8510 Apr 23, 2020

everett980 left a comment

DDG: Remove kind.server filter and validate the case of service calling itself #557

DDG: Remove kind.server filter and validate the case of service calling itself #557

Conversation

rubenvp8510 commented Apr 4, 2020

Which problem is this PR solving?

Short description of the changes

rubenvp8510 commented Apr 4, 2020

codecov bot commented Apr 14, 2020 • edited Loading

Codecov Report

everett980 Apr 16, 2020

Choose a reason for hiding this comment

everett980 Apr 16, 2020

Choose a reason for hiding this comment

rubenvp8510 Apr 16, 2020

Choose a reason for hiding this comment

rubenvp8510 Apr 17, 2020

Choose a reason for hiding this comment

rubenvp8510 commented Apr 17, 2020

yurishkuro Apr 23, 2020

Choose a reason for hiding this comment

everett980 left a comment

Choose a reason for hiding this comment

everett980 Apr 23, 2020

Choose a reason for hiding this comment

rubenvp8510 Apr 23, 2020

Choose a reason for hiding this comment

everett980 left a comment

Choose a reason for hiding this comment

codecov bot commented Apr 14, 2020 •

edited

Loading