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 performance when parsing GraphQL schema into AST which causes downtime on cache stampede #31879

Conversation

convenient
Copy link
Contributor

This is still a bit work in progress as we need to discuss some of the testing I think.

There is a performance issue when reading all the graphql schema and stitching them together, each new schema adds significant runtime and on a PWA/headless magento site with many customisation this adds up.

Usually this is well hidden because the parsed schema are held in cache but when a cache flush occurs while many graphql requests are occurring the system can suffer a cache stampede which brings down the site.

In this pull request I improve the performance by over 80% percentage on vanilla, but this would be more in a customised magento instance.

Related Pull Requests

The symptoms

When the cache is flushed when the site is under load we experience significant slowdown of the graphql requests which cause a minute or so downtime. Each graphql request tries to parse the configuration and store it in the cache at the same time, and as this process is CPU intensive it causes the issue.

In this screenshot we can see that some of the requests take 89 seconds to process all the graphql schema.
They also go alongside large spikes
Screenshot 2021-01-28 at 11 25 02
download
Screenshot 2021-01-19 at 15 22 55

We would not expect a cache flush to cause this problem, as the site performs well in all other aspects.

The problem

$knownTypes = [];
foreach ($schemaFiles as $filePath => $partialSchemaContent) {
$partialSchemaTypes = $this->parseTypes($partialSchemaContent);
// Keep declarations from current partial schema, add missing declarations from all previously read schemas
$knownTypes = $partialSchemaTypes + $knownTypes;
$schemaContent = implode("\n", $knownTypes);
$partialResults = $this->readPartialTypes($schemaContent);
$results = array_replace_recursive($results, $partialResults);
$results = $this->addModuleNameToTypes($results, $filePath);
}
$results = $this->copyInterfaceFieldsToConcreteTypes($results);
return $results;

The magento GraphQLSchemaStitching logic currently works like so

  • For each schema.graphqls files and their content
    • Split the text document into an array ($partialSchemaTypes) with keys like ComplexTextValue and value like type ComplexTextValue { html: String! @doc(description: "HTML format")}
    • Combine this list with all previously identified types ($knownTypes)
    • Use webonyx/graphql-php code to build full array of types and schema into an AST, merge these results with all previously built AST ($results)

As we progress further through the list of available schema.graphqls the total number of identified types ($knownTypes) grows which are then fed into the AST generation repeatedly. This is so that we can declare a new schema file and if it refer to something like Money the AST builder needs to have those definitions available or an exception is thrown.

There is a performance problem with GraphQL when parsing large data documents, see webonyx/graphql-php#244 and the comment at webonyx/graphql-php#244 (comment)

I added some logging locally

