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.

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

fix(graphql-server): Improve GraphQL API performance by not accessing project config in makeMergedSchema #9032

Merged
merged 1 commit into from
Aug 11, 2023

Conversation

Josh-Walker-GM
Copy link
Collaborator

Problem
The toml config is not always available in serverless deploy environments and because makeMergedSchema is called on every request (in such deploy environments) then throwing & catching a JS error causes a heavy performance decrease.

Changes

  1. Base the opentelemetry functionality on the graphql plugin options alone.

Notes

  1. Will have to update the experimental docs. You must disable both toml and graphql options now as toml does not override/impact the functionality.

@Josh-Walker-GM Josh-Walker-GM added the release:fix This PR is a fix label Aug 11, 2023
@Josh-Walker-GM Josh-Walker-GM added this to the next-release-patch milestone Aug 11, 2023
@dthyresson dthyresson changed the title fix(graphql-server): Do not access toml config fix(graphql-server): Improve GraphQL API performance by not accessing project config in makeMergedSchema Aug 11, 2023
Copy link
Contributor

@dthyresson dthyresson left a comment

Choose a reason for hiding this comment

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

@Josh-Walker-GM and I paired one this PR and determined by running many k6 benchmark tests that accessing the project config was costly for two reasons: 1) access file system and 2) when deployed in serverless like Netlify or Vercel since there is no file system throwing an error adds to the cost.

We realized that the GraphQL handler already had options to "use open telemetry" and if that was present then the plugin was added -- and that fact could be used to determine to enable and wrap resolvers if needed. Thus no project config access is needed and performance return to v5.4.3 levels.

@Josh-Walker-GM
Copy link
Collaborator Author

For reference:
image

@Josh-Walker-GM Josh-Walker-GM merged commit 01346f9 into main Aug 11, 2023
27 checks passed
@Josh-Walker-GM Josh-Walker-GM deleted the jgmw-graphql/otel-fix branch August 11, 2023 18:23
@jtoar jtoar modified the milestones: next-release-patch, v6.0.6 Aug 11, 2023
jtoar pushed a commit that referenced this pull request Aug 11, 2023
… project config in makeMergedSchema (#9032)

**Problem**
The toml config is not always available in serverless deploy
environments and because `makeMergedSchema` is called on every request
(in such deploy environments) then throwing & catching a JS error causes
a heavy performance decrease.

**Changes**
1. Base the opentelemetry functionality on the graphql plugin options
alone.

**Notes**
1. Will have to update the experimental docs. You must disable both toml
and graphql options now as toml does not override/impact the
functionality.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:fix This PR is a fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants