Skip to content

feat: only eagerlyLoadType on introspection requests #3172

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
Merged
jasonbahl merged 9 commits into from
Jul 19, 2024
Merged

feat: only eagerlyLoadType on introspection requests #3172

jasonbahl merged 9 commits into from
Jul 19, 2024

Conversation

@jasonbahl
Copy link
Collaborator

@jasonbahl jasonbahl commented Jul 9, 2024 •
edited
Loading

What does this implement/fix? Explain your changes.

Any type that is registered to eagerlyLoadType will now only be eagerly loaded for Introspection requests (typically used for tooling such as GraphiQL IDE, Codegen, etc)

For standard execution of queries the eagerly loaded types will not be eagerly loaded which dramatically reduces the number of Object Types that are required to be generated per request.

This introduces:

  • WPGraphQL::set_is_introspection_query
  • WPGraphQL::is_introspection_query

Does this close any currently open issues?

Closes #3159

Any other comments?

This might be a breaking change if shipped as-is, as the Schema is now generated later in the lifecycle.

Performance Tests

I ran some (super basic) performance tests using k6.

(performance-test.js) 
```js
import http from 'k6/http'
import { check, sleep } from 'k6'

export const options = {
    vus: 10,
    duration: '10s',
};

export default function() {
    const url = 'http://uri-debugging.local/graphql'
    const query = `
    query {
        posts(first: 10) {
            nodes {
                __typename
                title
                date
            }
        }
    }
    `;


    // format the query as a query param and append to the url
    // const res = http.get(`${url}?query=${encodeURIComponent(query)}`)

    const headers = { 'Content-Type': 'application/json' }
    const res = http.post(url, JSON.stringify({ query: query }), { headers: headers })
    sleep(1)

    check(res, {
        'status was 200': r => r.status === 200,
        'transaction time OK (less than 200ms)': r => r.timings.duration < 200
    });

}
```

The test simulates 10 concurrent users executing the following query as a POST request and continues to execute as many times as it can for 10 seconds.

    query {
        posts(first: 10) {
            nodes {
                __typename
                title
                date
            }
        }
    }

BEFORE

http_req_duration..............: avg=156.21ms min=52.89ms med=109.72ms max=585.73ms p(90)=331.21ms p(95)=371.64ms

AFTER

http_req_duration..............: avg=67.21ms  min=44.3ms  med=56.27ms max=279.04ms p(90)=70.38ms p(95)=164.29ms

@jasonbahl
- move schema generation to just before execution begins
de2e5f2 
- introduce WPGraphQL::set_is_introspection_query and WPGraphQL::is_introspection_query
- Prevent eagerly loading types if the request is not an introspection request
- adjust tests to get the schema using WPGraphQL::get_schema instead of $request->schema
- adjust PostObjectMutationTest which was failing because of invalid mutation, not because the error preventing the post from being deleted
- update RequestTest to expect an exception
@jasonbahl jasonbahl added the compat: possible break There is a possibility that this might lead to breaking changes, but not confirmed yet label Jul 9, 2024
@jasonbahl jasonbahl self-assigned this Jul 9, 2024
@coveralls
Copy link

coveralls commented Jul 10, 2024 •
edited
Loading

Coverage Status

coverage: 84.174% (-0.03%) from 84.207%
when pulling 5095d41 on jasonbahl:feat/3159-eagerly-load-types-for-introspection-only
into d011c9a on wp-graphql:develop.

@jasonbahl jasonbahl mentioned this pull request Jul 11, 2024
Closed
3 tasks
justlevine
justlevine reviewed Jul 12, 2024
View reviewed changes
justlevine
justlevine reviewed Jul 12, 2024
View reviewed changes
justlevine
justlevine reviewed Jul 12, 2024
View reviewed changes
@justlevine
Copy link
Collaborator

justlevine commented Jul 12, 2024

Tests passing against this branch for:

  • wp-graphql-gravity-forms
  • wp-graphql-facetwp
  • wp-graphql-headless-login
  • wp-graphql-rank-math

Of them, Rank Math is strangely experiencing a significant slowdown on interface queries, but that could also be something with my local machine. I'll try to run some k6/xhprof benchmarks over the weekend and see if I can spot a real discrepancy between this branch and develop

@jasonbahl
Copy link
Collaborator Author

jasonbahl commented Jul 12, 2024

@justlevine sounds good! Ya as much as I want to get this out ASAP I want to get it "right" more than get it out fast

@justlevine
Copy link
Collaborator

justlevine commented Jul 14, 2024

@jasonbahl not sure if its because I'm stuck trying to run these tests on LocalWP + Windows, but while i'm seeing marginal->modest improvements on object queries, queries that rely on interfaces seem to be slower. Would recommend you do some performance comparisons locally as well.

DMing you the test conditions + data I used.

@qlty-cloud-legacy
Copy link

qlty-cloud-legacy bot commented Jul 19, 2024

Code Climate has analyzed commit 5095d41 and detected 0 issues on this pull request.

View more on Code Climate.

@jasonbahl jasonbahl merged commit 4fb1694 into wp-graphql:develop Jul 19, 2024
@jasonbahl jasonbahl changed the title fix: only eagerlyLoadType on introspection requests feat: only eagerlyLoadType on introspection requests Aug 8, 2024
This was referenced Aug 8, 2024
Merged
Merged
@justlevine justlevine mentioned this pull request Aug 24, 2024
Closed
@kenseiatwork kenseiatwork mentioned this pull request Jan 27, 2025
Open
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@justlevine justlevine justlevine left review comments

@josephfusco josephfusco Awaiting requested review from josephfusco

@kidunot89 kidunot89 Awaiting requested review from kidunot89

Assignees

@jasonbahl jasonbahl

Labels

compat: possible break There is a possibility that this might lead to breaking changes, but not confirmed yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Eagerly Load Types should only be true for introspection requests

3 participants

@jasonbahl @coveralls @justlevine