private function readPartialTypes(string $graphQlSchemaContent) : array
{
    $logger = \Magento\Framework\App\ObjectManager::getInstance()->get(\Psr\Log\LoggerInterface::class);
    $logger->debug("SchemaStitching - readPartialTypes on schema string of length " . strlen($graphQlSchemaContent));

On a vanilla magento instance of 2.4-develop we can see the following results, that the total size of the schema parsed grows and grows and that we parse once per every graphql file. This means that for every new graphql file added we have to parse the entire schema again.

main.DEBUG: SchemaStitching - readPartialTypes on schema string of length 4954
main.DEBUG: SchemaStitching - readPartialTypes on schema string of length 8079
main.DEBUG: SchemaStitching - readPartialTypes on schema string of length 9345
main.DEBUG: SchemaStitching - readPartialTypes on schema string of length 9855
main.DEBUG: SchemaStitching - readPartialTypes on schema string of length 10554
main.DEBUG: SchemaStitching - readPartialTypes on schema string of length 10601
main.DEBUG: SchemaStitching - readPartialTypes on schema string of length 57326
main.DEBUG: SchemaStitching - readPartialTypes on schema string of length 71006
main.DEBUG: SchemaStitching - readPartialTypes on schema string of length 66396
main.DEBUG: SchemaStitching - readPartialTypes on schema string of length 85173
main.DEBUG: SchemaStitching - readPartialTypes on schema string of length 86315
main.DEBUG: SchemaStitching - readPartialTypes on schema string of length 89836
main.DEBUG: SchemaStitching - readPartialTypes on schema string of length 89793
main.DEBUG: SchemaStitching - readPartialTypes on schema string of length 97946
main.DEBUG: SchemaStitching - readPartialTypes on schema string of length 96998
main.DEBUG: SchemaStitching - readPartialTypes on schema string of length 94136
main.DEBUG: SchemaStitching - readPartialTypes on schema string of length 95130
main.DEBUG: SchemaStitching - readPartialTypes on schema string of length 92333
main.DEBUG: SchemaStitching - readPartialTypes on schema string of length 92482
main.DEBUG: SchemaStitching - readPartialTypes on schema string of length 105498
main.DEBUG: SchemaStitching - readPartialTypes on schema string of length 112194
main.DEBUG: SchemaStitching - readPartialTypes on schema string of length 112296
main.DEBUG: SchemaStitching - readPartialTypes on schema string of length 115529
main.DEBUG: SchemaStitching - readPartialTypes on schema string of length 131708
main.DEBUG: SchemaStitching - readPartialTypes on schema string of length 132418
main.DEBUG: SchemaStitching - readPartialTypes on schema string of length 134087
main.DEBUG: SchemaStitching - readPartialTypes on schema string of length 126070
main.DEBUG: SchemaStitching - readPartialTypes on schema string of length 126077
main.DEBUG: SchemaStitching - readPartialTypes on schema string of length 128494
main.DEBUG: SchemaStitching - readPartialTypes on schema string of length 129368
main.DEBUG: SchemaStitching - readPartialTypes on schema string of length 129775
main.DEBUG: SchemaStitching - readPartialTypes on schema string of length 129766
main.DEBUG: SchemaStitching - readPartialTypes on schema string of length 129624
main.DEBUG: SchemaStitching - readPartialTypes on schema string of length 130909
main.DEBUG: SchemaStitching - readPartialTypes on schema string of length 137686

The solution

We cannot realistically fix webonyx/graphql-php to fix this issue with large documents, it seems a bit of a systemic issue with graphql. What we can do is try to reduce the amount of documents Magento needs to produce, and to reduce their size.

By improving the performance of this task we can remove the CPU bottleneck which causes downtime.

In this pull request

  • I change the algorithm to try to minimise the total number of calls to readPartialTypes by batching the schemas from many different schema.graphqls into one parse.
  • I try to only process batches with their minimum required schema to reduce the document sizes. This means that adding a new schema.graphqls scales much better and only brings in the required types for that given schema rather than the all schemas.

My solution here feels a bit weird, doing some static analysis over the available schema before building the AST, but the results are good.

A blackfire comparison shows a vast improvement
Screenshot 2021-01-28 at 12 28 48

Looking at the same log we have called readPartialTypes 17 times rather than 34 and the string size of the documents parsed are significantly smaller.

main.DEBUG: SchemaStitching - readPartialTypes on schema string of length 157047
main.DEBUG: SchemaStitching - readPartialTypes on schema string of length 75969
main.DEBUG: SchemaStitching - readPartialTypes on schema string of length 35876
main.DEBUG: SchemaStitching - readPartialTypes on schema string of length 59638
main.DEBUG: SchemaStitching - readPartialTypes on schema string of length 33325
main.DEBUG: SchemaStitching - readPartialTypes on schema string of length 27093
main.DEBUG: SchemaStitching - readPartialTypes on schema string of length 16616
main.DEBUG: SchemaStitching - readPartialTypes on schema string of length 23712
main.DEBUG: SchemaStitching - readPartialTypes on schema string of length 27387
main.DEBUG: SchemaStitching - readPartialTypes on schema string of length 28387
main.DEBUG: SchemaStitching - readPartialTypes on schema string of length 10122
main.DEBUG: SchemaStitching - readPartialTypes on schema string of length 3877
main.DEBUG: SchemaStitching - readPartialTypes on schema string of length 49237
main.DEBUG: SchemaStitching - readPartialTypes on schema string of length 19429
main.DEBUG: SchemaStitching - readPartialTypes on schema string of length 29782
main.DEBUG: SchemaStitching - readPartialTypes on schema string of length 16344
main.DEBUG: SchemaStitching - readPartialTypes on schema string of length 16842

Automated testing

⚠️ There is still some work to be done here. ⚠️

I added a test which stitches together all the graphql schema from the system just to verify the process ran complete from end to end without any mocking.

However to make this change I needed to remove some of the work done in #28747. In that pull request meta data is added to the parsed schema per schema.graphqls, however we no longer process the data file by file because of the performance issues involved.

The test I added covers that the entire schema does generate in order, but it will be breaking the tests added in that PR.

How do we want to proceed with this? Having a test to verify dependencies is a good idea, but for the sake of performance we cannot process each graphql document file by file, it does not work at an enterprise level site with many graphql requests being fired.

Manual testing

Manual testing method

I have a few PHP scripts to investigate the graphql schema in the cache and report on its contents after triggering a graphql request.

You start by running test.sh with these scripts in the magento root.

# test.sh
#!/bin/bash
set -e
php bin/magento | head -1 # ensure main magento configuration is cached
php cache-graphql-remove.php
php cache-graphql-report.php
time curl "https://magento-contribution-github.ampdev.co/graphql/?query=%7Bproducts(filter%3A%7Bsku%3A%7Beq%3A%22nonsuchsku%22%7D%7D)%7Bitems%7Bname%20sku%7D%7D%7D"
php cache-graphql-report.php
# cache-graphql-report.php 
<?php
use Magento\Framework\App\Bootstrap;

require __DIR__ . '/app/bootstrap.php';

$bootstrap = Bootstrap::create(BP, $_SERVER);
$obj = $bootstrap->getObjectManager();
/** @var \Magento\Framework\Config\CacheInterface $cache */
$cache = $obj->get(Magento\Framework\Config\CacheInterface::class);

$cacheId ='Magento_Framework_GraphQlSchemaStitching_Config_Data';
echo "Reporting cache:  $cacheId" . PHP_EOL;
$result  = $cache->load($cacheId);

if (!strlen($result)) {
    echo "No entry in cache" . PHP_EOL;
    return;
}

$result = \json_decode($result, true);
recur_ksort($result);

echo "String length " . strlen(json_encode($result)) .  PHP_EOL;
echo "MD5 (sorted by key) " . md5(json_encode($result)) .  PHP_EOL;

function recur_ksort(&$array) {
    foreach ($array as &$value) {
        if (is_array($value)) recur_ksort($value);
    }
    if (isset($array['module'])) {
        unset($array['module']); // strip out meta data used in dependency tests which is removed as part of PR
    }
    return ksort($array);
}
# cache-graphql-remove.php
<?php
use Magento\Framework\App\Bootstrap;

require __DIR__ . '/app/bootstrap.php';

$bootstrap = Bootstrap::create(BP, $_SERVER);
$obj = $bootstrap->getObjectManager();
/** @var \Magento\Framework\Config\CacheInterface $cache */
$cache = $obj->get(Magento\Framework\Config\CacheInterface::class);

$cacheId ='Magento_Framework_GraphQlSchemaStitching_Config_Data';
echo "Removing cache:   $cacheId" . PHP_EOL;
$result  = $cache->remove($cacheId);
echo "done" . PHP_EOL;

Manual testing results

I have to sort the data as my method changes the ordering of some of the fields, I also have to strip out the metadata that I discussed above in automated testing notes.

We can see that before and after this change we produce a string of the same length, with the same md5 hash.

This gives me confidence that we're producing the same graphql schema for the magento system at a fraction of the runtime.

Before change

Magento CLI dev-improve-graphql-stitching-perf
Removing cache:   Magento_Framework_GraphQlSchemaStitching_Config_Data
done
Reporting cache:  Magento_Framework_GraphQlSchemaStitching_Config_Data
No entry in cache
{"data":{"products":{"items":[]}}}
real    0m33.099s
user    0m0.019s
sys     0m0.009s
Reporting cache:  Magento_Framework_GraphQlSchemaStitching_Config_Data
String length 445187
MD5 (sorted by key) 46ccebeb2579886592acaffea2320398

After change

Magento CLI dev-improve-graphql-stitching-perf
Removing cache:   Magento_Framework_GraphQlSchemaStitching_Config_Data
done
Reporting cache:  Magento_Framework_GraphQlSchemaStitching_Config_Data
No entry in cache
{"data":{"products":{"items":[]}}}
real    0m7.443s
user    0m0.013s
sys     0m0.007s
Reporting cache:  Magento_Framework_GraphQlSchemaStitching_Config_Data
String length 445187
MD5 (sorted by key) 46ccebeb2579886592acaffea2320398

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds are green)

