perf: Expose `graphql_should_analyze_queries` as setting #3008

justlevine · 2024-01-01T10:07:26Z

What does this implement/fix? Explain your changes.

This PR adds an admin setting for enabling/disabling the Query Analyzer, and defaults the setting value to off on new installs, unless GraphQL Debugging is enabled. In other words by default, the QueryAnalyzer will only be enabled when GraphQL Debugging is also enabled.

The reason for this is the overhead caused by having QueryAnalyzer enabled. While this cost is justified during debugging, or when needed to cache future responses, on vanilla WPGraphQL setups there is no need to pay for that extra processing.

More specifically:

Adds a new query_analyzer_enabled setting to the WPGraphQL Settings page. The setting control is disabled if GraphQL::debug() is true.
Adds a new QueryAnalyzer::is_enabled() static method that can be used to check whether the QA is disabled site wide. The return value in order of priority is:
- GraphQL Debugging enabled => true
- The filtered value of the existing graphql_should_analyze_queries filter.
- The stored setting value.
Adds a new QueryAnalyzer::is_enabled_for_query() instance method that uses a new graphql_should_analyze_query filter which passes the \WPGraphQL\Request object as a parameter, and can be used to enable/disable the QueryAnalyzer for specific requests.
Sets the default query_analyzer_enabled value of true for existing WPGraphQL installs on upgrade, to prevent back-compat issues.
Outputting the data to response.extensions now requires both QueryAnalyzer::is_enabled_for_query() and \WPGraphQL::debug() to be true.

Does this close any currently open issues?

Related #1873

Any relevant logs, error output, GraphiQL screenshots, etc?

Per the below screenshots TTFB decreased from 165.68ms => 125.93ms on a simple query with very few types to analyze.

Before (Query Analyzer enabled by default)

After (Query Analyzer disabled by default)

Settings toggle:

GraphQL Debugging enabled
GraphQL Debugging disabled

Any other comments?

Important

Plugins that rely on the Query Analyzer should update their code with add_action( 'graphql_should_analyze_queries', true ); or do a check around the QueryAnalyzer::is_enabled() method.

I went with a Setting Label of Enable GraphQL Type Tracking, since "Query Analyzer" is the internal feature name, and not self-documenting for end users.
Made the assumption that this will be part of the 1.20.0 release. If not the activation.php method should be updated.
The future potential of graphql_should_analyze_query should make it easier to exclude or set custom rules around caching specific types/models/or even named queries.

Where has this been tested?

Operating System: Ubuntu 20.04 (wsl2 + devilbox + 8.1.15)

WordPress Version: 6.4.2

coveralls · 2024-01-01T10:59:40Z

coverage: 84.753% (-0.05%) from 84.798%
when pulling 8ee0e4c on justlevine:dev/disable-qa-by-default
into 3d3d723 on wp-graphql:develop.

renatonascalves · 2024-01-02T13:15:19Z

I like this! I thought this was off by default.

jasonbahl · 2024-01-08T17:43:10Z

Ok, so this will technically be a breaking change as this feature is needed for folks that are querying via GET and have a caching client (Varnish, etc) or otherwise doing something with the headers that are returned by X-GraphQL-Keys. . .or when folks are using POST requests and have WPGraphQL Smart Cache object cache enabled.

Before merging this, we need to make sure WPGraphQL Smart Cache ensures QueryAnalyzer is "ON" for GET requests or when GraphQL Object cache is available. If that's not already enforced by WPGraphQL Smart Cache, we need to make sure it is and communicate well that WPGraphQL Smart Cache users NEED to update to the version that does that before updating WPGraphQL to the version that switches the default to "OFF"

jasonbahl · 2024-01-08T17:46:45Z

@justlevine lol, after writing my comment above, I realized you already called this out:

🤦🏻‍♂️

justlevine · 2024-01-08T19:32:52Z

@justlevine lol, after writing my comment above, I realized you already called this out:

@jasonbahl sure, but I wasn't as detailed, and seeing it written out makes me worry about the compatibility for non-Smart Cache solutions (this would be a patch change in Smart Cache, so anyone with autoupdates enabled would get both updates and keep compatibility seamless).

