Add support for GraphQL 2.0 #1982

marcotc · 2022-04-21T20:31:34Z

Fixes #1325.
Fixes #1866.

This PR adds support for GraphQL 2.x series.
Additional work is done in an upstream PR.

Also, this PR backfill a few parts of the upstream integration to support ddtrace 1.0 on versions of the graphql gem that do not have the upstream PR changes.

In sum, this PR:

Ensures graphql 1.x series works with ddtrace 1.0, by patching the upstream integration.
- A new test combination (contrib-old) was added to ensure we are testing the 1.x graphql series.
Ensures graphql 2.x series works with ddtrace 1.0, by patching the upstream integration.
Ensures upstream PR changes do not conflict with our patching of the upstream integration.

I'm assuming in this PR that the next version of graphql (currently 2.0.6) will contain our upstream changes. If not, I'll update our code to address that, but I believe the current implementation is ready to go.

marcotc · 2022-04-21T20:54:10Z

lib/datadog/tracing/contrib/graphql/ext.rb

@@ -7,11 +7,11 @@ module GraphQL
        # GraphQL integration constants
        # @public_api Changing resource names, tag names, or environment variables creates breaking changes.
        module Ext


These Ext changes were done for all integrations, except GraphQL in #1800. Because GraphQL changes involved upstream changes, we held off until now to make them.

‼️ One issue I spot is that we seem to have a bit of a dependency loop, as you mention that

I'm assuming in this PR that the next version of graphql (currently 2.0.6) will contain our upstream changes. If not, I'll update our code to address that, but I believe the current implementation is ready to go.

The next version has been released as 2.0.7: https://github.com/rmosolgo/graphql-ruby/blob/master/CHANGELOG.md .

Unfortunately, the code added in rmosolgo/graphql-ruby#4038 is

if defined?(Datadog::Tracing::Metadata::Ext) # Introduced in ddtrace 1.0 span.set_tag(Datadog::Tracing::Metadata::Ext::TAG_COMPONENT, 'graphql') span.set_tag(Datadog::Tracing::Metadata::Ext::TAG_OPERATION, key) end

and if I'm looking at this correctly, this code DEPENDS on this PR. Aka I think 2.0.7 is broken for customers on 1.0.0.beta2, because beta2 has Datadog::Tracing::Metadata::Ext but no TAG_COMPONENT.

My suggestion would be to possibly submit a new PR upstream to change defined?(Datadog::Tracing::Metadata::Ext) into defined?(Datadog::Tracing::Metadata::Ext::TAG_COMPONENT), which would make graphql fallback to the previous behavior on beta1/2.

Hmmm, that makes sense.
Nothing we can do in this PR, unfortunately.
I'll take note of this so we either quickly release a new version of ddtrace or update upstream.

codecov-commenter · 2022-04-21T21:13:04Z

Codecov Report

Merging #1982 (0ce85b9) into master (663aaa4) will increase coverage by 0.02%.
The diff coverage is 92.00%.

@@            Coverage Diff             @@
##           master    #1982      +/-   ##
==========================================
+ Coverage   97.71%   97.74%   +0.02%     
==========================================
  Files        1004     1004              
  Lines       50628    50651      +23     
==========================================
+ Hits        49472    49508      +36     
+ Misses       1156     1143      -13

Impacted Files	Coverage Δ
...pec/datadog/tracing/contrib/graphql/tracer_spec.rb	`96.36% <83.33%> (+19.61%)`	⬆️
lib/datadog/tracing/contrib/graphql/ext.rb	`100.00% <100.00%> (ø)`
lib/datadog/tracing/contrib/graphql/patcher.rb	`100.00% <100.00%> (ø)`
lib/datadog/core/diagnostics/environment_logger.rb	`100.00% <0.00%> (+1.58%)`	⬆️
spec/datadog/tracing/contrib/graphql/test_types.rb	`100.00% <0.00%> (+5.17%)`	⬆️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

ivoanjo

I spotted a potential issue, but the issue is on the upstream side, not on this PR.

Otherwise it looks good, thanks for fixing this!

ivoanjo · 2022-04-26T08:27:29Z

Appraisals

  appraise 'contrib-old' do
    gem 'dalli', '< 3.0.0'
+    gem 'graphql', '>= 1.12.0', '< 2.0'
  end


Apologies for the annoyance -- could you also enable this for 3.2 which I've merged recently in #1974 ?

Note: This also needs a fix on the Rakefile to add 3.2 to the changed line.

Don't bother updating the gemfiles for 3.2 since we still haven't got it enabled in CI; we'll need to do it later anyway.

ivoanjo · 2022-04-26T08:28:41Z

Appraisals

-    gem 'graphql', '>= 1.12.0', '< 1.13.0' # Newer versions are broken, needs to be investigated, see https://github.com/DataDog/dd-trace-rb/issues/1866
+    gem 'graphql'


Just to clarify -- I'm assuming this PR also fixes #1866 ?

Yes it does! Thank you for the reminder.

ivoanjo · 2022-04-26T08:36:20Z

gemfiles/ruby_2.7.3_cucumber3.gemfile.lock

-    libddwaf (1.3.0.0.0.beta1-x86_64-linux)
+    libddwaf (1.3.0.0.0.beta1)


I... think this is harmless but just in case you see issues in CI I'll point out that this changed.