@m2-assistant
Copy link

m2-assistant bot commented Jan 28, 2021

Hi @convenient. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento give me test instance - deploy test instance based on PR changes
  • @magento give me 2.4-develop instance - deploy vanilla Magento instance

❗ Automated tests can be triggered manually with an appropriate comment:

  • @magento run all tests - run or re-run all required tests against the PR changes
  • @magento run <test-build(s)> - run or re-run specific test build(s)
    For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names. Allowed build names are:

  1. Database Compare
  2. Functional Tests CE
  3. Functional Tests EE,
  4. Functional Tests B2B
  5. Integration Tests
  6. Magento Health Index
  7. Sample Data Tests CE
  8. Sample Data Tests EE
  9. Sample Data Tests B2B
  10. Static Tests
  11. Unit Tests
  12. WebAPI Tests
  13. Semantic Version Checker

You can find more information about the builds here

ℹ️ Please run only needed test builds instead of all when developing. Please run all test builds before sending your PR for review.

For more details, please, review the Magento Contributor Guide documentation.

⚠️ According to the Magento Contribution requirements, all Pull Requests must go through the Community Contributions Triage process. Community Contributions Triage is a public meeting.

🕙 You can find the schedule on the Magento Community Calendar page.

📞 The triage of Pull Requests happens in the queue order. If you want to speed up the delivery of your contribution, please join the Community Contributions Triage session to discuss the appropriate ticket.

