From ab945ba09636edf5c39fde9b523f1fe9b5b6155d Mon Sep 17 00:00:00 2001 From: Everett Ross Date: Thu, 19 Dec 2019 16:32:41 -0500 Subject: [PATCH 1/3] Reverse and filter trace paths Signed-off-by: Everett Ross --- .../model/ddg/transformTracesToPaths.test.js | 34 ++++++++++++++++++- .../src/model/ddg/transformTracesToPaths.tsx | 13 ++++--- .../jaeger-ui/src/utils/span-ancestor-ids.tsx | 8 ++--- 3 files changed, 46 insertions(+), 9 deletions(-) diff --git a/packages/jaeger-ui/src/model/ddg/transformTracesToPaths.test.js b/packages/jaeger-ui/src/model/ddg/transformTracesToPaths.test.js index 05970215e7..712be4670b 100644 --- a/packages/jaeger-ui/src/model/ddg/transformTracesToPaths.test.js +++ b/packages/jaeger-ui/src/model/ddg/transformTracesToPaths.test.js @@ -15,7 +15,7 @@ import transformTracesToPaths from './transformTracesToPaths'; describe('transform traces to ddg paths', () => { - const makeSpan = (spanName, childOf) => ({ + const makeSpan = (spanName, childOf, kind) => ({ hasChildren: true, operationName: `${spanName} operation`, processID: `${spanName} processID`, @@ -28,6 +28,15 @@ describe('transform traces to ddg paths', () => { }, ] : [], + tags: + kind === false + ? [] + : [ + { + key: 'span.kind', + value: kind === undefined ? 'server' : kind, + }, + ], spanID: `${spanName} spanID`, }); const makeTrace = (spans, traceID) => ({ @@ -150,4 +159,27 @@ describe('transform traces to ddg paths', () => { const { dependencies: result } = transformTracesToPaths(traces, 'child service'); expect(result.length).toBe(1); }); + + it("omits span if tags does not have span.kind === 'server'", () => { + const badSpanName = 'test bad span name'; + const clientSpan = makeSpan(badSpanName, childSpan, 'client'); + clientSpan.hasChildren = false; + const clientTraceID = 'test client trace ID'; + const clientTrace = makeTrace([rootSpan, childSpan, clientSpan], clientTraceID); + const kindlessSpan = makeSpan(badSpanName, childSpan, false); + kindlessSpan.hasChildren = false; + const kindlessTraceID = 'test kindless trace ID'; + const kindlessTrace = makeTrace([rootSpan, childSpan, kindlessSpan], kindlessTraceID); + + const traces = { + [clientTraceID]: clientTrace, + [kindlessTraceID]: kindlessTrace, + }; + const { dependencies: result } = transformTracesToPaths(traces, 'child service'); + expect(result.length).toBe(2); + expect(result[0].path.length).toBe(clientTrace.data.spans.length - 1); + expect(result[0].path.some(({ operation }) => operation.startsWith(badSpanName))).toBe(false); + expect(result[1].path.length).toBe(kindlessTrace.data.spans.length - 1); + expect(result[1].path.some(({ operation }) => operation.startsWith(badSpanName))).toBe(false); + }); }); diff --git a/packages/jaeger-ui/src/model/ddg/transformTracesToPaths.tsx b/packages/jaeger-ui/src/model/ddg/transformTracesToPaths.tsx index d5e5d4b514..bba00fe927 100644 --- a/packages/jaeger-ui/src/model/ddg/transformTracesToPaths.tsx +++ b/packages/jaeger-ui/src/model/ddg/transformTracesToPaths.tsx @@ -42,13 +42,17 @@ function transformTracesToPaths( return !span.hasChildren; }) .forEach(leaf => { - const path = spanAncestorIds(leaf).map(id => { + const spans = spanAncestorIds(leaf).map(id => { const span = spanMap.get(id); if (!span) throw new Error(`Ancestor spanID ${id} not found in trace ${traceID}`); - - return convertSpan(span, data); + return span; }); - path.push(convertSpan(leaf, data)); + spans.reverse(); + spans.push(leaf); + + const path: TDdgPayloadEntry[] = spans + .filter(span => span.tags.find(({ key, value }) => key === 'span.kind' && value === 'server')) + .map(span => convertSpan(span, data)); if ( path.some( @@ -56,6 +60,7 @@ function transformTracesToPaths( service === focalService && (!focalOperation || operation === focalOperation) ) ) { + // TODO: Paths should be deduped with all traceIDs #503 dependencies.push({ path, attributes: [ diff --git a/packages/jaeger-ui/src/utils/span-ancestor-ids.tsx b/packages/jaeger-ui/src/utils/span-ancestor-ids.tsx index fa999b3fce..a3fe0d7e72 100644 --- a/packages/jaeger-ui/src/utils/span-ancestor-ids.tsx +++ b/packages/jaeger-ui/src/utils/span-ancestor-ids.tsx @@ -29,12 +29,12 @@ function getFirstAncestor(span: Span): Span | TNil { } export default function spanAncestorIds(span: Span | TNil): string[] { - if (!span) return []; - const ancestorIDs: Set = new Set(); + const ancestorIDs: string[] = []; + if (!span) return ancestorIDs; let ref = getFirstAncestor(span); while (ref) { - ancestorIDs.add(ref.spanID); + ancestorIDs.push(ref.spanID); ref = getFirstAncestor(ref); } - return Array.from(ancestorIDs); + return ancestorIDs; } From ac9cffb5cf8b352fef3e43609c0b2993d076099b Mon Sep 17 00:00:00 2001 From: Everett Ross Date: Tue, 14 Jan 2020 15:47:14 -0500 Subject: [PATCH 2/3] Hardens tests to ensure path contents Signed-off-by: Everett Ross --- .../model/ddg/transformTracesToPaths.test.js | 162 ++++++++---------- .../src/model/ddg/transformTracesToPaths.tsx | 15 +- 2 files changed, 80 insertions(+), 97 deletions(-) diff --git a/packages/jaeger-ui/src/model/ddg/transformTracesToPaths.test.js b/packages/jaeger-ui/src/model/ddg/transformTracesToPaths.test.js index 712be4670b..963dd41f8b 100644 --- a/packages/jaeger-ui/src/model/ddg/transformTracesToPaths.test.js +++ b/packages/jaeger-ui/src/model/ddg/transformTracesToPaths.test.js @@ -15,6 +15,13 @@ import transformTracesToPaths from './transformTracesToPaths'; describe('transform traces to ddg paths', () => { + const makePath = (pathSpans, trace) => ({ + path: pathSpans.map(({ processID, operationName: operation }) => ({ + service: trace.data.processes[processID].serviceName, + operation, + })), + attributes: [{ key: 'exemplar_trace_id', value: trace.data.traceID }], + }); const makeSpan = (spanName, childOf, kind) => ({ hasChildren: true, operationName: `${spanName} operation`, @@ -54,132 +61,111 @@ describe('transform traces to ddg paths', () => { traceID, }, }); + const makeTraces = (...traces) => + traces.reduce((res, trace) => ({ ...res, [trace.data.traceID]: trace }), {}); - const linearTraceID = 'linearTraceID'; + const branchingTraceID = 'branchingTraceID'; const missTraceID = 'missTraceID'; - const shortTraceID = 'shortTraceID'; const rootSpan = makeSpan('root'); - const childSpan = makeSpan('child', rootSpan); - const grandchildSpan = makeSpan('grandchild', childSpan); + const focalSpan = makeSpan('focal', rootSpan); + const followsFocalSpan = makeSpan('followsFocal', focalSpan); + followsFocalSpan.hasChildren = false; + const notPathSpan = makeSpan('notPath', rootSpan); + notPathSpan.hasChildren = false; - it('transforms single short trace result payload', () => { - const traces = { - [shortTraceID]: makeTrace([rootSpan, { ...childSpan, hasChildren: false }], shortTraceID), - }; + const shortTrace = makeTrace([rootSpan, { ...focalSpan, hasChildren: false }], 'shortTraceID'); + const shortPath = makePath([rootSpan, focalSpan], shortTrace); + const longerTrace = makeTrace([rootSpan, focalSpan, followsFocalSpan], 'longerTraceID'); + const longerPath = makePath([rootSpan, focalSpan, followsFocalSpan], longerTrace); + const missTrace = makeTrace([rootSpan, notPathSpan], missTraceID); - const { dependencies: result } = transformTracesToPaths(traces, 'child service'); - expect(result.length).toBe(1); - expect(result[0].path.length).toBe(2); + const focalSvc = shortTrace.data.processes[focalSpan.processID].serviceName; + + it('transforms single short trace result payload', () => { + const { dependencies: result } = transformTracesToPaths(makeTraces(shortTrace), focalSvc); + expect(result).toEqual([shortPath]); }); it('transforms multiple traces result payload', () => { - const traces = { - [shortTraceID]: makeTrace([rootSpan, { ...childSpan, hasChildren: false }], shortTraceID), - [linearTraceID]: makeTrace( - [rootSpan, childSpan, { ...grandchildSpan, hasChildren: false }], - linearTraceID - ), - }; - - const { dependencies: result } = transformTracesToPaths(traces, 'child service'); - expect(result.length).toBe(2); - expect(result[0].path.length).toBe(2); - expect(result[1].path.length).toBe(3); + const { dependencies: result } = transformTracesToPaths(makeTraces(shortTrace, longerTrace), focalSvc); + expect(new Set(result)).toEqual(new Set([shortPath, longerPath])); }); it('ignores paths without focalService', () => { - const branchingTraceID = 'branchingTraceID'; - const uncleSpan = makeSpan('uncle', rootSpan); - uncleSpan.hasChildren = false; - const traces = { - [missTraceID]: makeTrace([rootSpan, childSpan, uncleSpan], missTraceID), - [branchingTraceID]: makeTrace( - [rootSpan, childSpan, uncleSpan, { ...grandchildSpan, hasChildren: false }], - branchingTraceID - ), - }; + const branchingTrace = makeTrace([rootSpan, focalSpan, notPathSpan, followsFocalSpan], branchingTraceID); - const { dependencies: result } = transformTracesToPaths(traces, 'child service'); - expect(result.length).toBe(1); - expect(result[0].path.length).toBe(3); + const { dependencies: result } = transformTracesToPaths(makeTraces(missTrace, branchingTrace), focalSvc); + expect(result).toEqual([makePath([rootSpan, focalSpan, followsFocalSpan], branchingTrace)]); }); it('matches service and operation names', () => { - const childSpanWithDiffOp = { - ...childSpan, + const focalSpanWithDiffOp = { + ...focalSpan, hasChildren: false, operationName: 'diff operation', }; - const traces = { - [missTraceID]: makeTrace([rootSpan, childSpanWithDiffOp], missTraceID), - [linearTraceID]: makeTrace( - [rootSpan, childSpan, { ...grandchildSpan, hasChildren: false }], - linearTraceID - ), - }; - - const { dependencies: result } = transformTracesToPaths(traces, 'child service'); - expect(result.length).toBe(2); - expect(result[0].path.length).toBe(2); - expect(result[1].path.length).toBe(3); - - const { dependencies: resultWithOp } = transformTracesToPaths(traces, 'child service', 'child operation'); - expect(resultWithOp.length).toBe(1); - expect(resultWithOp[0].path.length).toBe(3); + const diffOpTrace = makeTrace([rootSpan, focalSpanWithDiffOp], 'diffOpTraceID'); + const traces = makeTraces(diffOpTrace, longerTrace); + + const { dependencies: result } = transformTracesToPaths(traces, focalSvc); + expect(new Set(result)).toEqual( + new Set([makePath([rootSpan, focalSpanWithDiffOp], diffOpTrace), longerPath]) + ); + + const { dependencies: resultWithFocalOp } = transformTracesToPaths( + traces, + focalSvc, + focalSpan.operationName + ); + expect(resultWithFocalOp).toEqual([longerPath]); }); it('transforms multiple paths from single trace', () => { - const traces = { - [linearTraceID]: makeTrace( - [rootSpan, { ...childSpan, hasChildren: false }, { ...grandchildSpan, hasChildren: false }], - linearTraceID - ), - }; - - const { dependencies: result } = transformTracesToPaths(traces, 'child service'); - expect(result.length).toBe(2); - expect(result[0].path.length).toBe(2); - expect(result[1].path.length).toBe(3); + const alsoFollowsFocalSpan = makeSpan('alsoFollows', focalSpan); + alsoFollowsFocalSpan.hasChildren = false; + const branchingTrace = makeTrace( + [rootSpan, focalSpan, followsFocalSpan, alsoFollowsFocalSpan], + branchingTraceID + ); + + const { dependencies: result } = transformTracesToPaths(makeTraces(branchingTrace), focalSvc); + expect(new Set(result)).toEqual( + new Set([ + makePath([rootSpan, focalSpan, alsoFollowsFocalSpan], branchingTrace), + makePath([rootSpan, focalSpan, followsFocalSpan], branchingTrace), + ]) + ); }); it('errors if span has ancestor id not in trace data', () => { - const traces = { - [linearTraceID]: makeTrace([rootSpan, { ...grandchildSpan, hasChildren: false }], linearTraceID), - }; - - expect(() => transformTracesToPaths(traces, 'child service')).toThrowError(/Ancestor spanID.*not found/); + const traces = makeTraces(makeTrace([rootSpan, followsFocalSpan], missTraceID)); + expect(() => transformTracesToPaths(traces, focalSvc)).toThrowError(/Ancestor spanID.*not found/); }); it('skips trace without data', () => { const traces = { - [shortTraceID]: makeTrace([rootSpan, { ...childSpan, hasChildren: false }], shortTraceID), + ...makeTraces(shortTrace), noData: {}, }; - const { dependencies: result } = transformTracesToPaths(traces, 'child service'); + const { dependencies: result } = transformTracesToPaths(traces, focalSvc); expect(result.length).toBe(1); }); it("omits span if tags does not have span.kind === 'server'", () => { const badSpanName = 'test bad span name'; - const clientSpan = makeSpan(badSpanName, childSpan, 'client'); + + const clientSpan = makeSpan(badSpanName, focalSpan, 'client'); clientSpan.hasChildren = false; - const clientTraceID = 'test client trace ID'; - const clientTrace = makeTrace([rootSpan, childSpan, clientSpan], clientTraceID); - const kindlessSpan = makeSpan(badSpanName, childSpan, false); + const clientTrace = makeTrace([rootSpan, focalSpan, clientSpan], 'clientTraceID'); + + const kindlessSpan = makeSpan(badSpanName, focalSpan, false); kindlessSpan.hasChildren = false; - const kindlessTraceID = 'test kindless trace ID'; - const kindlessTrace = makeTrace([rootSpan, childSpan, kindlessSpan], kindlessTraceID); + const kindlessTrace = makeTrace([rootSpan, focalSpan, kindlessSpan], 'kindlessTraceID'); - const traces = { - [clientTraceID]: clientTrace, - [kindlessTraceID]: kindlessTrace, - }; - const { dependencies: result } = transformTracesToPaths(traces, 'child service'); - expect(result.length).toBe(2); - expect(result[0].path.length).toBe(clientTrace.data.spans.length - 1); - expect(result[0].path.some(({ operation }) => operation.startsWith(badSpanName))).toBe(false); - expect(result[1].path.length).toBe(kindlessTrace.data.spans.length - 1); - expect(result[1].path.some(({ operation }) => operation.startsWith(badSpanName))).toBe(false); + const { dependencies: result } = transformTracesToPaths(makeTraces(clientTrace, kindlessTrace), focalSvc); + expect(new Set(result)).toEqual( + new Set([makePath([rootSpan, focalSpan], clientTrace), makePath([rootSpan, focalSpan], kindlessTrace)]) + ); }); }); diff --git a/packages/jaeger-ui/src/model/ddg/transformTracesToPaths.tsx b/packages/jaeger-ui/src/model/ddg/transformTracesToPaths.tsx index bba00fe927..76e7b10c59 100644 --- a/packages/jaeger-ui/src/model/ddg/transformTracesToPaths.tsx +++ b/packages/jaeger-ui/src/model/ddg/transformTracesToPaths.tsx @@ -18,18 +18,12 @@ import spanAncestorIds from '../../utils/span-ancestor-ids'; import { TDdgPayloadEntry, TDdgPayloadPath, TDdgPayload } from './types'; import { FetchedTrace } from '../../types'; -import { Span, Trace } from '../../types/trace'; - -function convertSpan(span: Span, trace: Trace): TDdgPayloadEntry { - const serviceName = trace.processes[span.processID].serviceName; - const operationName = span.operationName; - return { service: serviceName, operation: operationName }; -} +import { Span } from '../../types/trace'; function transformTracesToPaths( traces: Record, focalService: string, - focalOperation: string | undefined + focalOperation?: string ): TDdgPayload { const dependencies: TDdgPayloadPath[] = []; Object.values(traces).forEach(({ data }) => { @@ -52,7 +46,10 @@ function transformTracesToPaths( const path: TDdgPayloadEntry[] = spans .filter(span => span.tags.find(({ key, value }) => key === 'span.kind' && value === 'server')) - .map(span => convertSpan(span, data)); + .map(({ processID, operationName: operation }) => ({ + service: data.processes[processID].serviceName, + operation, + })); if ( path.some( From 7ba726bdf4ec73604fc1c1f01010d787fe8a3d09 Mon Sep 17 00:00:00 2001 From: Everett Ross Date: Wed, 15 Jan 2020 16:56:39 -0500 Subject: [PATCH 3/3] Improve test variable names Signed-off-by: Everett Ross --- .../model/ddg/transformTracesToPaths.test.js | 27 ++++++++++--------- 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/packages/jaeger-ui/src/model/ddg/transformTracesToPaths.test.js b/packages/jaeger-ui/src/model/ddg/transformTracesToPaths.test.js index 963dd41f8b..9980512ed0 100644 --- a/packages/jaeger-ui/src/model/ddg/transformTracesToPaths.test.js +++ b/packages/jaeger-ui/src/model/ddg/transformTracesToPaths.test.js @@ -15,23 +15,23 @@ import transformTracesToPaths from './transformTracesToPaths'; describe('transform traces to ddg paths', () => { - const makePath = (pathSpans, trace) => ({ + const makeExpectedPath = (pathSpans, trace) => ({ path: pathSpans.map(({ processID, operationName: operation }) => ({ service: trace.data.processes[processID].serviceName, operation, })), attributes: [{ key: 'exemplar_trace_id', value: trace.data.traceID }], }); - const makeSpan = (spanName, childOf, kind) => ({ + const makeSpan = (spanName, parent, kind) => ({ hasChildren: true, operationName: `${spanName} operation`, processID: `${spanName} processID`, - references: childOf + references: parent ? [ { refType: 'CHILD_OF', - span: childOf, - spanID: childOf.spanID, + span: parent, + spanID: parent.spanID, }, ] : [], @@ -74,9 +74,9 @@ describe('transform traces to ddg paths', () => { notPathSpan.hasChildren = false; const shortTrace = makeTrace([rootSpan, { ...focalSpan, hasChildren: false }], 'shortTraceID'); - const shortPath = makePath([rootSpan, focalSpan], shortTrace); + const shortPath = makeExpectedPath([rootSpan, focalSpan], shortTrace); const longerTrace = makeTrace([rootSpan, focalSpan, followsFocalSpan], 'longerTraceID'); - const longerPath = makePath([rootSpan, focalSpan, followsFocalSpan], longerTrace); + const longerPath = makeExpectedPath([rootSpan, focalSpan, followsFocalSpan], longerTrace); const missTrace = makeTrace([rootSpan, notPathSpan], missTraceID); const focalSvc = shortTrace.data.processes[focalSpan.processID].serviceName; @@ -95,7 +95,7 @@ describe('transform traces to ddg paths', () => { const branchingTrace = makeTrace([rootSpan, focalSpan, notPathSpan, followsFocalSpan], branchingTraceID); const { dependencies: result } = transformTracesToPaths(makeTraces(missTrace, branchingTrace), focalSvc); - expect(result).toEqual([makePath([rootSpan, focalSpan, followsFocalSpan], branchingTrace)]); + expect(result).toEqual([makeExpectedPath([rootSpan, focalSpan, followsFocalSpan], branchingTrace)]); }); it('matches service and operation names', () => { @@ -109,7 +109,7 @@ describe('transform traces to ddg paths', () => { const { dependencies: result } = transformTracesToPaths(traces, focalSvc); expect(new Set(result)).toEqual( - new Set([makePath([rootSpan, focalSpanWithDiffOp], diffOpTrace), longerPath]) + new Set([makeExpectedPath([rootSpan, focalSpanWithDiffOp], diffOpTrace), longerPath]) ); const { dependencies: resultWithFocalOp } = transformTracesToPaths( @@ -131,8 +131,8 @@ describe('transform traces to ddg paths', () => { const { dependencies: result } = transformTracesToPaths(makeTraces(branchingTrace), focalSvc); expect(new Set(result)).toEqual( new Set([ - makePath([rootSpan, focalSpan, alsoFollowsFocalSpan], branchingTrace), - makePath([rootSpan, focalSpan, followsFocalSpan], branchingTrace), + makeExpectedPath([rootSpan, focalSpan, alsoFollowsFocalSpan], branchingTrace), + makeExpectedPath([rootSpan, focalSpan, followsFocalSpan], branchingTrace), ]) ); }); @@ -165,7 +165,10 @@ describe('transform traces to ddg paths', () => { const { dependencies: result } = transformTracesToPaths(makeTraces(clientTrace, kindlessTrace), focalSvc); expect(new Set(result)).toEqual( - new Set([makePath([rootSpan, focalSpan], clientTrace), makePath([rootSpan, focalSpan], kindlessTrace)]) + new Set([ + makeExpectedPath([rootSpan, focalSpan], clientTrace), + makeExpectedPath([rootSpan, focalSpan], kindlessTrace), + ]) ); }); });