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

Jump to bottom

feat(gatsby): Add tracing for graphql resolvers #23589

Merged
merged 3 commits into from
May 12, 2020
Merged

feat(gatsby): Add tracing for graphql resolvers #23589

merged 3 commits into from
May 12, 2020

Conversation

freiksenet
Copy link
Contributor

@freiksenet freiksenet commented Apr 29, 2020 •
edited

Description

This adds per-resolver tracing to GraphQL Resolvers during build. This also traces runQuery parts like materialization, running and inline tracing.

Documentation

Related Issues

@freiksenet freiksenet mentioned this pull request Apr 29, 2020
Closed
@freiksenet freiksenet force-pushed the graphql-tracing-2 branch 2 times, most recently from 7e8415d to 2a8a62e Compare April 29, 2020 14:32
@freiksenet freiksenet marked this pull request as ready for review April 29, 2020 14:33
@freiksenet freiksenet requested a review from a team as a code owner April 29, 2020 14:33
@freiksenet freiksenet changed the title Add tracing for graphql resolvers feat(gatsby): Add tracing for graphql resolvers Apr 29, 2020
pvdz
pvdz reviewed Apr 29, 2020
View reviewed changes
.eslintrc.js Outdated Show resolved Hide resolved
packages/gatsby/src/query/graphql-runner.ts Outdated Show resolved Hide resolved
packages/gatsby/src/query/graphql-runner.ts Outdated Show resolved Hide resolved
packages/gatsby/src/query/graphql-runner.ts Outdated Show resolved Hide resolved
@pvdz
Copy link
Contributor

pvdz commented Apr 29, 2020

I'm really happy with this PR 🎉 Thank you so much! 🌮
It looks good to me, although we should look into the class hoisting thing.

Semantically I will defer to @vladar

vladar
vladar reviewed Apr 30, 2020
View reviewed changes
packages/gatsby/src/query/graphql-runner.ts Show resolved Hide resolved
packages/gatsby/src/schema/resolvers.ts Outdated Show resolved Hide resolved
packages/gatsby/src/schema/resolvers.ts Outdated Show resolved Hide resolved
packages/gatsby/src/schema/resolvers.ts
if (typeof property === `function`) {
return source[info.fieldName](args, context, info)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We didn't have this before - is there a specific reason for this change or is it just to be closer to default GraphQL resolver?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So default resolver was'nt a default resolver before, but default resolver for some directives (weird, I know). Now that we put this resolver on everything, I wanted to make it behave like graphql-js default one.

packages/gatsby/src/schema/schema.js
Comment on lines +858 to +860
resolve: field.resolve
? tracingResolver(field.resolve)
: defaultTracingResolver,
Copy link
Contributor

Choose a reason for hiding this comment

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

We have some logic in node-model which tests for presence of the resolver:

gatsby/packages/gatsby/src/schema/node-model.js

Lines 591 to 602 in 73a4b09

if (gqlField.resolve) {
innerValue = await resolveField(
nodeModel,
schemaComposer,
schema,
node,
gqlField,
fieldName
)
} else {
innerValue = node[fieldName]
}

Can this change have some effects on it? I am not entirely sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it should be same (or actually fixed) behaviour.

packages/gatsby/src/query/graphql-runner.ts Outdated Show resolved Hide resolved
@freiksenet freiksenet requested a review from a team as a code owner May 11, 2020 11:35
@freiksenet freiksenet requested review from vladar and pvdz May 11, 2020 13:16
pvdz
pvdz approved these changes May 11, 2020
View reviewed changes
Copy link
Contributor

@pvdz pvdz left a comment

Choose a reason for hiding this comment

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

Looks good as far as I can see. Can't wait to use this while profiling :)

packages/gatsby/src/schema/context.ts
@@ -0,0 +1,51 @@
import { GraphQLSchema } from "graphql"
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand why you'd do it in this PR but it does make reviews more difficult because github fails to pick up on the rename and has no way to manually ask to compare two files. Please next time do TS conversions in a separate PR.

@freiksenet freiksenet merged commit e124aae into master May 12, 2020
@delete-merged-branch delete-merged-branch bot deleted the graphql-tracing-2 branch May 12, 2020 11:52
@zslabs zslabs mentioned this pull request May 14, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pvdz pvdz pvdz approved these changes

@vladar vladar Awaiting requested review from vladar

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@freiksenet @pvdz @vladar