🎥 You can find the recording of the previous Community Contributions Triage on the Magento Youtube Channel

✏️ Feel free to post questions/proposals/feedback related to the Community Contributions Triage process to the corresponding Slack Channel

@convenient convenient force-pushed the improve-graphql-stitching-perf branch from fb10610 to 0672892 Compare January 28, 2021 14:12
@convenient
Copy link
Contributor Author

@magento run all tests

@gabrieldagama gabrieldagama added the Priority: P1 Once P0 defects have been fixed, a defect having this priority is the next candidate for fixing. label Jan 28, 2021
@convenient
Copy link
Contributor Author

When I click on any of the build reports they just spin for me never loading.

Either way I intend to cover all the tests in one swoop after the discussion on what we do with the meta data that can no longer be asserted against.

@convenient convenient changed the title Fix performance when parsing GraphQL schema into AST which causes downtime on cache stampede [wip] [wip] Fix performance when parsing GraphQL schema into AST which causes downtime on cache stampede Jan 28, 2021
@convenient
Copy link
Contributor Author

And to aid with the stampeding during deployments I am testing out an overwrite to the core like the following, it's a bit more specific to our situation but just in case other people are reading this in the same scenario

--- setup/src/Magento/Setup/Model/Installer.php	2020-06-23 12:03:22.000000000 +0100
+++ setup/src/Magento/Setup/Model/Installer.php	2021-01-29 16:52:16.000000000 +0000
@@ -1279,6 +1279,10 @@
         $types = empty($types) ? $availableTypes : array_intersect($availableTypes, $types);
         $enabledTypes = $cacheManager->setEnabled($types, $isEnabled);
         if ($isEnabled) {
+            // Do not flush varnish during a deployment
+            $enabledTypes = array_filter($enabledTypes, function ($type) {
+                return $type !== 'full_page';
+            });
             $cacheManager->clean($enabledTypes);
         }
 
