feat: support `graphql` v16 #998

dchambers · 2022-05-03T20:20:48Z

Which problem is this PR solving?

Allow developers to use any version of the graphql library they want to by making graphql a peer dependency.
Allow developers to use graphql v16 by introducing an adaptor function to abstract away the API change in v16.

Short description of the changes

As per the problem solving statement, but defined as three distinct commits:
1. Prove that graphql can be made a peer dependency. 🟢
2. Start testing against graphql v16. 🔴
  - Green on CI because CI doesn't run test-all-versions, but verified as failing locally on my machine.
3. Introduce the graphql adaptor function to fix the broken tests. 🟢
  - Again, verified test-all-versions on my machine, but this time where all versions pass again.

Checklist

Ran npm run test-all-versions for the edited package(s) on the latest commit if applicable.

codecov · 2022-05-03T20:35:18Z

Codecov Report

Merging #998 (e148301) into main (502caae) will decrease coverage by 0.68%.
The diff coverage is 66.66%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #998      +/-   ##
==========================================
- Coverage   96.08%   95.39%   -0.69%     
==========================================
  Files          14       21       +7     
  Lines         893     1346     +453     
  Branches      191      278      +87     
==========================================
+ Hits          858     1284     +426     
- Misses         35       62      +27

Impacted Files	Coverage Δ
...opentelemetry-instrumentation-graphql/src/types.ts	`100.00% <ø> (ø)`
...opentelemetry-instrumentation-graphql/src/utils.ts	`93.40% <50.00%> (ø)`
...try-instrumentation-graphql/src/instrumentation.ts	`91.66% <100.00%> (ø)`
...tapackages/auto-instrumentations-node/src/utils.ts	`98.18% <0.00%> (ø)`
...nstrumentation-graphql/src/enums/AttributeNames.ts	`100.00% <0.00%> (ø)`
...entelemetry-instrumentation-graphql/src/symbols.ts	`100.00% <0.00%> (ø)`
.../opentelemetry-instrumentation-graphql/src/enum.ts	`100.00% <0.00%> (ø)`

dchambers · 2022-05-03T21:40:47Z

This PR fixes #784.

rauno56 · 2022-05-08T20:06:13Z

Thanks for the PR. I appreciate the clear PR description and commit structure. 👍

Allow developers to use any version of the graphql library they want to by making graphql a peer dependency.

Instrumentation users can actually already use any version. Moving it to peer dependency only makes it installed by default on new npm versions.

Green on CI because CI doesn't run test-all-versions, but verified as failing locally on my machine.

Do I understand you correctly that tests fail on graphql v16? How does that qualify supporting it in that case or is it still WIP?

dchambers · 2022-05-09T08:42:22Z

Instrumentation users can actually already use any version. Moving it to peer dependency only makes it installed by default on new npm versions.

Hi @rauno56 👋. So prior to this PR users could only use versions other than graphql v15 by forcing it using something like resolutions (assuming they use Yarn), plus they had no way of knowing whether it would work (it didn't for v16 for example, and I haven't bothered to test v14 as it's now pretty old, and I don't need it).

The peer dependency approach, which includes a formal specification of which peer versions are supported, is what's used by the vast majority of graphql dependent NPM libraries that we currently use (50 out of 53 to be exact), with this being one of the three libraries we use that doesn't currently work that way.

Do I understand you correctly that tests fail on graphql v16? How does that qualify supporting it in that case or is it still WIP?

No, graphql v16 is made to work in commit number 3. I only point out that it doesn't initially work (in commit number 2) to demonstrate the need for the changes in that third commit. This isn't reflected in the CI results however since test-all-versions is only run nightly, rather than being run on every single PR, and therefore I confirm that I've run it locally to make up for, what is otherwise, a lack of evidence for the utility of the PR.

rauno56

Sorry, I missed the fact that we had graphql listed under the dependencies not only as a devDep. It's not common among the packages here.

plugins/node/opentelemetry-instrumentation-graphql/.tav.yml

rauno56 · 2022-05-09T10:42:56Z

plugins/node/opentelemetry-instrumentation-graphql/package.json

  },
  "devDependencies": {
    "@opentelemetry/api": "1.0.2",
    "@opentelemetry/sdk-trace-base": "1.2.0",
    "@types/mocha": "8.2.3",
    "@types/node": "16.11.21",
+    "graphql": "^15.5.1",


Why not update it among the dev-deps as well to the latest and leave testing on v15 for TAV?

Thanks for the great feedback @rauno56. Those changes are now made.

@rauno56, I finally had some time to get the tests working while using v16 internally. However, since graphql v16 doesn't appear to support Node.js v8 or Node.js v10, that has meant conditionally disabling a few of the tests. Perhaps, therefore, you might prefer to use the latest 15.x internally, so that running npm test locally exercises all tests.

BTW the failing code coverage check appears to be as a result of an as Error cast I perform, which is weird since casting is only a notional concept, and does not translate to code paths that can be tested.

Flarna · 2022-05-09T11:04:04Z

graphql was move between dependency and devDependency a few times.

if it is a dependency - in special with a pinned version - it tends to collide with the user version
if it is a dep dependency it results in transpile problems if user has no graphql installed because it needs the types.

As it is part of the autoinstrument metapackage it's a common setup that users have the instrumentation installed even if they don't use graphql.

Refs: #754 and #850

Co-authored-by: Rauno Viskus <Rauno56@users.noreply.github.com>

dchambers · 2022-05-09T18:30:20Z

graphql was move between dependency and devDependency a few times.

if it is a dependency - in special with a pinned version - it tends to collide with the user version

if it is a dep dependency it results in transpile problems if user has no graphql installed because it needs the types.

As it is part of the autoinstrument metapackage it's a common setup that users have the instrumentation installed even if they don't use graphql.

Refs: #754 and #850

So by using peer dependencies I believe that this PR takes a third approach, which is different from both of the two previously utilised approaches. That is:

graphql as a devDependency:
- Consumers have the flexibility to use any version of graphql that's >= 15.5.1.
- Consumers are responsible for installing graphql if they don't already have it, and with no feedback from NPM/Yarn as to this requirement.
graphql as a dependency:
- Consumers have the flexibility to use any version of graphql that's >= 15.5.1.
- Consumers don't need to install graphql if they don't already have it.
graphql as a peerDependency and a devDependency:
- Consumers have the flexibility to use any version of graphql that's >= 14.0.0 || 15.0.0 || 16.0.0.
- Consumers are responsible for installing graphql if they don't already have it, but NPM/Yarn informs them about this requirement when they install @opentelemetry/instrumentation-graphql (assuming they don't already have graphql installed that is).

To be clear, I'm not suggesting that this is perfect, but it does seem (to me) like the best compromise.

@dyladan, as author of #850 do you have any thoughts on whether this is sufficient, or whether there may be a better solution?

dchambers · 2022-05-10T12:30:21Z

plugins/node/opentelemetry-instrumentation-graphql/src/instrumentation.ts

-        typeInfo?: graphqlTypes.TypeInfo,
-        options?: { maxErrors?: number }
+        options?: { maxErrors?: number },
+        typeInfo?: graphqlTypes.TypeInfo


graphql v16 flips around these two parameters 🤯 . This has no effect at runtime when using v15 Vs v16 because they both pass whatever they receive through, but needs to be changed in the case we want to compile against v16.

dchambers · 2022-05-10T12:32:21Z

plugins/node/opentelemetry-instrumentation-graphql/src/utils.ts

