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.

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

Correct trace name resolution #541

Merged
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
172 changes: 172 additions & 0 deletions packages/jaeger-ui/src/model/find-trace-name.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,172 @@
// Copyright (c) 2018 Uber Technologies, Inc.
Copy link
Member

Choose a reason for hiding this comment

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

2020

Copy link
Contributor Author

Choose a reason for hiding this comment

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

//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

import { getTraceName } from './trace-viewer';

describe('getTraceName', () => {
const firstSpanId = 'firstSpanId';
const secondSpanId = 'secondSpanId';
const thirdSpanId = 'thirdSpanId';
const remoteSpanId = 'remoteSpanId';

const serviceName = 'serviceName';
const operationName = 'operationName';

const spansWithNoRoots = [
swapster marked this conversation as resolved.
Show resolved Hide resolved
{
spanID: firstSpanId,
startTime: 1583758690000,
Copy link
Member

Choose a reason for hiding this comment

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

I recommend defining a constant t and using deltas like t+10 to vary the timestamps.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, fixed.

references: [
{
spanID: secondSpanId,
refType: 'CHILD_OF',
},
],
},
{
spanID: secondSpanId,
startTime: 1583758680000,
references: [
{
spanID: thirdSpanId,
refType: 'CHILD_OF',
},
],
},
{
spanID: thirdSpanId,
startTime: 1583758670000,
references: [
{
spanID: firstSpanId,
yurishkuro marked this conversation as resolved.
Show resolved Hide resolved
refType: 'CHILD_OF',
},
],
},
];
const spansWithMultipleRoots = [
{
spanID: firstSpanId,
startTime: 1583758690000,
references: [
{
spanID: thirdSpanId,
refType: 'CHILD_OF',
},
],
},
{
spanID: secondSpanId, // root span
startTime: 1583758680000,
},
{
spanID: thirdSpanId, // root span
startTime: 1583758670000,
operationName,
process: {
serviceName,
},
references: [
{
spanID: remoteSpanId,
Copy link
Member

Choose a reason for hiding this comment

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

nit: this looks more like missingSpanId rather than remote.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, its' better, fixed.

refType: 'CHILD_OF',
},
],
},
];
const spansWithOneRootWithRemoteRef = [
{
spanID: firstSpanId,
startTime: 1583758690000,
references: [
{
spanID: secondSpanId,
refType: 'CHILD_OF',
},
],
},
{
spanID: secondSpanId,
startTime: 1583758680000,
references: [
{
spanID: thirdSpanId,
refType: 'CHILD_OF',
},
],
},
{
spanID: thirdSpanId, // root span
swapster marked this conversation as resolved.
Show resolved Hide resolved
startTime: 1583758670000,
operationName,
process: {
serviceName,
},
references: [
{
spanID: remoteSpanId,
refType: 'CHILD_OF',
},
],
},
];
const spansWithOneRootWithNoRefs = [
{
spanID: firstSpanId,
startTime: 1583758690000,
references: [
{
spanID: thirdSpanId,
refType: 'CHILD_OF',
},
],
},
{
spanID: secondSpanId, // root span
startTime: 1583758680000,
operationName,
process: {
serviceName,
},
},
{
spanID: thirdSpanId,
startTime: 1583758670000,
references: [
{
spanID: secondSpanId,
refType: 'CHILD_OF',
},
],
},
];

const fullTraceName = `${serviceName}: ${operationName}`;

it('returns an empty string if given spans with no root among them', () => {
expect(getTraceName(spansWithNoRoots)).toEqual('');
});

it('returns an id of root span with the earliest startTime', () => {
expect(getTraceName(spansWithMultipleRoots)).toEqual(fullTraceName);
});

it('returns an id of root span with remote ref', () => {
expect(getTraceName(spansWithOneRootWithRemoteRef)).toEqual(fullTraceName);
});

it('returns an id of root span with no refs', () => {
expect(getTraceName(spansWithOneRootWithNoRefs)).toEqual(fullTraceName);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,22 @@
// See the License for the specific language governing permissions and
// limitations under the License.

import { Span } from '../types/trace';

// eslint-disable-next-line import/prefer-default-export
export function getTraceName(spans) {
const span = spans.filter(sp => !sp.references || !sp.references.length)[0];
return span ? `${span.process.serviceName}: ${span.operationName}` : '';
export function getTraceName(spans: Span[]) {
const allSpanIDs = spans.map(sp => sp.spanID);
const rootSpan = spans
.filter(sp => {
if (!sp.references || !sp.references.length) {
return true;
}
const parentIDs = sp.references.filter(r => r.refType === 'CHILD_OF').map(r => r.spanID);
Copy link
Member

Choose a reason for hiding this comment

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

What if you dropped filters for CHILD_OF? It's possible to have an async trace using FOLLOWS_FROM. I don't think the algorithm is actually sensitive to the type of reference. If anything, I would filter on r.spanContext.traceID being the same as the current span, because a reference to an external trace is definitely not a parent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm. Did I understand correctly, span.references can't contain refs to a children? I mean, if some span listed as a reference, it can be either a parent or a some missing ref, but not a child?
If so, I suggest to change this logic by this way. There is no need to calculate a dict of all trace spans anymore, since some() can check traceID by itself. If there are no situations when spans (which is an argument to this function) contains more data then all .references, this approach looks more correct.

P. S. "I didn't find spanContext in a types, think it should be r.traceID or r.span.traceID :)"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I was wrong.

There may be situations when there are references with same traceID but with incorrect (missing) spanID. It this case such span won't define as a root but it can be a root.

Updated a logic and a tests. Now it should work correct.


// no parent from trace
return !parentIDs.some(pID => allSpanIDs.includes(pID));
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't allSpanIDs.includes do a linear search? Why not construct allSpanIDs as a dictionary {spanID->span}?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just didn't think about it. But yeap, dictionary makes sense here.
Changed an array to a dict.

})
.sort((sp1, sp2) => sp1.startTime - sp2.startTime)[0];
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is correct, as it does not give preference to the true root span (that one that has NO parent). It's only correct in the absence of clock skews.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I missed it.

Added a parents check to a sort() method (also, left untouched comparing by a startTime since, I think, it's better than not to sort multiple "root" spans at all if there are several of them).

Also, changed a couple of tests.


return rootSpan ? `${rootSpan.process.serviceName}: ${rootSpan.operationName}` : '';
}
1 change: 0 additions & 1 deletion packages/jaeger-ui/tsconfig.lint.json
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@
"src/api/jaeger.js",
"src/components/SearchTracePage/SearchResults/ResultItem.markers.js",
"src/model/order-by.js",
"src/model/trace-viewer.js",
Copy link
Member

Choose a reason for hiding this comment

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

should the tsx file be added to some other linter config, or will it be automatically linted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will be linted since this config file inherits another config with include option.
Please correct me if I'm wrong.

"src/selectors/process.js",
"src/selectors/span.js",
"src/selectors/trace.js",
Expand Down