@@ -1308,6 +1312,12 @@
         /** @var \Magento\Framework\App\Cache\Manager $cacheManager */
         $cacheManager = $this->objectManagerProvider->get()->get(\Magento\Framework\App\Cache\Manager::class);
         $types = $cacheManager->getAvailableTypes();
+
+        // Do not flush varnish during a deployment
+        $types = array_filter($types, function ($type) {
+            return $type !== 'full_page';
+        });
+
         $cacheManager->clean($types);
         $this->log->log('Cache cleared successfully');
     }

This means that we don't automatically purge varnish during deplyoment, but we have a short enough TTL that his is forgiving and it is better for some stale pages to be served than a 503.

@steven-hoffman-jomashop
Copy link

@convenient,

Another possible solution is to move the SchemaStitching to the deployment phase.
This would include the GraphQlReader's results only.
This would obviously not generate the final schema as that relies on product attributes and so on.
It would cover the expensive part, and can be done during di/code generation.

After such change, the cache flush would not trigger the expensive logic involved in parsing the massive ASTs.
I see no major downside to this change, as the schema files are not expected to change in production mode.
While development mode would be effectively unchanged.
(I would agree that this slows down development work at times also; so the above fix might still be quite valuable).

@convenient
Copy link
Contributor Author

Just a note that this has been deployed to production without issue, and no spike in CPU or anything to remark during the deployment. Went through without downtime because of this patch.

@convenient
Copy link
Contributor Author

Is there anything you need from me on this?

@danslo
Copy link
Contributor

danslo commented May 10, 2021

FWIW: We're seeing this too;

Degraded GQL performance during high load on deployments. New Relic traces point exactly to what @convenient has been describing above.

We've applied patch and no issues during UAT so far.

@cpartica
Copy link
Contributor

@magento run all tests

@magento-automated-testing
Copy link

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

There was a bug with the enum placeholder ordering

Input like the following
```
            interface SomeInterface  {
                product_type: String @doc(description: "The type of product, such as simple, configurable, etc.")
            }
            enum SomeEnum {
            }
```

would have placeholder logic ran against it and end up like
```
            interface SomeInterface  {
                product_type: String @doc(description: "The type of product, such as simple, configurable, etc.")
            }
            enum SomeEnum {
placeholder_graphql_field: String
}
```

This caused an error, something in the AST was trying to make it resolve against the word "Type" in the @doc description

By fixing the enum so that it does not have `: String` in it the AST is generated successfully, we do this by replacing those empty enums first so that they cannot get caught by the empty type check
The newly supported union is the cause of this issue in the parseTypes function.

The regex is finding the type of union CompanyStructureEntity but its also grabbing the item below it and including it as part of its schema content, which means CompanyStructureEntity contains the schema for itself as well as CompanyStructureItem , this means I never have a $knownTypes['CompanyStructureItem']  to work with which means it is a missing dependency.