@@ -387,7 +387,7 @@ export function wrapFieldResolver<TSource = any, TContext = any, TArgs = any>(
            >
        >(
          () => {
-            return fieldResolver.call(this, source, args, contextValue, info);
+            return fieldResolver.call(this, source, args, contextValue, info) as any;


The any used to be baked into the generic return type, whereas it now uses unknown instead, which is insufficient here.

dchambers · 2022-05-10T12:33:23Z

plugins/node/opentelemetry-instrumentation-graphql/test/graphql.test.ts

@@ -976,7 +979,7 @@ describe('graphql', () => {
      assert.deepStrictEqual(spans.length, 1);
    });

-    it('should instrument parse with error', () => {
+    itMaybe('should instrument parse with error', () => {


This test doesn't work on Node.js v8 and Node.js v10 when using graphql v16, so make only run it on more recent versions of Node.js.

dchambers · 2022-05-17T08:31:45Z

HI @rauno56 / @dyladan 👋 . A week has passed by. Anyway to get this PR moving again?

As a recap it's ready to merge now with some quirks due to upgrading internally to use v16, or there's the option to use v15 internally if you prefer not to have the quirks?

dchambers · 2022-05-18T09:54:48Z

ℹ️ If you're using Yarn then you can actually just force it to use graphql v16 using the resolutions block since the code changes in this PR are only needed for the test code and for the TypeScript types, and are not required for the running of the plugin itself. Here's the config I used in my case:

"resolutions": {
  "@opentelemetry/instrumentation-graphql@npm:0.27.4/graphql": "^16.5.0",
},

github-actions · 2022-07-18T06:37:25Z

This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

vmarchaud · 2022-07-18T11:13:02Z

I'm not the right person to review this since i don't even know how graphql works but @open-telemetry/javascript-approvers if someone can that would be nice

vmarchaud · 2022-10-02T07:05:17Z

@rauno56 i saw you made modification, are you ok with the current state of the PR ? I can review it if you can more eyes so we can merge this

rauno56 · 2022-10-03T07:45:20Z

@dchambers , I removed the maybe-functionality in tests.

@vmarchaud, your review would be helpful for sure.

rauno56

Thanks for the contribution, @dchambers. Sorry, that it took so long to get merged.

@vmarchaud, nvm about my comment above. I think we can just merge it once CI becomes green.

fix: make graphql a peer dependency

16b5226

dchambers requested a review from a team May 3, 2022 20:20

github-actions bot assigned obecny May 3, 2022

github-actions bot requested a review from obecny May 3, 2022 20:21

ci: run tests against v16

f6f7ff1

dchambers force-pushed the support-graphql-v16 branch from 4a2f382 to ba537ee Compare May 3, 2022 21:20

feat: support graphql v16

feccd28

dchambers force-pushed the support-graphql-v16 branch from ba537ee to feccd28 Compare May 3, 2022 21:24

Merge branch 'main' into support-graphql-v16

c20d7f6

rauno56 reviewed May 9, 2022

View reviewed changes

Merge branch 'main' into support-graphql-v16

3b16575

Remove overlapping version ranges

dd00556

Co-authored-by: Rauno Viskus <Rauno56@users.noreply.github.com>

dchambers force-pushed the support-graphql-v16 branch from 19acc6e to 75eb836 Compare May 10, 2022 12:28

dchambers commented May 10, 2022

View reviewed changes

dchambers force-pushed the support-graphql-v16 branch from 75eb836 to cce6216 Compare May 10, 2022 12:42

feat: use latest version of graphql internally

92be5ed

dchambers force-pushed the support-graphql-v16 branch from cce6216 to 92be5ed Compare May 11, 2022 11:41

github-actions bot added the stale label Jul 18, 2022

vmarchaud added never-stale and removed stale labels Jul 18, 2022

rauno56 and others added 3 commits September 9, 2022 18:28

Merge branch 'main' into support-graphql-v16

6f88e67

Merge branch 'main' into support-graphql-v16

25037e5

fix: remove comma

73bdebb

rauno56 added the pkg:instrumentation-graphql label Sep 27, 2022

Merge branch 'main' into support-graphql-v16

69c10cf

test: remove exceptions for node@8 and node@10

639a77f

Merge branch 'main' into support-graphql-v16

e148301

rauno56 approved these changes Oct 3, 2022

View reviewed changes

rauno56 mentioned this pull request Oct 3, 2022

feat: add support for graphql 16 #1124

Closed

1 task

vmarchaud approved these changes Oct 3, 2022

View reviewed changes

vmarchaud merged commit 5da46ef into open-telemetry:main Oct 3, 2022

dyladan mentioned this pull request Oct 3, 2022

chore: release main #1214

Merged

rauno56 mentioned this pull request Oct 10, 2022

Add support for graphql@16 #784

Closed

jDzames mentioned this pull request Dec 6, 2022

docs(instrumentation-graphql): gql supported versions #1322

Closed

overbit mentioned this pull request Feb 28, 2024

docs(instrumentation-graphql): gql supported versions #1969

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support `graphql` v16 #998

feat: support `graphql` v16 #998

dchambers commented May 3, 2022 •

edited

Loading

codecov bot commented May 3, 2022 •

edited

Loading

dchambers commented May 3, 2022

rauno56 commented May 8, 2022

dchambers commented May 9, 2022

rauno56 left a comment

rauno56 May 9, 2022

dchambers May 9, 2022

dchambers May 11, 2022

Flarna commented May 9, 2022

dchambers commented May 9, 2022 •

edited

Loading

dchambers May 10, 2022

dchambers May 10, 2022

dchambers May 10, 2022

dchambers commented May 17, 2022

dchambers commented May 18, 2022

github-actions bot commented Jul 18, 2022

vmarchaud commented Jul 18, 2022

vmarchaud commented Oct 2, 2022

rauno56 commented Oct 3, 2022

rauno56 left a comment •

edited

Loading

feat: support graphql v16 #998

feat: support graphql v16 #998

Conversation

dchambers commented May 3, 2022 • edited Loading

Which problem is this PR solving?

Short description of the changes

Checklist

codecov bot commented May 3, 2022 • edited Loading

Codecov Report

dchambers commented May 3, 2022

rauno56 commented May 8, 2022

dchambers commented May 9, 2022

rauno56 left a comment

Choose a reason for hiding this comment

rauno56 May 9, 2022

Choose a reason for hiding this comment

dchambers May 9, 2022

Choose a reason for hiding this comment

dchambers May 11, 2022

Choose a reason for hiding this comment

Flarna commented May 9, 2022

dchambers commented May 9, 2022 • edited Loading

dchambers May 10, 2022

Choose a reason for hiding this comment

dchambers May 10, 2022

Choose a reason for hiding this comment

dchambers May 10, 2022

Choose a reason for hiding this comment

dchambers commented May 17, 2022

dchambers commented May 18, 2022

github-actions bot commented Jul 18, 2022

vmarchaud commented Jul 18, 2022

vmarchaud commented Oct 2, 2022

rauno56 commented Oct 3, 2022

rauno56 left a comment • edited Loading

Choose a reason for hiding this comment

feat: support `graphql` v16 #998

feat: support `graphql` v16 #998

dchambers commented May 3, 2022 •

edited

Loading

codecov bot commented May 3, 2022 •

edited

Loading

dchambers commented May 9, 2022 •

edited

Loading

rauno56 left a comment •

edited

Loading