fix: graphql v16 support

[jgillick]

Adds support for graph16.

From what I could tell, this broke because graphql dropped support for positional arguments, and the APM agent was forcing the arg object to be converted to positional args.

This PR simply passes the args straight through to graphql, just as they are passed to it.

Fixes: #2508

Checklist

• Implement code
• Add tests (existing tests cover this change)
• Update TypeScript typings (no new typings)
• Update documentation
• Add CHANGELOG.asciidoc entry
• Commit message follows commit guidelines

[apmmachine]

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

• Start Time: 2022-06-13T16:13:09.728+0000

• Duration: 40 min 12 sec

Test stats 🧪

Test	Results
Failed	0
Passed	256235
Skipped	0
Total	256235

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

• /test : Re-trigger the build.

• run module tests for <modules> : Run TAV tests for one or more modules, where <modules> can be either a comma separated list of modules (e.g. memcached,redis) or the string literal ALL to test all modules

• run benchmark tests : Run the benchmark test only.

• run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

[trentm]

@jgillick Thanks very much for tracking down the cause of the breakage and for starting this!

If I attempt to run the current tests with graphql@16.5.0 then they fail, because the test code is still attempting to use positional arguments:

% npm install graphql@16
...

% node test/instrumentation/modules/graphql.test.js
TAP version 13
# graphql.graphql
ok 1 no currentSpan in sync code after .graphql(...)
/Users/trentm/el/apm-agent-nodejs8/node_modules/graphql/type/schema.js:37
    throw new Error(
          ^

Error: Expected undefined to be a GraphQL schema.
    at assertSchema (/Users/trentm/el/apm-agent-nodejs8/node_modules/graphql/type/schema.js:37:11)
    at validateSchema (/Users/trentm/el/apm-agent-nodejs8/node_modules/graphql/type/validate.js:34:28)
    at graphqlImpl (/Users/trentm/el/apm-agent-nodejs8/node_modules/graphql/graphql.js:60:63)
    at /Users/trentm/el/apm-agent-nodejs8/node_modules/graphql/graphql.js:23:43
    at new Promise (<anonymous>)
    at Object.graphql (/Users/trentm/el/apm-agent-nodejs8/node_modules/graphql/graphql.js:23:10)
    at AsyncHooksRunContextManager.with (/Users/trentm/el/apm-agent-nodejs8/lib/instrumentation/run-context/BasicRunContextManager.js:59:17)
    at Instrumentation.withRunContext (/Users/trentm/el/apm-agent-nodejs8/lib/instrumentation/index.js:595:30)
    at Object.wrappedGraphql (/Users/trentm/el/apm-agent-nodejs8/lib/instrumentation/modules/graphql.js:73:21)
    at Test.<anonymous> (/Users/trentm/el/apm-agent-nodejs8/test/instrumentation/modules/graphql.test.js:31:11)
    at Test.bound [as _cb] (/Users/trentm/el/apm-agent-nodejs8/node_modules/tape/lib/test.js:99:32)
    at Test.run (/Users/trentm/el/apm-agent-nodejs8/node_modules/tape/lib/test.js:117:31)
    at Test.bound [as run] (/Users/trentm/el/apm-agent-nodejs8/node_modules/tape/lib/test.js:99:32)
    at Immediate.next (/Users/trentm/el/apm-agent-nodejs8/node_modules/tape/lib/results.js:88:19)
    at processImmediate (node:internal/timers:466:21)
    at process.callbackTrampoline (node:internal/async_hooks:130:17)

We use https://github.com/watson/test-all-versions to test the APM agent against all/most supported versions of the modules that we instrument. This PR should eventually update .tav.yml to include graphql@16.

However, before that, when reviewing your change, I noticed that the wrappedGraphql needs the same treatment -- the current wrappedGraphql won't break graphql, but the instrumentation will produce poor span data.

I have a proposed patch to add on top of your changes: https://gist.github.com/trentm/ee7d2d5f7c7a98917b362c405830c84b
It does a few things:

• slightly tweaks your wrapExecute fix (I've moved the arguments extracting to after the createSpan call, so that we don't bother if we get that early return.)
• does the equivalent for wrapGraphql
• fixes an unrelated issue with handling source in wrapGraphql (the args.source property can already be a graphql.Source object)
• updates the tests

Would you be willing to apply this patch to your PR branch? Then I can get the full test suite running on this PR. Thanks.

[jgillick]

Hi @trentm, thank you for the patch! It's been applied and pushed.

[trentm]

Will still need a .tav.yml update at least, but good to get tests going.

[trentm]

/test

[trentm]

@jgillick Thanks. This got further, but testing with graphql@16 and node@10 fails because graphql dropped support for node 10 and earlier.

running test: cd . && node --require /home/runner/work/apm-agent-nodejs/apm-agent-nodejs/test/_promise_rejection.js test/instrumentation/modules/express-graphql.test.js
TAP version 13
# POST /graphql
/home/runner/work/apm-agent-nodejs/apm-agent-nodejs/node_modules/graphql/utilities/extendSchema.js:605
    return nodes.flatMap(
                 ^
TypeError: nodes.flatMap is not a function
    at buildInterfaces (/home/runner/work/apm-agent-nodejs/apm-agent-nodejs/node_modules/graphql/utilities/extendSchema.js:605:18)
    at interfaces (/home/runner/work/apm-agent-nodejs/apm-agent-nodejs/node_modules/graphql/utilities/extendSchema.js:671:29)

Here is a second proposed patch: https://gist.github.com/trentm/1fdbdac639d7db58933374d0c269d78a

• It skips testing if the graphql and node vers are incompatible.
• Adds the appropriate changes to ".tav.yml" so we can test all supported graphql versions.
• Updates the support graphql ver in our docs.

Please add this to your branch and I'll get the tests running again.

[jgillick]

@trentm Thanks for your help with this PR. Your patch has been applied. I've not used tav before, but that setup is pretty cool.

[trentm]

@jgillick Thanks. Here is another proposed patch for you: https://gist.github.com/trentm/f544d984c512543765ce5723f5ac7603

This fixes a crash in a test case for the relatest express-graphql package. The tests for it require('graphql'), so the same compatibility guard is needed there.

[trentm]

@elasticmachine, run elasticsearch-ci/docs

[trentm]

run module tests for graphql,express-graphql

[trentm]

@jgillick Thanks very much!

[jgillick]

Thank you for all of your help @trentm!