Skip to content

Logging support #402

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

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all 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
54 changes: 54 additions & 0 deletions src/__tests__/starWarsQuery-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import { expect } from 'chai';
import { describe, it } from 'mocha';
import { StarWarsSchema } from './starWarsSchema.js';
import { graphql } from '../graphql';
import { TAG } from '../execution';
import {
GraphQLObjectType,
GraphQLNonNull,
Expand Down Expand Up @@ -528,4 +529,57 @@ describe('Star Wars Query Tests', () => {
[ [ 'nullableA', 'nullableA', 'nonNullA', 'nonNullA', 'throws' ] ]);
});
});

describe('Allows to pass in a logger function', () => {
it('Logs on errors', async () => {
const query = `
query HeroNameQuery {
mainHero: hero {
name
story: secretBackstory
}
}
`;

const expected = {
mainHero: {
name: 'R2-D2',
story: null,
}
};

const logged = [];
const expectedLogged = [
[ TAG.RESOLVER_START, [ 'mainHero' ] ],
[ TAG.RESOLVER_END, [ 'mainHero' ] ],
[ TAG.SUBTREE_START, [ 'mainHero' ] ],
[ TAG.RESOLVER_START, [ 'mainHero', 'name' ] ],
[ TAG.RESOLVER_END, [ 'mainHero', 'name' ] ],
[ TAG.SUBTREE_START, [ 'mainHero', 'name' ] ],
[ TAG.SUBTREE_END, [ 'mainHero', 'name' ] ],
[ TAG.RESOLVER_START, [ 'mainHero', 'story' ] ],
[ TAG.RESOLVER_ERROR, [ 'mainHero', 'story' ] ],
[ TAG.RESOLVER_END, [ 'mainHero', 'story' ] ],
[ TAG.SUBTREE_START, [ 'mainHero', 'story' ] ],
[ TAG.SUBTREE_ERROR, [ 'mainHero', 'story' ] ],
[ TAG.SUBTREE_END, [ 'mainHero', 'story' ] ],
[ TAG.SUBTREE_END, [ 'mainHero' ] ],
];

const result = await graphql(
StarWarsSchema,
query,
null,
null,
null,
null,
(tag, payload) => {
const { path } = payload;
logged.push([ tag, path ]);
});

expect(result.data).to.deep.equal(expected);
expect(logged).to.deep.equal(expectedLogged);
});
});
});
87 changes: 77 additions & 10 deletions src/execution/execute.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import isNullish from '../jsutils/isNullish';
import { typeFromAST } from '../utilities/typeFromAST';
import { Kind } from '../language';
import { getVariableValues, getArgumentValues } from './values';
import { TAG, passThroughResultAndLog } from './logging';
import {
GraphQLScalarType,
GraphQLObjectType,
Expand Down Expand Up @@ -88,6 +89,7 @@ type ExecutionContext = {
operation: OperationDefinition;
variableValues: {[key: string]: mixed};
errors: Array<GraphQLError>;
logFn?: (tag: string, payload: mixed, info: mixed) => void
}

/**
Expand All @@ -114,7 +116,8 @@ export function execute(
rootValue?: mixed,
contextValue?: mixed,
variableValues?: ?{[key: string]: mixed},
operationName?: ?string
operationName?: ?string,
logFn?: (tag: string, payload: mixed, info: mixed) => void
): Promise<ExecutionResult> {
invariant(schema, 'Must provide schema');
invariant(
Expand All @@ -131,7 +134,8 @@ export function execute(
rootValue,
contextValue,
variableValues,
operationName
operationName,
logFn
);

// Return a Promise that will eventually resolve to the data described by
Expand Down Expand Up @@ -169,7 +173,8 @@ function buildExecutionContext(
rootValue: mixed,
contextValue: mixed,
rawVariableValues: ?{[key: string]: mixed},
operationName: ?string
operationName: ?string,
logFn?: (tag: string, payload: mixed, info: mixed) => void
): ExecutionContext {
const errors: Array<GraphQLError> = [];
let operation: ?OperationDefinition;
Expand Down Expand Up @@ -216,7 +221,8 @@ function buildExecutionContext(
contextValue,
operation,
variableValues,
errors
errors,
logFn,
};
}

Expand Down Expand Up @@ -577,9 +583,33 @@ function resolveField(
variableValues: exeContext.variableValues,
};

const logFn = exeContext.logFn || (() => {});
logFn(TAG.RESOLVER_START, { path }, info);

// Get the resolve function, regardless of if its result is normal
// or abrupt (error).
const result = resolveOrError(resolveFn, source, args, context, info);
let result = resolveOrError(resolveFn, source, args, context, info);

if (isThenable(result)) {
result = ((result: any): Promise<*>).then(
passThroughResultAndLog(logFn, TAG.RESOLVER_END, { path }, info),
reason => {
logFn(TAG.RESOLVER_ERROR, {
error: reason,
path,
}, info);
logFn(TAG.RESOLVER_END, { path });
return Promise.reject(reason);
});
} else {
if (result instanceof Error) {
logFn(TAG.RESOLVER_ERROR, {
error: result,
path,
}, info);
}
logFn(TAG.RESOLVER_END, { path }, info);
}

return completeValueCatchingError(
exeContext,
Expand Down Expand Up @@ -619,17 +649,36 @@ function completeValueCatchingError(
path: Array<string | number>,
result: mixed
): mixed {
const logFn = exeContext.logFn || (() => {});
logFn(TAG.SUBTREE_START, { path }, info);

// If the field type is non-nullable, then it is resolved without any
// protection from errors.
if (returnType instanceof GraphQLNonNull) {
return completeValue(
const completed = completeValue(
exeContext,
returnType,
fieldASTs,
info,
path,
result
);

if (isThenable(completed)) {
return ((completed: any): Promise<*>).then(
passThroughResultAndLog(logFn, TAG.SUBTREE_END, { path }, info),
reason => {
logFn(TAG.SUBTREE_ERROR, {
error: reason,
path,
}, info);
logFn(TAG.SUBTREE_END, { path }, info);
return Promise.reject(reason);
});
}

logFn(TAG.SUBTREE_END, { path }, info);
return completed;
}

// Otherwise, error protection is applied, logging the error and resolving
Expand All @@ -648,16 +697,34 @@ function completeValueCatchingError(
// error and resolve to null.
// Note: we don't rely on a `catch` method, but we do expect "thenable"
// to take a second callback for the error case.
return ((completed: any): Promise<*>).then(undefined, error => {
exeContext.errors.push(error);
return Promise.resolve(null);
});
return ((completed: any): Promise<*>).then(
passThroughResultAndLog(logFn, TAG.SUBTREE_END, { path }, info),
error => {
exeContext.errors.push(error);
logFn(TAG.SUBTREE_ERROR, {
error,
path
}, info);
logFn(TAG.SUBTREE_END, { path }, info);

return Promise.resolve(null);
});
}

logFn(TAG.SUBTREE_END, { path }, info);

return completed;
} catch (error) {
// If `completeValue` returned abruptly (threw an error), log the error
// and return null.
exeContext.errors.push(error);

logFn(TAG.SUBTREE_ERROR, {
error,
path
}, info);
logFn(TAG.SUBTREE_END, { path }, info);

return null;
}
}
Expand Down
1 change: 1 addition & 0 deletions src/execution/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,4 @@
*/

export { execute } from './execute';
export { TAG } from './logging';
32 changes: 32 additions & 0 deletions src/execution/logging.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
/* @flow */
/**
* Copyright (c) 2015, Facebook, Inc.
* All rights reserved.
*
* This source code is licensed under the BSD-style license found in the
* LICENSE file in the root directory of this source tree. An additional grant
* of patent rights can be found in the PATENTS file in the same directory.
*/

export const TAG = {
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the purpose of logging RESOLVER_ERROR or SUBTREE_ERROR if you've already got the exeContext.errors Array?

SUBTREE_START: 'SUBTREE_START',
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't a SUBTREE just a non-leaf node? I don't understand what the reason to log here is since the RESOLVER will log immediately after. Seems redundant but I'm probably missing something.

SUBTREE_END: 'SUBTREE_END',
SUBTREE_ERROR: 'SUBTREE_ERROR',
RESOLVER_START: 'RESOLVER_START',
RESOLVER_END: 'RESOLVER_END',
RESOLVER_ERROR: 'RESOLVER_ERROR',
};

export type GraphQLLoggingTagType = $Keys<typeof TAG>; // eslint-disable-line

export function passThroughResultAndLog(
logFn: any,
tag: GraphQLLoggingTagType,
payload: any,
info: any
): any {
return function (result) {
logFn(tag, payload, info);
return result;
};
}
10 changes: 8 additions & 2 deletions src/graphql.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,14 +39,19 @@ import type { GraphQLSchema } from './type/schema';
* The name of the operation to use if requestString contains multiple
* possible operations. Can be omitted if requestString contains only
* one operation.
* logFn:
* The function that is called with a tag of an event, an event payload
* and other execution information.
* The examples include: errors thrown, before and after the resolver call.
*/
export function graphql(
schema: GraphQLSchema,
requestString: string,
rootValue?: mixed,
contextValue?: mixed,
variableValues?: ?{[key: string]: mixed},
operationName?: ?string
operationName?: ?string,
logFn?: (tag: string, payload: mixed, info: mixed) => void
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like there are two main cases that this PR will need to account for:

  • Timing information
  • Inspection of poorly performing resolvers

Timings
Since the logging of *_START and *_END are separate, in order to measure resolver timings similar to what Apollo Optics does, this function should probably guarantee either a key like query:<path>:<fieldName> or enough information about a resolver to construct one. I see that it accepts path (in some places) already, but that is likely not quite enough, especially if multiple queries are running at once.

Inspection
My guess is that it will eventually be useful to folks to log all sorts of information about poorly performing resolvers, much more than just the path or key. For example, if an Array resolver has a search/filter argument, it would be helpful to know which filters are running slowly. Rather than a payload and info of type mixed, why not just pass the same or similar objects to what a resolver function receives?

): Promise<GraphQLResult> {
return new Promise(resolve => {
const source = new Source(requestString || '', 'GraphQL request');
Expand All @@ -62,7 +67,8 @@ export function graphql(
rootValue,
contextValue,
variableValues,
operationName
operationName,
logFn
)
);
}
Expand Down
1 change: 1 addition & 0 deletions src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,7 @@ export {
// Execute GraphQL queries.
export {
execute,
TAG
} from './execution';


Expand Down