Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix search results DDG path ordering #504

Merged
merged 6 commits into from
Jan 23, 2020
Merged

Fix search results DDG path ordering #504

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ab945ba
Reverse and filter trace paths
everett980 Dec 19, 2019
a43ccfc
Merge branch 'master' into ddg-fix-search-results-paths
everett980 Dec 19, 2019
ffab3ee
Merge branch 'master' into ddg-fix-search-results-paths
everett980 Jan 6, 2020
ac9cffb
Hardens tests to ensure path contents
everett980 Jan 14, 2020
7ba726b
Improve test variable names
everett980 Jan 15, 2020
494e9f6
Merge branch 'master' into ddg-fix-search-results-paths
everett980 Jan 23, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
164 changes: 91 additions & 73 deletions packages/jaeger-ui/src/model/ddg/transformTracesToPaths.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,14 @@
import transformTracesToPaths from './transformTracesToPaths';

describe('transform traces to ddg paths', () => {
const makeSpan = (spanName, childOf) => ({
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) => ({
everett980 marked this conversation as resolved.
Show resolved Hide resolved
hasChildren: true,
operationName: `${spanName} operation`,
processID: `${spanName} processID`,
Expand All @@ -28,6 +35,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) => ({
Expand All @@ -45,109 +61,111 @@ describe('transform traces to ddg paths', () => {
traceID,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

},
});
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, 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([makePath([rootSpan, focalSpan], clientTrace), makePath([rootSpan, focalSpan], kindlessTrace)])
);
});
});
26 changes: 14 additions & 12 deletions packages/jaeger-ui/src/model/ddg/transformTracesToPaths.tsx
View file
Open in desktop
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
Open in desktop
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;
}