fix(graphql): graphql instrumentation throw for sync calls (#1254)

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",

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<graphqlTypes.ExecutionResult>).then(resultData => {
         if (typeof config.responseHook !== 'function') {
           endSpan(span);

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<unknown> => {
+  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<TSource = any, TContext = any, TArgs = any>(
   tracer: api.Tracer,
   getConfig: () => Required<GraphQLInstrumentationConfig>,
@@ -380,27 +411,33 @@ export function wrapFieldResolver<TSource = any, TContext = any, TArgs = any>(
     return api.context.with(
       api.trace.setSpan(api.context.active(), field.span),
       () => {
-        return safeExecuteInTheMiddleAsync<
-          | Maybe<graphqlTypes.GraphQLFieldResolver<TSource, TContext, TArgs>>
-          | Promise<
-              Maybe<graphqlTypes.GraphQLFieldResolver<TSource, TContext, TArgs>>
-            >
-        >(
-          () => {
-            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<TSource = any, TContext = any, TArgs = any>(
 
   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<T>(
-  execute: () => T,
-  onFinish: (e: Error | undefined, result: T | undefined) => void,
-  preventThrowingError?: boolean
-): Promise<T> {
-  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;
-  }
-}

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<GraphQLTypeResolver<any, any>>;
 }
 
+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);

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);
+    });
+  });
 });

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: {