```
<?php
use Magento\Framework\App\Bootstrap;
require __DIR__ . '/app/bootstrap.php';
​
$bootstrap = Bootstrap::create(BP, $_SERVER);
$obj = $bootstrap->getObjectManager();
/** @var Magento\Framework\GraphQlSchemaStitching\GraphQlReader $graphqlReader */
$graphqlReader = $obj->get(Magento\Framework\GraphQlSchemaStitching\GraphQlReader::class);
$reflector = new ReflectionObject($graphqlReader);
$method = $reflector->getMethod('parseTypes');
$method->setAccessible(true);
​
$broken =
'type IsCompanyEmailAvailableOutput @doc(description: "Contains the response of a company email validation query") {
    is_email_available: Boolean! @doc(description: "A value of `true` indicates the email address can be used to create a company")
}
​
union CompanyStructureEntity @typeResolver(class: "Magento\\CompanyGraphQl\\Model\\Resolver\\StructureEntityTypeResolver") = CompanyTeam | Customer
​
type CompanyStructureItem @doc(description: "Defines an individual node in the company structure") {
    id: ID! @doc(description: "The unique ID for a `CompanyStructureItem` object")
    parent_id: ID @doc(description: "The ID of the parent item in the company hierarchy")
    entity: CompanyStructureEntity @doc(description: "A union of `CompanyTeam` and `Customer` objects")
}
​
type CompanyStructure @doc(description: "Contains an array of the individual nodes that comprise the company structure") {
    items: [CompanyStructureItem] @doc(description: "An array of elements in a company structure")
}';
​
$result = $method->invoke($graphqlReader, $broken);
​
echo "Got the following types" . PHP_EOL;
echo implode(PHP_EOL, array_keys($result)) . PHP_EOL;
​
echo "The values for the union actually contains CompanyStructureItem which is why the initial approach works any mine does not" . PHP_EOL;
echo $result['CompanyStructureEntity'] . PHP_EOL;
```
@convenient
Copy link
Contributor Author

@magento run all tests

@magento-automated-testing
Copy link

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

@convenient
Copy link
Contributor Author

convenient commented May 13, 2021

Hey @cpartica

As discussed on slack there were 2 issues I encountered, both these errors completely broke all graphql requests.

Syntax Error: Expected Name found :

The first one was a bug in the placeholder generation logic which was turning schema like

interface SomeInterface  {
    product_type: String @doc(description: "The type of product, such as simple, configurable, etc.")
}
enum SomeEnum {
}

into

interface SomeInterface  {
    product_type: String @doc(description: "The type of product, such as simple, configurable, etc.")
}
enum SomeEnum {
    placeholder_graphql_field: String
}     

This is wrong because enum should not have the : String at the end.

Ideally I would like to update the regex to properly fix this issue, however I have worked around it in 15bad09 by doing the enum regex first instead of the type regex. The issue was that the type regex was somehow picking up this kind of schema first and making it invalid.

I believe this fix is safe and makes sense, but fixing the original regex may make more sense if time allows. I am not sure it's in my skillset to fix this regex.

union types breaking parsetypes

With 2.4.2 and B2B we were seeing the following error

1 exception(s):
Exception #0 (GraphQL\Error\Error): Type "CompanyStructureItem" not found in document.

After debugging I found the issue in the parseTypes function, it does not handle union types properly.

With input like

union CompanyStructureEntity @typeResolver(class: "Magento\\CompanyGraphQl\\Model\\Resolver\\StructureEntityTypeResolver") = CompanyTeam | Customer

type CompanyStructureItem @doc(description: "Defines an individual node in the company structure") {
  id: ID! @doc(description: "The unique ID for a `CompanyStructureItem` object")
  parent_id: ID @doc(description: "The ID of the parent item in the company hierarchy")
  entity: CompanyStructureEntity @doc(description: "A union of `CompanyTeam` and `Customer` objects")
}

type CompanyStructure @doc(description: "Contains an array of the individual nodes that comprise the company structure") {
  items: [CompanyStructureItem] @doc(description: "An array of elements in a company structure")
}'

We were getting an array like

$knownTypes['CompanyStructureEntity'] = 'union CompanyStructureEntity @typeResolver(class: "Magento\\CompanyGraphQl\\Model\\Resolver\\StructureEntityTypeResolver") = CompanyTeam | Customer

