Fix search results DDG path ordering

[everett980]

Signed-off-by: Everett Ross [email protected]

Which problem is this PR solving?

• Closes Search results DDG looks incorrect for hotrod data #485

Short description of the changes

• Paths were reversed, except last element of path was still last (i.e.: 12345 was rendered as 43215)
• Paths were not filtered by kind, resulting in client span noise

[codecov]

Codecov Report

Merging #504 into master will increase coverage by 0.04%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #504      +/-   ##
==========================================
+ Coverage   92.92%   92.97%   +0.04%     
==========================================
  Files         197      197              
  Lines        4808     4808              
  Branches     1160     1160              
==========================================
+ Hits         4468     4470       +2     
+ Misses        299      297       -2     
  Partials       41       41

Impacted Files	Coverage Δ
.../components/SearchTracePage/SearchResults/index.js	80.76% <ø> (ø)	⬆️
...racePage/TracePageHeader/TracePageHeader.track.tsx	100% <100%> (ø)	⬆️
...nents/TracePage/TracePageHeader/AltViewOptions.tsx	100% <100%> (ø)	⬆️
...s/SearchTracePage/SearchResults/AltViewOptions.tsx	100% <100%> (ø)	⬆️
...eViewer/TimelineHeaderRow/TimelineViewingLayer.tsx	91.52% <0%> (+3.38%)	⬆️

---

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 7ba726b...494e9f6. Read the comment docs.

[yurishkuro]

1. Big nit for test logic: it would be a LOT easier to read if the test used letters to denote spans, rather than spelled out names, e.g. makeTrace([A, B, C]) results in path(A, B, C).
2. I am generally not a fan of service graph logic depending on span.kind tags. An edge between two services can be established purely from the transition between two services, regardless of the span annotations. So I am not clear as to the intention of the added test. May want to check the logic in the memory storage (although this one is for p2p graphs, but the detection is similar: https://github.com/jaegertracing/jaeger/blob/8b1ebe6be8583b86968e28fd9727c53f02fcd62b/plugin/storage/memory/memory.go#L61).
3. Could we introduce some simple JSON-based DSL for describing the traces, to make the data much more readable and the tests data driven? Such as:

{
  "test 1": {
     "traces": [ 
        [
            { "service": "X", "name": "A" },
            { "service": "Y", "name": "B", "parent": "A" },
            { "service": "Z", "name": "C", "parent": "A" },
        ],
     ],
     "expectedPaths": [
        [
            { "service": "X", "name": "A" },
            { "service": "Y", "name": "B" },
        ],
        [
            { "service": "X", "name": "A" },
            { "service": "Z", "name": "C" },
        ],
     ],
   }

Note that trace IDs are largely irrelevant, so can represent them just as arrays of spans, and span names can double as IDs in the DSL.

[everett980]

1. Big nit for test logic: it would be a LOT easier to read if the test used letters to denote spans, rather than spelled out names, e.g. makeTrace([A, B, C]) results in path(A, B, C).

IME with tree-structured data, names that include relationships help with debugging. Granted the test trees here are small so they're not that hard to follow.
Also, test names like kindlessSpan and clientSpan are more clear about their distinguishing detail than C or D. If the spans are A, B, C, and D it's less clear why transformTracesToPaths({ c: makeTrace([A, B, C]), d: makeTrace([A, B, D]) }) results in two paths of length two.

2. I am generally not a fan of service graph logic depending on span.kind tags. An edge between two services can be established purely from the transition between two services, regardless of the span annotations. So I am not clear as to the intention of the added test. May want to check the logic in the memory storage (although this one is for p2p graphs, but the detection is similar: https://github.com/jaegertracing/jaeger/blob/8b1ebe6be8583b86968e28fd9727c53f02fcd62b/plugin/storage/memory/memory.go#L61).

I thought that @vprithvi said we changed the definition of an edge from "change in serviceName" to "span.kind===server spans". There are pros and cons for each approach, but I'd like search results DDGs to match the flink job DDGs. @vprithvi can you confirm which definition we are using? If both definitions are valid, we can have a UI toggle or a config key to determine which rule we use for search results DDGs.

3. Could we introduce some simple JSON-based DSL for describing the traces, to make the data much more readable and the tests data driven? Such as:

{
  "test 1": {
     "traces": [ 
        [
            { "service": "X", "name": "A" },
            { "service": "Y", "name": "B", "parent": "A" },
            { "service": "Z", "name": "C", "parent": "A" },
        ],
     ],
     "expectedPaths": [
        [
            { "service": "X", "name": "A" },
            { "service": "Y", "name": "B" },
        ],
        [
            { "service": "X", "name": "A" },
            { "service": "Z", "name": "C" },
        ],
     ],
   }

Note that trace IDs are largely irrelevant, so can represent them just as arrays of spans, and span names can double as IDs in the DSL.

I agree that the traceGenerator used for tests should be beefed up, but I think that exceeds the scope of this fix.

[yurishkuro]

I'm a bit confused by how service names are assigned in L55. Looks like a string "{first word of span id} service", but the first word of span is, per L47, is span name. So it seems every span in these tests will have a different service name.

[everett980]

Yes every span gets a unique service and operation name. currently service name is not used as part of the trace to path calculation. once we settle on a specification for the flink job we will apply the same rule here and likely need to tweak the test.