Skip to content

Commit

Permalink
Fix search results DDG path ordering (jaegertracing#504)
Browse files Browse the repository at this point in the history 
* Reverse and filter trace paths

Signed-off-by: Everett Ross <reverett@uber.com>

* Hardens tests to ensure path contents

Signed-off-by: Everett Ross <reverett@uber.com>

* Improve test variable names

Signed-off-by: Everett Ross <reverett@uber.com>

Signed-off-by: vvvprabhakar <vvvprabhakar@gmail.com>
  • Loading branch information
@everett980
everett980 committed Jan 23, 2020
1 parent b09a5da commit e4561dc
Show file tree
Hide file tree
Showing 3 changed files with 115 additions and 92 deletions.
173 changes: 97 additions & 76 deletions packages/jaeger-ui/src/model/ddg/transformTracesToPaths.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,19 +15,35 @@
import transformTracesToPaths from './transformTracesToPaths';

describe('transform traces to ddg paths', () => {
const makeSpan = (spanName, childOf) => ({
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, 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,
},
]
: [],
tags:
kind === false
? []
: [
{
key: 'span.kind',
value: kind === undefined ? 'server' : kind,
},
],
spanID: `${spanName} spanID`,
});
const makeTrace = (spans, traceID) => ({
Expand All @@ -45,109 +61,114 @@ 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 = makeExpectedPath([rootSpan, focalSpan], shortTrace);
const longerTrace = makeTrace([rootSpan, focalSpan, followsFocalSpan], 'longerTraceID');
const longerPath = makeExpectedPath([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([makeExpectedPath([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([makeExpectedPath([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([
makeExpectedPath([rootSpan, focalSpan, alsoFollowsFocalSpan], branchingTrace),
makeExpectedPath([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, focalSpan, 'client');
clientSpan.hasChildren = false;
const clientTrace = makeTrace([rootSpan, focalSpan, clientSpan], 'clientTraceID');

const kindlessSpan = makeSpan(badSpanName, focalSpan, false);
kindlessSpan.hasChildren = false;
const kindlessTrace = makeTrace([rootSpan, focalSpan, kindlessSpan], 'kindlessTraceID');

const { dependencies: result } = transformTracesToPaths(makeTraces(clientTrace, kindlessTrace), focalSvc);
expect(new Set(result)).toEqual(
new Set([
makeExpectedPath([rootSpan, focalSpan], clientTrace),
makeExpectedPath([rootSpan, focalSpan], kindlessTrace),
])
);
});
});
26 changes: 14 additions & 12 deletions packages/jaeger-ui/src/model/ddg/transformTracesToPaths.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<string, FetchedTrace>,
focalService: string,
focalOperation: string | undefined
focalOperation?: string
): TDdgPayload {
const dependencies: TDdgPayloadPath[] = [];
Object.values(traces).forEach(({ data }) => {
Expand All @@ -42,20 +36,28 @@ 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(({ processID, operationName: operation }) => ({
service: data.processes[processID].serviceName,
operation,
}));

if (
path.some(
({ service, operation }) =>
service === focalService && (!focalOperation || operation === focalOperation)
)
) {
// TODO: Paths should be deduped with all traceIDs #503
dependencies.push({
path,
attributes: [
Expand Down
8 changes: 4 additions & 4 deletions packages/jaeger-ui/src/utils/span-ancestor-ids.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,12 @@ function getFirstAncestor(span: Span): Span | TNil {
}

export default function spanAncestorIds(span: Span | TNil): string[] {
if (!span) return [];
const ancestorIDs: Set<string> = 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;
}

0 comments on commit e4561dc

Please sign in to comment.