Perhaps as a way to preserve back-compat, we can expose this as a setting and only default it to false on new installs. It'l be tech debt come v2.0, but it gets this into the codebase faster than waiting for me to finish the extension-compatibility update checker 🤔

jasonbahl · 2024-01-08T23:26:18Z

Perhaps as a way to preserve back-compat, we can expose this as a setting and only default it to false on new installs

@justlevine this seems sensible to me

…ing status

justlevine · 2024-01-09T14:22:31Z

@jasonbahl Changes made and the PR description updated with the specifics of the new implementation.

I still need to update/add the corresponding WPUnit tests, but in the interim please let me know if you like the new proposed solution.

activation.php

Set default value for get_option

jasonbahl · 2024-01-09T16:00:18Z

@jasonbahl Changes made and the PR description updated with the specifics of the new implementation.

I still need to update/add the corresponding WPUnit tests, but in the interim please let me know if you like the new proposed solution.

@justlevine ya, this looks good! Thanks 🙏🏻

src/Router.php

justlevine · 2024-01-10T11:21:17Z

Tests added - @jasonbahl I believe this is ready for merge 🤞

justlevine · 2024-01-10T11:41:38Z

Overall coverage drop is caused by the increased settings config array, but the actual code-logic covered has gone up:

jasonbahl · 2024-01-10T17:45:32Z

activation.php

 	}

+	// Run any activation scripts before updating the version.
+	graphql_migrate_1_20_0();


Does this run for folks that update via composer too? 🤔

Now that you mention it it might not even work on plugin update and only on actual activation. 🤦

Not sure the best approach here:

If it's non-composer, we could conditional it as a run_once on admin_init, but someone updating via composer would still need to log in.

Or we absorb the db version check on every init, the extra potential hit is annoying, but still less than with QA activated.

Or we just keep the setting and filter logic, and wait until 2.0 to switch the the default behavior to off.

@jasonbahl thoughts?

@justlevine perhaps in the is_enabled_for_query we can add some logic to handle folks that might not have triggered the migrate script.

Like, when we determine if GRAPHQL_DEBUG is enabled, etc. . .

If we are able to determine no value has been set in the db for query_analyzer_enabled we can assume the migrate script hasn't run and/or the user hasn't manually saved the settings yet, so we can have some logic there to compensate for users that didn't trigger the migrate script?

qlty-cloud-legacy · 2024-01-23T00:09:22Z

Code Climate has analyzed commit 8ee0e4c and detected 2 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Complexity	2

View more on Code Climate.

justlevine · 2024-01-23T00:13:15Z

Scope of this PR was reduced to just exposing the toggle/filters/method calls.

A mechanism for updating old DB values on migration will be evaluated/handled independently, to avoid the risks of tech debt by having the solution coupled too deeply to this PR's use case.

@jasonbahl ready for your review again 🤞

jasonbahl · 2024-01-23T01:45:22Z

@justlevine looks good.

I'll open a corresponding Issue and PR to handle enabling when WPGraphQL Smart Cache is active.

jasonbahl · 2024-01-23T01:50:49Z

@justlevine fwiw wp-graphql/wp-graphql-smart-cache#268

While the default will be "on" as of this PR, since it can easily be checked "off" I opened this issue for WPGraphQL Smart Cache to make it more difficult to turn of the analyzer as it would have negative impact for WPGraphQL Smart Cache users if it were disabled.

justlevine requested a review from jasonbahl January 1, 2024 10:07

justlevine added type: enhancement Improvements to existing functionality status: in review Awaiting review before merging or closing scope: api Issues related to access functions, actions, and filters scope: performance Enhancing speed and efficiency labels Jan 1, 2024

justlevine force-pushed the dev/disable-qa-by-default branch 2 times, most recently from fe8a90c to 8452be8 Compare January 1, 2024 10:44

justlevine mentioned this pull request Jan 2, 2024

chore: implement stricter PHPStan config and clean up unnecessary type-guards #3009

Merged

jasonbahl added the compat: possible break There is a possibility that this might lead to breaking changes, but not confirmed yet label Jan 8, 2024

jasonbahl assigned justlevine Jan 8, 2024

justlevine added status: in progress Currently being worked on and removed status: in review Awaiting review before merging or closing labels Jan 8, 2024