I looks harmless, and CI is happy for now.
I'm not 100% sure how to avoid this with our current Rake task we use to update dependencies, but if it's an issue we'll circle back to this.

ivoanjo · 2022-04-26T08:47:44Z

lib/datadog/tracing/contrib/graphql/ext.rb

@@ -7,11 +7,11 @@ module GraphQL
        # GraphQL integration constants
        # @public_api Changing resource names, tag names, or environment variables creates breaking changes.
        module Ext


‼️ One issue I spot is that we seem to have a bit of a dependency loop, as you mention that

I'm assuming in this PR that the next version of graphql (currently 2.0.6) will contain our upstream changes. If not, I'll update our code to address that, but I believe the current implementation is ready to go.

The next version has been released as 2.0.7: https://github.com/rmosolgo/graphql-ruby/blob/master/CHANGELOG.md .

Unfortunately, the code added in rmosolgo/graphql-ruby#4038 is

if defined?(Datadog::Tracing::Metadata::Ext) # Introduced in ddtrace 1.0 span.set_tag(Datadog::Tracing::Metadata::Ext::TAG_COMPONENT, 'graphql') span.set_tag(Datadog::Tracing::Metadata::Ext::TAG_OPERATION, key) end

and if I'm looking at this correctly, this code DEPENDS on this PR. Aka I think 2.0.7 is broken for customers on 1.0.0.beta2, because beta2 has Datadog::Tracing::Metadata::Ext but no TAG_COMPONENT.

My suggestion would be to possibly submit a new PR upstream to change defined?(Datadog::Tracing::Metadata::Ext) into defined?(Datadog::Tracing::Metadata::Ext::TAG_COMPONENT), which would make graphql fallback to the previous behavior on beta1/2.

ivoanjo · 2022-04-26T09:02:21Z

lib/datadog/tracing/contrib/graphql/patcher.rb

+            # Ensure invocation to #trace method targets the new namespaced public API object,
+            # instead of the old global Datadog.trace.
+            # This is fixed in graphql > 2.0.6.
+            def tracer
+              options.fetch(:tracer, Datadog::Tracing) # GraphQL will invoke #trace on the returned object
+            end


Actually this is fixed by rmosolgo/graphql-ruby#3978 which was in 2.0.3. So technically this would not be needed in [2.0.3, 2.0.6].

(I do realize that the fallback_transaction_name fix is only in 2.0.7, and so we need the patch anyway BUT I would suggest fixing or even deleting the comment, since it's a bit misleading, and a bad comment is worse than no comment :P)

I had this comment mentioning version 2.0.3 at first, but purposefully updated it to 2.0.6 for a reason, but I don't recall why. I'll updated it to 2.0.3.

marcotc added 4 commits April 20, 2022 15:56

Add graphql gem 1.x and 2.x combinations

e329c8c

Update graphql sorbet shim

b65b4be

Add support for Graphql 2.0

370e090

Update documentation with external PR number

c414774

marcotc added the integrations Involves tracing integrations label Apr 21, 2022

marcotc added this to the 1.0.0.beta3 milestone Apr 21, 2022

marcotc requested a review from a team April 21, 2022 20:31

marcotc self-assigned this Apr 21, 2022

marcotc added 3 commits April 21, 2022 13:40

Merge branch 'master' into graphql-2.0

6af8158

Add contrib-old tests for graphql

56a2e06

Remove unused constant

03aee1b

marcotc commented Apr 21, 2022

View reviewed changes

Remove resolved TODO

0ce85b9

ivoanjo approved these changes Apr 26, 2022

View reviewed changes

Merge branch 'master' into graphql-2.0

0146358

marcotc merged commit 86d16c7 into master Apr 27, 2022

marcotc deleted the graphql-2.0 branch April 27, 2022 19:22

ivoanjo mentioned this pull request Apr 29, 2022

Bump to version 1.0.0 #1993

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for GraphQL 2.0 #1982

Add support for GraphQL 2.0 #1982

marcotc commented Apr 21, 2022 •

edited

Loading

marcotc Apr 21, 2022

ivoanjo Apr 26, 2022 •

edited

Loading

marcotc Apr 27, 2022

codecov-commenter commented Apr 21, 2022

ivoanjo left a comment

ivoanjo Apr 26, 2022

marcotc Apr 27, 2022

ivoanjo Apr 26, 2022

marcotc Apr 27, 2022

ivoanjo Apr 26, 2022

marcotc Apr 27, 2022

ivoanjo Apr 26, 2022 •

edited

Loading

ivoanjo Apr 26, 2022

marcotc Apr 27, 2022

		gem 'graphql', '>= 1.12.0', '< 1.13.0' # Newer versions are broken, needs to be investigated, see https://github.com/DataDog/dd-trace-rb/issues/1866
		gem 'graphql'

		libddwaf (1.3.0.0.0.beta1-x86_64-linux)
		libddwaf (1.3.0.0.0.beta1)

Add support for GraphQL 2.0 #1982

Add support for GraphQL 2.0 #1982

Conversation

marcotc commented Apr 21, 2022 • edited Loading

Choose a reason for hiding this comment

ivoanjo Apr 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Apr 21, 2022

Codecov Report

ivoanjo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivoanjo Apr 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcotc commented Apr 21, 2022 •

edited

Loading

ivoanjo Apr 26, 2022 •

edited

Loading

ivoanjo Apr 26, 2022 •

edited

Loading