Skip to content

Commit

Permalink
fix: early close of span with graphql execute (#752)
Browse files Browse the repository at this point in the history
  • Loading branch information
@samriley
samriley authored Dec 5, 2021
1 parent 53691e4 commit 4a3b410
Show file tree
Hide file tree
Showing 2 changed files with 61 additions and 36 deletions.
14 changes: 9 additions & 5 deletions plugins/node/opentelemetry-instrumentation-graphql/src/instrumentation.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -245,20 +245,24 @@ export class GraphQLInstrumentation extends InstrumentationBase {
result?: PromiseOrValue<graphqlTypes.ExecutionResult>
) {
const config = this._getConfig();
if (
typeof config.responseHook !== 'function' ||
result === undefined ||
err
) {
if (result === undefined || err) {
endSpan(span, err);
return;
}

if (result.constructor.name === 'Promise') {
(result as Promise<graphqlTypes.ExecutionResult>).then(resultData => {
if (typeof config.responseHook !== 'function') {
endSpan(span);
return;
}
this._executeResponseHook(span, resultData);
});
} else {
if (typeof config.responseHook !== 'function') {
endSpan(span);
return;
}
this._executeResponseHook(span, result as graphqlTypes.ExecutionResult);
}
}
Expand Down
83 changes: 52 additions & 31 deletions plugins/node/opentelemetry-instrumentation-graphql/test/graphql.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ describe('graphql', () => {
});

it('should instrument execute', () => {
const executeSpan = spans[2];
const executeSpan = spans[6];

assert.deepStrictEqual(
executeSpan.attributes[AttributeNames.SOURCE],
Expand All @@ -163,11 +163,11 @@ describe('graphql', () => {
});

it('should instrument resolvers', () => {
const executeSpan = spans[2];
const resolveParentSpan = spans[3];
const span1 = spans[4];
const span2 = spans[5];
const span3 = spans[6];
const executeSpan = spans[6];
const resolveParentSpan = spans[2];
const span1 = spans[3];
const span2 = spans[4];
const span3 = spans[5];

assertResolveSpan(
resolveParentSpan,
Expand Down Expand Up @@ -203,6 +203,27 @@ describe('graphql', () => {
parentId
);
});

it('should execute with correct timing', async () => {
const PARSE = 0;
const VALIDATE = 1;
const RESOLVE = 2;
const EXECUTE = 6;

const times = spans.map(s => {
return {
start: s.startTime[0] * 1_000 + s.startTime[1] / 1_000_000,
end: s.endTime[0] * 1_000 + s.endTime[1] / 1_000_000,
};
});

assert.ok(times[PARSE].start <= times[PARSE].end);
assert.ok(times[PARSE].end <= times[VALIDATE].start);
assert.ok(times[VALIDATE].start <= times[VALIDATE].end);
assert.ok(times[VALIDATE].end <= times[EXECUTE].start);
assert.ok(times[EXECUTE].start <= times[RESOLVE].start);
assert.ok(times[RESOLVE].end <= times[EXECUTE].end);
});
});
describe('AND source is query with param', () => {
let spans: ReadableSpan[];
Expand Down Expand Up @@ -245,7 +266,7 @@ describe('graphql', () => {
});

it('should instrument execute', () => {
const executeSpan = spans[2];
const executeSpan = spans[4];

assert.deepStrictEqual(
executeSpan.attributes[AttributeNames.SOURCE],
Expand All @@ -265,9 +286,9 @@ describe('graphql', () => {
});

it('should instrument resolvers', () => {
const executeSpan = spans[2];
const resolveParentSpan = spans[3];
const span1 = spans[4];
const executeSpan = spans[4];
const resolveParentSpan = spans[2];
const span1 = spans[3];

assertResolveSpan(
resolveParentSpan,
Expand Down Expand Up @@ -331,7 +352,7 @@ describe('graphql', () => {
});

it('should instrument execute', () => {
const executeSpan = spans[2];
const executeSpan = spans[4];

assert.deepStrictEqual(
executeSpan.attributes[AttributeNames.SOURCE],
Expand All @@ -355,9 +376,9 @@ describe('graphql', () => {
});

it('should instrument resolvers', () => {
const executeSpan = spans[2];
const resolveParentSpan = spans[3];
const span1 = spans[4];
const executeSpan = spans[4];
const resolveParentSpan = spans[2];
const span1 = spans[3];

assertResolveSpan(
resolveParentSpan,
Expand Down Expand Up @@ -487,7 +508,7 @@ describe('graphql', () => {
});

it('should instrument execute', () => {
const executeSpan = spans[2];
const executeSpan = spans[4];

assert.deepStrictEqual(
executeSpan.attributes[AttributeNames.SOURCE],
Expand Down Expand Up @@ -574,7 +595,7 @@ describe('graphql', () => {
});

it('should instrument execute', () => {
const executeSpan = spans[2];
const executeSpan = spans[4];

assert.deepStrictEqual(
executeSpan.attributes[AttributeNames.SOURCE],
Expand All @@ -594,9 +615,9 @@ describe('graphql', () => {
});

it('should instrument resolvers', () => {
const executeSpan = spans[2];
const resolveParentSpan = spans[3];
const span1 = spans[4];
const executeSpan = spans[4];
const resolveParentSpan = spans[2];
const span1 = spans[3];

assertResolveSpan(
resolveParentSpan,
Expand Down Expand Up @@ -663,7 +684,7 @@ describe('graphql', () => {
});

it('should instrument execute', () => {
const executeSpan = spans[2];
const executeSpan = spans[4];

assert.deepStrictEqual(
executeSpan.attributes[AttributeNames.SOURCE],
Expand All @@ -686,9 +707,9 @@ describe('graphql', () => {
});

it('should instrument resolvers', () => {
const executeSpan = spans[2];
const resolveParentSpan = spans[3];
const span1 = spans[4];
const executeSpan = spans[4];
const resolveParentSpan = spans[2];
const span1 = spans[3];

assertResolveSpan(
resolveParentSpan,
Expand Down Expand Up @@ -752,7 +773,7 @@ describe('graphql', () => {
});

it('should instrument execute', () => {
const executeSpan = spans[2];
const executeSpan = spans[4];

assert.deepStrictEqual(
executeSpan.attributes[AttributeNames.SOURCE],
Expand All @@ -776,9 +797,9 @@ describe('graphql', () => {
});

it('should instrument resolvers', () => {
const executeSpan = spans[2];
const resolveParentSpan = spans[3];
const span1 = spans[4];
const executeSpan = spans[4];
const resolveParentSpan = spans[2];
const span1 = spans[3];

assertResolveSpan(
resolveParentSpan,
Expand Down Expand Up @@ -847,7 +868,7 @@ describe('graphql', () => {
});

it('should instrument execute', () => {
const executeSpan = spans[2];
const executeSpan = spans[4];

assert.deepStrictEqual(
executeSpan.attributes[AttributeNames.SOURCE],
Expand All @@ -870,9 +891,9 @@ describe('graphql', () => {
});

it('should instrument resolvers', () => {
const executeSpan = spans[2];
const resolveParentSpan = spans[3];
const span1 = spans[4];
const executeSpan = spans[4];
const resolveParentSpan = spans[2];
const span1 = spans[3];

assertResolveSpan(
resolveParentSpan,
Expand Down

0 comments on commit 4a3b410

Please sign in to comment.