type CompanyStructureItem @doc(description: "Defines an individual node in the company structure") {
  id: ID! @doc(description: "The unique ID for a `CompanyStructureItem` object")
  parent_id: ID @doc(description: "The ID of the parent item in the company hierarchy")
  entity: CompanyStructureEntity @doc(description: "A union of `CompanyTeam` and `Customer` objects")
}';
$knownTypes['CompanyStructure'] = 'type CompanyStructure @doc(description: "Contains an array of the individual nodes that comprise the company structure") {
  items: [CompanyStructureItem] @doc(description: "An array of elements in a company structure")
}';

The bug being that there is no $knownTypes['CompanyStructureItem'] generated, its contained within the CompanyStructureEntity. This is a bug that exists in the current implementation but only becomes an issue in this PR where we attempt to regenerate the schema with only the minimal amount of values from $knownTypes, as there is a missing key we cannot find the type to bundle so we get this exception.

I have added a super hacky string hack (cfcbcd8) just so we can run it against the magento test suites, but I would appreciate any help from someone who is a bit better at regex than myself. Because i think we should fix parseTypes so that it does not miss types immediately after a union. Again, I would appreciate help with this regex.

Next steps

Tomorrow I will run this through blackfire, just to confirm it still performs well (don't imagine it won't but just in case).

I will also diff the final schema before my change and after my change, to ensure I have not adjusted the final output.

…hql-stitching-perf

 Conflicts:
	lib/internal/Magento/Framework/GraphQlSchemaStitching/GraphQlReader.php
@convenient
Copy link
Contributor Author

@magento run all tests

@magento-automated-testing
Copy link

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

@convenient
Copy link
Contributor Author

@magento run Functional Tests EE, Unit Tests

@magento-automated-testing
Copy link

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

@convenient
Copy link
Contributor Author

Just so its recorded @danslo asked for something to repro the union issue.

This uses magento

<?php
use Magento\Framework\App\Bootstrap;
require __DIR__ . '/app/bootstrap.php';

$bootstrap = Bootstrap::create(BP, $_SERVER);
$obj = $bootstrap->getObjectManager();

/** @var Magento\Framework\GraphQlSchemaStitching\GraphQlReader */
$reader = $obj->get(Magento\Framework\GraphQlSchemaStitching\GraphQlReader::class);

$schema = 'type IsCompanyEmailAvailableOutput @doc(description: "Contains the response of a company email validation query") {
    is_email_available: Boolean! @doc(description: "A value of `true` indicates the email address can be used to create a company")
}

union CompanyStructureEntity @typeResolver(class: "Magento\\CompanyGraphQl\\Model\\Resolver\\StructureEntityTypeResolver") = CompanyTeam | Customer

type CompanyStructureItem @doc(description: "Defines an individual node in the company structure") {
    id: ID! @doc(description: "The unique ID for a `CompanyStructureItem` object")
    parent_id: ID @doc(description: "The ID of the parent item in the company hierarchy")
    entity: CompanyStructureEntity @doc(description: "A union of `CompanyTeam` and `Customer` objects")
}

type CompanyStructure @doc(description: "Contains an array of the individual nodes that comprise the company structure") {
    items: [CompanyStructureItem] @doc(description: "An array of elements in a company structure")
}';

$method = new ReflectionMethod(\Magento\Framework\GraphQlSchemaStitching\GraphQlReader::class, "parseTypes");
$method->setAccessible(true);
$results = $method->invoke($reader, $schema);

if (!array_key_exists('CompanyStructureItem', $results)) {
    echo "FAILURE - Missing CompanyStructureItem" . PHP_EOL;
} else {
    echo "SUCCESS" . PHP_EOL;
}

echo implode(PHP_EOL, array_keys($results)) . PHP_EOL;

This uses just PHP