justlevine added 3 commits January 9, 2024 09:08

dev: default graphql_should_analyze_queries value to GraphQL Debugg…

95704ee

…ing status

fix: check if query anaylzer is enabled before setting headers

38c079a

test: test graphql keys with debugging enabled

8e9ce6a

justlevine force-pushed the dev/disable-qa-by-default branch from 8452be8 to 8e9ce6a Compare January 9, 2024 09:08

dev: rework as exposed setting

0b82e99

justlevine changed the title ~~perf: default graphql_should_analyze_queries value to GraphQL Debugging status~~ perf: Expose graphql_should_analyze_queries as setting and default to off on new installs Jan 9, 2024

jasonbahl reviewed Jan 9, 2024

View reviewed changes

activation.php Outdated Show resolved Hide resolved

Update activation.php

fc6b1d9

Set default value for get_option

jasonbahl previously approved these changes Jan 9, 2024

View reviewed changes

justlevine commented Jan 9, 2024

View reviewed changes

src/Router.php Outdated Show resolved Hide resolved

Update src/Router.php

2620e07

justlevine dismissed jasonbahl’s stale review via 2620e07 January 9, 2024 16:31

dev: fix edge cases and add unit tests

8ab4aa4

justlevine requested a review from jasonbahl January 10, 2024 11:20

jasonbahl reviewed Jan 10, 2024

View reviewed changes

justlevine mentioned this pull request Jan 22, 2024

feat: introduce "graphql_query_analyzer_get_headers" filter #3020

Merged

revert: don't disable by default / update db

8ee0e4c

justlevine changed the title ~~perf: Expose graphql_should_analyze_queries as setting and default to off on new installs~~ perf: Expose graphql_should_analyze_queries as setting Jan 23, 2024

jasonbahl approved these changes Jan 23, 2024

View reviewed changes

jasonbahl merged commit bed443d into wp-graphql:develop Jan 23, 2024

This was referenced Jan 23, 2024

release: v1.20.0 #3024

Closed

release: v1.20.0 #3025

Merged

jasonbahl mentioned this pull request Feb 7, 2024

feat: force enable query analyzer wp-graphql/wp-graphql-smart-cache#270

Merged

justlevine deleted the dev/disable-qa-by-default branch February 10, 2024 19:38

perf: Expose graphql_should_analyze_queries as setting #3008

perf: Expose graphql_should_analyze_queries as setting #3008

Uh oh!

Conversation

justlevine commented Jan 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this implement/fix? Explain your changes.

Does this close any currently open issues?

Any relevant logs, error output, GraphiQL screenshots, etc?

Before (Query Analyzer enabled by default)

After (Query Analyzer disabled by default)

Settings toggle:

Any other comments?

Where has this been tested?

Uh oh!

coveralls commented Jan 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

renatonascalves commented Jan 2, 2024

Uh oh!

jasonbahl commented Jan 8, 2024

Uh oh!

jasonbahl commented Jan 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justlevine commented Jan 8, 2024

Uh oh!

jasonbahl commented Jan 8, 2024

Uh oh!

justlevine commented Jan 9, 2024

Uh oh!

Uh oh!

jasonbahl commented Jan 9, 2024

Uh oh!

Uh oh!

justlevine commented Jan 10, 2024

Uh oh!

justlevine commented Jan 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasonbahl Jan 10, 2024

Choose a reason for hiding this comment

Uh oh!

justlevine Jan 10, 2024

Choose a reason for hiding this comment

Uh oh!

jasonbahl Jan 11, 2024

Choose a reason for hiding this comment

Uh oh!

qlty-cloud-legacy bot commented Jan 23, 2024

Uh oh!

justlevine commented Jan 23, 2024

Uh oh!

jasonbahl commented Jan 23, 2024

Uh oh!

jasonbahl commented Jan 23, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

perf: Expose `graphql_should_analyze_queries` as setting #3008

perf: Expose `graphql_should_analyze_queries` as setting #3008

justlevine commented Jan 1, 2024 •

edited

Loading

coveralls commented Jan 1, 2024 •

edited

Loading

jasonbahl commented Jan 8, 2024 •

edited

Loading

justlevine commented Jan 10, 2024 •

edited

Loading