From 524d98e4fac3322b7da3cc865f53043f03f67bb7 Mon Sep 17 00:00:00 2001 From: Amir Blum Date: Fri, 28 Oct 2022 07:35:21 +0300 Subject: [PATCH] fix(graphql): graphql instrumentation throw for sync calls (#1254) --- .../package.json | 1 + .../src/instrumentation.ts | 3 +- .../src/utils.ts | 106 ++++++++++-------- .../test/graphql-adaptor.ts | 42 ++++--- .../test/graphql.test.ts | 95 +++++++++++++++- .../test/schema.ts | 2 +- 6 files changed, 179 insertions(+), 70 deletions(-) diff --git a/plugins/node/opentelemetry-instrumentation-graphql/package.json b/plugins/node/opentelemetry-instrumentation-graphql/package.json index 0777ea5682..31ec14494e 100644 --- a/plugins/node/opentelemetry-instrumentation-graphql/package.json +++ b/plugins/node/opentelemetry-instrumentation-graphql/package.json @@ -50,6 +50,7 @@ "devDependencies": { "@opentelemetry/api": "^1.0.0", "@opentelemetry/sdk-trace-base": "^1.3.1", + "@opentelemetry/semantic-conventions": "^1.3.1", "@types/mocha": "8.2.3", "@types/node": "16.11.21", "graphql": "^16.5.0", diff --git a/plugins/node/opentelemetry-instrumentation-graphql/src/instrumentation.ts b/plugins/node/opentelemetry-instrumentation-graphql/src/instrumentation.ts index efb54b6c87..cec77eefe0 100644 --- a/plugins/node/opentelemetry-instrumentation-graphql/src/instrumentation.ts +++ b/plugins/node/opentelemetry-instrumentation-graphql/src/instrumentation.ts @@ -46,6 +46,7 @@ import { addSpanSource, endSpan, getOperation, + isPromise, wrapFieldResolver, wrapFields, } from './utils'; @@ -249,7 +250,7 @@ export class GraphQLInstrumentation extends InstrumentationBase { return; } - if (result.constructor.name === 'Promise') { + if (isPromise(result)) { (result as Promise).then(resultData => { if (typeof config.responseHook !== 'function') { endSpan(span); diff --git a/plugins/node/opentelemetry-instrumentation-graphql/src/utils.ts b/plugins/node/opentelemetry-instrumentation-graphql/src/utils.ts index 0a938dfb72..f0af42d9ef 100644 --- a/plugins/node/opentelemetry-instrumentation-graphql/src/utils.ts +++ b/plugins/node/opentelemetry-instrumentation-graphql/src/utils.ts @@ -31,6 +31,11 @@ import { const OPERATION_VALUES = Object.values(AllowedOperationTypes); +// https://github.com/graphql/graphql-js/blob/main/src/jsutils/isPromise.ts +export const isPromise = (value: any): value is Promise => { + return typeof value?.then === 'function'; +}; + // eslint-disable-next-line @typescript-eslint/no-explicit-any function addInputVariableAttribute(span: api.Span, key: string, variable: any) { if (Array.isArray(variable)) { @@ -329,6 +334,32 @@ export function wrapFields( }); } +const handleResolveSpanError = ( + resolveSpan: api.Span, + err: any, + shouldEndSpan: boolean +) => { + if (!shouldEndSpan) { + return; + } + resolveSpan.recordException(err); + resolveSpan.setStatus({ + code: api.SpanStatusCode.ERROR, + message: err.message, + }); + resolveSpan.end(); +}; + +const handleResolveSpanSuccess = ( + resolveSpan: api.Span, + shouldEndSpan: boolean +) => { + if (!shouldEndSpan) { + return; + } + resolveSpan.end(); +}; + export function wrapFieldResolver( tracer: api.Tracer, getConfig: () => Required, @@ -380,27 +411,33 @@ export function wrapFieldResolver( return api.context.with( api.trace.setSpan(api.context.active(), field.span), () => { - return safeExecuteInTheMiddleAsync< - | Maybe> - | Promise< - Maybe> - > - >( - () => { - return fieldResolver.call( - this, - source, - args, - contextValue, - info - ) as any; - }, - err => { - if (shouldEndSpan) { - endSpan(field.span, err); - } + try { + const res = fieldResolver.call( + this, + source, + args, + contextValue, + info + ); + if (isPromise(res)) { + return res.then( + (r: any) => { + handleResolveSpanSuccess(field.span, shouldEndSpan); + return r; + }, + (err: Error) => { + handleResolveSpanError(field.span, err, shouldEndSpan); + throw err; + } + ); + } else { + handleResolveSpanSuccess(field.span, shouldEndSpan); + return res; } - ); + } catch (err: any) { + handleResolveSpanError(field.span, err, shouldEndSpan); + throw err; + } } ); } @@ -409,32 +446,3 @@ export function wrapFieldResolver( return wrappedFieldResolver; } - -/** - * Async version of safeExecuteInTheMiddle from instrumentation package - * can be removed once this will be added to instrumentation package - * @param execute - * @param onFinish - * @param preventThrowingError - */ -async function safeExecuteInTheMiddleAsync( - execute: () => T, - onFinish: (e: Error | undefined, result: T | undefined) => void, - preventThrowingError?: boolean -): Promise { - let error: Error | undefined; - let result: T | undefined; - try { - result = await execute(); - } catch (e) { - error = e as Error; - } finally { - onFinish(error, result); - if (error && !preventThrowingError) { - // eslint-disable-next-line no-unsafe-finally - throw error; - } - // eslint-disable-next-line no-unsafe-finally - return result as T; - } -} diff --git a/plugins/node/opentelemetry-instrumentation-graphql/test/graphql-adaptor.ts b/plugins/node/opentelemetry-instrumentation-graphql/test/graphql-adaptor.ts index c0ccac4848..3911f41b3f 100644 --- a/plugins/node/opentelemetry-instrumentation-graphql/test/graphql-adaptor.ts +++ b/plugins/node/opentelemetry-instrumentation-graphql/test/graphql-adaptor.ts @@ -19,11 +19,13 @@ import type { GraphQLTypeResolver, Source, } from 'graphql'; -import { graphql as origGraphql, version } from 'graphql'; +import { + graphql as origAsyncGraphQl, + graphqlSync as origSyncGraphQl, + version, +} from 'graphql'; import { Maybe } from 'graphql/jsutils/Maybe'; -const variantGraphql = origGraphql as Function; - interface GraphQLArgs { schema: GraphQLSchema; source: string | Source; @@ -37,16 +39,26 @@ interface GraphQLArgs { typeResolver?: Maybe>; } +const executeGraphqlQuery = (queryFunc: Function, args: GraphQLArgs) => { + const pre16Version = + !version || version.startsWith('14.') || version.startsWith('15.'); + if (pre16Version) { + return queryFunc( + args.schema, + args.source, + args.rootValue, + args.contextValue, + args.variableValues, + args.operationName, + args.fieldResolver, + args.typeResolver + ); + } else { + return queryFunc(args); + } +}; + export const graphql = (args: GraphQLArgs) => - !version || version.startsWith('14.') || version.startsWith('15.') - ? variantGraphql( - args.schema, - args.source, - args.rootValue, - args.contextValue, - args.variableValues, - args.operationName, - args.fieldResolver, - args.typeResolver - ) - : variantGraphql(args); + executeGraphqlQuery(origAsyncGraphQl, args); +export const graphqlSync = (args: GraphQLArgs) => + executeGraphqlQuery(origSyncGraphQl, args); diff --git a/plugins/node/opentelemetry-instrumentation-graphql/test/graphql.test.ts b/plugins/node/opentelemetry-instrumentation-graphql/test/graphql.test.ts index abdf338112..9828894d89 100644 --- a/plugins/node/opentelemetry-instrumentation-graphql/test/graphql.test.ts +++ b/plugins/node/opentelemetry-instrumentation-graphql/test/graphql.test.ts @@ -20,7 +20,8 @@ import { ReadableSpan, SimpleSpanProcessor, } from '@opentelemetry/sdk-trace-base'; -import { Span } from '@opentelemetry/api'; +import { Span, SpanStatusCode } from '@opentelemetry/api'; +import { SemanticAttributes } from '@opentelemetry/semantic-conventions'; import * as assert from 'assert'; import type * as graphqlTypes from 'graphql'; import { GraphQLInstrumentation } from '../src'; @@ -39,10 +40,11 @@ graphQLInstrumentation.disable(); // now graphql can be required -import { buildSchema } from './schema'; -import { graphql } from './graphql-adaptor'; +import { buildSchema } from 'graphql'; +import { buildTestSchema } from './schema'; +import { graphql, graphqlSync } from './graphql-adaptor'; // Construct a schema, using GraphQL schema language -const schema = buildSchema(); +const schema = buildTestSchema(); const sourceList1 = ` query { @@ -1177,4 +1179,89 @@ describe('graphql', () => { assert.deepStrictEqual(executeSpan.parentSpanId, undefined); }); }); + + describe('graphqlSync', () => { + const simpleSyncSchema = buildSchema(` + type Query { + hello: String + } + `); + + beforeEach(() => { + create({}); + }); + + afterEach(() => { + exporter.reset(); + }); + + it('should instrument successful graphqlSync', () => { + const rootValue = { + hello: () => 'Hello world!', + }; + const source = '{ hello }'; + + const res = graphqlSync({ schema: simpleSyncSchema, rootValue, source }); + assert.deepEqual(res.data, { hello: 'Hello world!' }); + + // validate execute span is present + const spans = exporter.getFinishedSpans(); + const executeSpans = spans.filter(s => s.name === SpanNames.EXECUTE); + assert.deepStrictEqual(executeSpans.length, 1); + const [executeSpan] = executeSpans; + assert.deepStrictEqual( + executeSpan.attributes[AttributeNames.SOURCE], + source + ); + assert.deepStrictEqual( + executeSpan.attributes[AttributeNames.OPERATION_TYPE], + 'query' + ); + }); + + it('should instrument when sync resolver throws', () => { + const rootValue = { + hello: () => { + throw Error('sync resolver error from tests'); + }, + }; + const source = '{ hello }'; + + // graphql will not throw, it will return "errors" in the result and the field will be null + const res = graphqlSync({ schema: simpleSyncSchema, rootValue, source }); + assert.deepEqual(res.data, { hello: null }); + + // assert errors are returned correctly + assert.deepStrictEqual(res.errors?.length, 1); + const resolverError = res.errors?.[0]; + assert.deepStrictEqual(resolverError.path, ['hello']); + assert.deepStrictEqual( + resolverError.message, + 'sync resolver error from tests' + ); + + // assert relevant spans are still created with error indications + const spans = exporter.getFinishedSpans(); + + // single resolve span with error and event for exception + const resolveSpans = spans.filter(s => s.name === SpanNames.RESOLVE); + assert.deepStrictEqual(resolveSpans.length, 1); + const resolveSpan = resolveSpans[0]; + assert.deepStrictEqual(resolveSpan.status.code, SpanStatusCode.ERROR); + assert.deepStrictEqual( + resolveSpan.status.message, + 'sync resolver error from tests' + ); + const resolveEvent = resolveSpan.events[0]; + assert.deepStrictEqual(resolveEvent.name, 'exception'); + assert.deepStrictEqual( + resolveEvent.attributes?.[SemanticAttributes.EXCEPTION_MESSAGE], + 'sync resolver error from tests' + ); + + // single execute span + const executeSpans = spans.filter(s => s.name === SpanNames.EXECUTE); + assert.deepStrictEqual(executeSpans.length, 1); + }); + }); }); diff --git a/plugins/node/opentelemetry-instrumentation-graphql/test/schema.ts b/plugins/node/opentelemetry-instrumentation-graphql/test/schema.ts index f62b21f0d4..51ec9420af 100644 --- a/plugins/node/opentelemetry-instrumentation-graphql/test/schema.ts +++ b/plugins/node/opentelemetry-instrumentation-graphql/test/schema.ts @@ -97,7 +97,7 @@ function prepareData() { prepareData(); -export function buildSchema() { +export function buildTestSchema() { const Author = new graphql.GraphQLObjectType({ name: 'Author', fields: {