<?php
$schema = 'type IsCompanyEmailAvailableOutput @doc(description: "Contains the response of a company email validation query") {
    is_email_available: Boolean! @doc(description: "A value of `true` indicates the email address can be used to create a company")
}

union CompanyStructureEntity @typeResolver(class: "Magento\\CompanyGraphQl\\Model\\Resolver\\StructureEntityTypeResolver") = CompanyTeam | Customer

type CompanyStructureItem @doc(description: "Defines an individual node in the company structure") {
    id: ID! @doc(description: "The unique ID for a `CompanyStructureItem` object")
    parent_id: ID @doc(description: "The ID of the parent item in the company hierarchy")
    entity: CompanyStructureEntity @doc(description: "A union of `CompanyTeam` and `Customer` objects")
}

type CompanyStructure @doc(description: "Contains an array of the individual nodes that comprise the company structure") {
    items: [CompanyStructureItem] @doc(description: "An array of elements in a company structure")
}';

$regex = '/(type|interface|union|enum|input)[\s\t\n\r]+([_A-Za-z][_0-9A-Za-z]+)[\s\t\n\r]+([^\{]*)(\{[^\}]*\})/i';
preg_match_all(
    "$regex",
    $schema,
    $matches
);

if (count($matches[0]) === 4) {
    echo "Pass" . PHP_EOL;
}  else {
    echo "Failure, didnt split out CompanyStructureItem from CompanyStructureEntity" . PHP_EOL;
}

If I can get the union hack in this completed it should reduce the cyclomatic complexity, allowing us to pass the static tests 🤞

This workaround handles a bug where parseTypes also contains the data from below it

```
union X = Y | Z

type foo {}
```

Would return
```php
[
    'x' => 'union X = Y | Z

    type foo {}'
]
```

instead of
```php
[
    'x' => 'union X = Y | Z',
    'foo' => 'type foo {}'
]
```

This workaround deals only with union types, so doesnt materially affect anything in most cases.
@convenient
Copy link
Contributor Author

@magento run all tests

@magento-automated-testing
Copy link

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

@convenient
Copy link
Contributor Author

@magento run all tests

@magento-automated-testing
Copy link

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

@convenient convenient changed the title [wip] Fix performance when parsing GraphQL schema into AST which causes downtime on cache stampede Fix performance when parsing GraphQL schema into AST which causes downtime on cache stampede Oct 9, 2021
@convenient
Copy link
Contributor Author

This is the best attempt I can do at resolving the issue with parseTypes and union schema c36fb66

It should be generic enough that it works with any union type I think. It's not great engineering, but downtime is worse so I'm going to leave it as is until someone from Magento wishes to discuss it further.

@convenient
Copy link
Contributor Author

convenient commented Dec 2, 2021

Can anyone from Magento chime in on whats happening with this?

It's 10 months that it's been open and it's a pretty serious issue. I know a bunch of users in the Magento slack have applied this as a patch and have had it fix their PWA sites.

This patch (or an alternative solution) is necessary for sites to perform properly as graphql schema / customisations are added.

I can see there's changes in 2.4-develop for the same class I'm targeting here Magento\Framework\GraphQlSchemaStitching\GraphQlReader. I don't really have the energy to keep my fork/branch up to date if this keeps drifting and issues arise out of it.

Like #34651 adds a new type which would mean rework for this patch. It's hard to keep fixing a moving target, and I do need to keep fixing it as our clients cant have the downtime.

@vzabaznov
Copy link
Contributor

Hi @convenient, you PR was merged into internal repo and will change status soon, thank you so much for your contribution

@convenient
Copy link
Contributor Author

@vzabaznov thank you!

@convenient convenient deleted the improve-graphql-stitching-perf branch January 14, 2022 09:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: GraphQlSchemaStitching Partner: Ampersand partners-contribution Pull Request is created by Magento Partner Priority: P1 Once P0 defects have been fixed, a defect having this priority is the next candidate for fixing. Progress: review Project: GraphQL Release Line: 2.4
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants