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

Prevent DAG crashes because of empty service name string #656

Merged
merged 2 commits into from
Nov 4, 2020
Merged

Prevent DAG crashes because of empty service name string #656

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3b00d30
Prevent DAG crashes because of empty service name string
rubenvp8510 Oct 29, 2020
9a5b510
Adding tests for empty service names
rubenvp8510 Nov 4, 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
31 changes: 20 additions & 11 deletions packages/jaeger-ui/src/components/DependencyGraph/DAG.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,26 +35,34 @@ export default class DAG extends React.Component {
serviceCalls: [],
};

constructor(props) {
super(props);
this.cytoscapeRef = React.createRef();
}

componentDidMount() {
const { serviceCalls } = this.props;
const nodeMap = {};
const nodes = [];
const edges = [];
serviceCalls.forEach(d => {
if (!nodeMap[d.parent]) {
nodes.push({ data: { id: d.parent } });
nodeMap[d.parent] = true;
if (d.parent.trim().length !== 0 && d.child.trim().length !== 0) {
Copy link
Member

Choose a reason for hiding this comment

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

What are the semantics of d.parent and d.child? Could we not ensure elsewhere that they are not empty?

Could you add a unit test to demonstrate that data leads to an issue you're trying to fix?

Copy link
Collaborator Author

@rubenvp8510 rubenvp8510 Oct 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

The way I was able to reproduce the issue is instrumenting a service with a space, (or empty string)

The context of this is d.parent and d.child are service names, I don't think there are other places where we ensure that the service name is not empty, indeed I can see the empty service name in the search bar

Is this considered an instrumentation error? Should we indicate it in some way? If this is consider an error in the instrumentation, may be we should do this validation in other place, more close to when we process the query service response.

I'll add a couple of tests.

Copy link
Member

Choose a reason for hiding this comment

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

imo empty or whitespace service name should be considered an instrumentation error, and we could build a sanitizer to fix it during ingestion.

However, w.r.t. this code that looks things up in a map, an empty or whitespace string is still a string, so why is the code failing on that?

Copy link
Collaborator Author

@rubenvp8510 rubenvp8510 Oct 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I agree that we should build a sanitizer that fix this during ingestion, but also this view should not fail even if there are empty strings.

The error is not on this part, but on cytoscape, which doesn't allow empty strings nor white spaces as node IDs, I'm just filtering those cases here before calling cytoscape a couple of lines below.

if (!nodeMap[d.parent]) {
nodes.push({ data: { id: d.parent } });
nodeMap[d.parent] = true;
}
if (!nodeMap[d.child]) {
nodes.push({ data: { id: d.child } });
nodeMap[d.child] = true;
}
edges.push({
data: { source: d.parent, target: d.child, label: `${d.callCount}` },
});
}
if (!nodeMap[d.child]) {
nodes.push({ data: { id: d.child } });
nodeMap[d.child] = true;
}
edges.push({
data: { source: d.parent, target: d.child, label: `${d.callCount}` },
});
});

cytoscape({
container: document.getElementById('cy'),
container: this.cytoscapeRef.current,
boxSelectionEnabled: false,
autounselectify: true,
layout: {
Expand Down Expand Up @@ -102,6 +110,7 @@ export default class DAG extends React.Component {
left: 0,
top: 0,
}}
ref={this.cytoscapeRef}
/>
);
}
Expand Down
53 changes: 49 additions & 4 deletions packages/jaeger-ui/src/components/DependencyGraph/DAG.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,48 @@
// See the License for the specific language governing permissions and
// limitations under the License.

/* eslint-disable import/first */
jest.mock('cytoscape');

import React from 'react';
import { mount } from 'enzyme';

import DAG from './DAG';

// mock canvas API (we don't care about canvas results)

window.HTMLCanvasElement.prototype.getContext = function() {
return {
fillRect() {},
clearRect() {},
getImageData(x, y, w, h) {
return {
data: new Array(w * h * 4),
};
},
putImageData() {},
createImageData() {
return [];
},
setTransform() {},
drawImage() {},
save() {},
fillText() {},
restore() {},
beginPath() {},
moveTo() {},
lineTo() {},
closePath() {},
stroke() {},
translate() {},
scale() {},
rotate() {},
arc() {},
fill() {},
measureText() {
return { width: 0 };
},
transform() {},
rect() {},
clip() {},
};
};
describe('<DAG>', () => {
it('does not explode', () => {
const serviceCalls = [
Expand All @@ -31,4 +65,15 @@ describe('<DAG>', () => {
];
expect(mount(<DAG serviceCalls={serviceCalls} />)).toBeDefined();
});

it('does not explode with empty strings or string with only spaces', () => {
const serviceCalls = [
{
callCount: 1,
child: '',
parent: ' ',
},
];
expect(mount(<DAG serviceCalls={serviceCalls} />)).toBeDefined();
});
});