Merge pull request #3859 from DataDog/graphql-fix

DataDog · Sep 17, 2024 · e32e038 · e32e038
2 parents 7c84edb + 6274db9
commit e32e038
Show file tree

Hide file tree

Showing 12 changed files with 209 additions and 153 deletions.
diff --git a/Rakefile b/Rakefile
@@ -84,6 +84,7 @@ desc 'Run RSpec'
 # rubocop:disable Metrics/BlockLength
 namespace :spec do
   task all: [:main, :benchmark,
+             :graphql, :graphql_unified_trace_patcher, :graphql_trace_patcher, :graphql_tracing_patcher,
              :rails, :railsredis, :railsredis_activesupport, :railsactivejob,
              :elasticsearch, :http, :redis, :sidekiq, :sinatra, :hanami, :hanami_autoinstrument,
              :profiling, :crashtracking]
@@ -101,6 +102,27 @@ namespace :spec do
     t.rspec_opts = args.to_a.join(' ')
   end
 
+  RSpec::Core::RakeTask.new(:graphql) do |t, args|
+    t.pattern = 'spec/datadog/tracing/contrib/graphql/**/*_spec.rb'
+    t.exclude_pattern = 'spec/datadog/tracing/contrib/graphql/{unified_trace,trace,tracing}_patcher_spec.rb'
+    t.rspec_opts = args.to_a.join(' ')
+  end
+
+  RSpec::Core::RakeTask.new(:graphql_unified_trace_patcher) do |t, args|
+    t.pattern = 'spec/datadog/tracing/contrib/graphql/unified_trace_patcher_spec.rb'
+    t.rspec_opts = args.to_a.join(' ')
+  end
+
+  RSpec::Core::RakeTask.new(:graphql_trace_patcher) do |t, args|
+    t.pattern = 'spec/datadog/tracing/contrib/graphql/trace_patcher_spec.rb'
+    t.rspec_opts = args.to_a.join(' ')
+  end
+
+  RSpec::Core::RakeTask.new(:graphql_tracing_patcher) do |t, args|
+    t.pattern = 'spec/datadog/tracing/contrib/graphql/tracing_patcher_spec.rb'
+    t.rspec_opts = args.to_a.join(' ')
+  end
+
   desc '' # "Explicitly hiding from `rake -T`"
   RSpec::Core::RakeTask.new(:opentelemetry) do |t, args|
     t.pattern = 'spec/datadog/opentelemetry/**/*_spec.rb,spec/datadog/opentelemetry_spec.rb'
@@ -223,7 +245,6 @@ namespace :spec do
     :excon,
     :faraday,
     :grape,
-    :graphql,
     :grpc,
     :http,
     :httpclient,

diff --git a/docs/GettingStarted.md b/docs/GettingStarted.md
@@ -845,8 +845,14 @@ To activate your integration, use the `Datadog.configure` method:
 
 ```ruby
 # Inside Rails initializer or equivalent
+# For graphql >= v2.2
 Datadog.configure do |c|
-  c.tracing.instrument :graphql, schemas: [YourSchema], **options
+  c.tracing.instrument :graphql, with_unified_tracer: true, **options
+end
+
+# For graphql < v2.2
+Datadog.configure do |c|
+   c.tracing.instrument :graphql, **options
 end
 
 # Then run a GraphQL query
@@ -859,31 +865,38 @@ The `instrument :graphql` method accepts the following parameters. Additional op
 | ------------------------ | -------------------------- | -------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ---------------- |
 | `enabled`                | `DD_TRACE_GRAPHQL_ENABLED` | `Bool`   | Whether the integration should create spans.                                                                                                                                                                    | `true`           |
 | `schemas`                |                            | `Array`  | Array of `GraphQL::Schema` objects (that support class-based schema only) to trace. If you do not provide any, then tracing will applied to all the schemas.                                                    | `[]`             |
-| `with_unified_tracer`    |                            | `Bool`   | Enable to instrument with `UnifiedTrace` tracer, enabling support for API Catalog. `with_deprecated_tracer` has priority over this. Default is `false`, using `GraphQL::Tracing::DataDogTrace` (Added in v2.2)  | `false`          |
-| `with_deprecated_tracer` |                            | `Bool`   | Enable to instrument with deprecated `GraphQL::Tracing::DataDogTracing`. This has priority over `with_unified_tracer`. Default is `false`, using `GraphQL::Tracing::DataDogTrace`                               | `false`          |
+| `with_unified_tracer`    |                            | `Bool`   | (Recommended) Enable to instrument with `UnifiedTrace` tracer for `graphql` >= v2.2, **enabling support for API Catalog**. `with_deprecated_tracer` has priority over this. Default is `false`, using `GraphQL::Tracing::DataDogTrace` instead | `false` |
+| `with_deprecated_tracer` |                            | `Bool`   | Enable to instrument with deprecated `GraphQL::Tracing::DataDogTracing`. This has priority over `with_unified_tracer`. Default is `false`, using `GraphQL::Tracing::DataDogTrace` instead | `false` |
 | `service_name`           |                            | `String` | Service name used for graphql instrumentation                                                                                                                                                                   | `'ruby-graphql'` |
 
+Once an instrumentation strategy is selected (`with_unified_tracer: true`, `with_deprecated_tracer: true`, or *no option set* which defaults to `GraphQL::Tracing::DataDogTrace`), it is not possible to change the instrumentation strategy in the same Ruby process.
+This is especially important for [auto instrumented applications](#rails-or-hanami-applications) because an automatic initial instrumentation is always applied at startup, thus such applications will always instrument GraphQL with the default strategy (`GraphQL::Tracing::DataDogTrace`).
+
 **Manually configuring GraphQL schemas**
 
-If you prefer to individually configure the tracer settings for a schema (e.g. you have multiple schemas), in the schema definition, you can add the following [using the GraphQL API](http://graphql-ruby.org/queries/tracing.html):
+If you prefer, you can individually configure the tracer settings per schema (e.g. you have multiple schemas with distinct instrumentation options).
+
+Do _NOT_ `c.tracing.instrument :graphql` in `Datadog.configure` if you choose to configure schema settings manually, as to avoid double tracing. These two means of configuring GraphQL tracing are mutually exclusive.
+
+To instrument each schema individually, you add the following [using the GraphQL API](http://graphql-ruby.org/queries/tracing.html):
 
-With `GraphQL::Tracing::DataDogTrace`
+For `graphql` >= v2.2:
 
 ```ruby
 class YourSchema < GraphQL::Schema
-  trace_with GraphQL::Tracing::DataDogTrace
+  trace_with Datadog::Tracing::Contrib::GraphQL::UnifiedTrace
 end
 ```
 
-With `UnifiedTracer` (Added in v2.2)
+For `graphql` < v2.2:
 
 ```ruby
 class YourSchema < GraphQL::Schema
-  trace_with Datadog::Tracing::Contrib::GraphQL::UnifiedTrace
+  trace_with GraphQL::Tracing::DataDogTrace
 end
 ```
 
-or with `GraphQL::Tracing::DataDogTracing` (deprecated)
+Using the deprecated tracer GraphQL (`GraphQL::Tracing::DataDogTracing`):
 
 ```ruby
 class YourSchema < GraphQL::Schema
@@ -893,8 +906,6 @@ end
 
 **Note**: This integration does not support define-style schemas. Only class-based schemas are supported.
 
-Do _NOT_ `instrument :graphql` in `Datadog.configure` if you choose to configure manually, as to avoid double tracing. These two means of configuring GraphQL tracing are considered mutually exclusive.
-
 **Adding custom tags to Datadog spans**
 
 You can add custom tags to Datadog spans by implementing the `prepare_span` method in a subclass, then manually configuring your schema.

diff --git a/lib/datadog/tracing/contrib/graphql/patcher.rb b/lib/datadog/tracing/contrib/graphql/patcher.rb
@@ -21,32 +21,29 @@ def target_version
           end
 
           def patch
+            # DEV-3.0: We should remove as many patching options as possible, given the alternatives do not
+            # DEV-3.0: provide any benefit to the recommended `with_unified_tracer` patching method.
+            # DEV-3.0: `with_deprecated_tracer` is likely safe to remove.
+            # DEV-3.0: `with_unified_tracer: false` should be removed if possible.
+            # DEV-3.0: `with_unified_tracer: true` should be the default and hopefully not even necessary as an option.
             if configuration[:with_deprecated_tracer]
-              TracingPatcher.patch!(schemas, trace_options)
+              TracingPatcher.patch!(schemas)
             elsif Integration.trace_supported?
               if configuration[:with_unified_tracer]
-                UnifiedTracePatcher.patch!(schemas, trace_options)
+                UnifiedTracePatcher.patch!(schemas)
               else
-                TracePatcher.patch!(schemas, trace_options)
+                TracePatcher.patch!(schemas)
               end
             else
               Datadog.logger.warn(
                 "GraphQL version (#{target_version}) does not support GraphQL::Tracing::DataDogTrace"\
                 'or Datadog::Tracing::Contrib::GraphQL::UnifiedTrace.'\
                 'Falling back to GraphQL::Tracing::DataDogTracing.'
               )
-              TracingPatcher.patch!(schemas, trace_options)
+              TracingPatcher.patch!(schemas)
             end
           end
 
-          def trace_options
-            {
-              service: configuration[:service_name],
-              analytics_enabled: Contrib::Analytics.enabled?(configuration[:analytics_enabled]),
-              analytics_sample_rate: configuration[:analytics_sample_rate]
-            }
-          end
-
           def configuration
             Datadog.configuration.tracing[:graphql]
           end

diff --git a/lib/datadog/tracing/contrib/graphql/trace_patcher.rb b/lib/datadog/tracing/contrib/graphql/trace_patcher.rb
@@ -8,12 +8,12 @@ module GraphQL
         module TracePatcher
           module_function
 
-          def patch!(schemas, options)
+          def patch!(schemas)
             if schemas.empty?
-              ::GraphQL::Schema.trace_with(::GraphQL::Tracing::DataDogTrace, **options)
+              ::GraphQL::Schema.trace_with(::GraphQL::Tracing::DataDogTrace)
             else
               schemas.each do |schema|
-                schema.trace_with(::GraphQL::Tracing::DataDogTrace, **options)
+                schema.trace_with(::GraphQL::Tracing::DataDogTrace)
               end
             end
           end

diff --git a/lib/datadog/tracing/contrib/graphql/tracing_patcher.rb b/lib/datadog/tracing/contrib/graphql/tracing_patcher.rb
@@ -8,13 +8,13 @@ module GraphQL
         module TracingPatcher
           module_function
 
-          def patch!(schemas, options)
+          def patch!(schemas)
             if schemas.empty?
-              ::GraphQL::Schema.tracer(::GraphQL::Tracing::DataDogTracing.new(**options))
+              ::GraphQL::Schema.tracer(::GraphQL::Tracing::DataDogTracing.new)
             else
               schemas.each do |schema|
                 if schema.respond_to? :use
-                  schema.use(::GraphQL::Tracing::DataDogTracing, **options)
+                  schema.use(::GraphQL::Tracing::DataDogTracing)
                 else
                   Datadog.logger.warn("Unable to patch #{schema}: Please migrate to class-based schema.")
                 end

diff --git a/lib/datadog/tracing/contrib/graphql/unified_trace.rb b/lib/datadog/tracing/contrib/graphql/unified_trace.rb
@@ -11,14 +11,7 @@ module GraphQL
         # which is required to use features such as API Catalog.
         # DEV-3.0: This tracer should be the default one in the next major version.
         module UnifiedTrace
-          # @param analytics_enabled [Boolean] Deprecated
-          # @param analytics_sample_rate [Float] Deprecated
-          # @param service [String|nil] The service name to be set on the spans
-          def initialize(*args, analytics_enabled: false, analytics_sample_rate: 1.0, service: nil, **kwargs)
-            @analytics_enabled = analytics_enabled
-            @analytics_sample_rate = analytics_sample_rate
-
-            @service_name = service
+          def initialize(*args, **kwargs)
             @has_prepare_span = respond_to?(:prepare_span)
             super
           end
@@ -139,7 +132,18 @@ def platform_resolve_type_key(type, *args, **kwargs)
           private
 
           def trace(callable, trace_key, resource, **kwargs)
-            Tracing.trace("graphql.#{trace_key}", resource: resource, service: @service_name, type: 'graphql') do |span|
+            config = Datadog.configuration.tracing[:graphql]
+
+            Tracing.trace(
+              "graphql.#{trace_key}",
+              type: 'graphql',
+              resource: resource,
+              service: config[:service_name]
+            ) do |span|
+              if Contrib::Analytics.enabled?(config[:analytics_enabled])
+                Contrib::Analytics.set_sample_rate(span, config[:analytics_sample_rate])
+              end
+
               yield(span) if block_given?
 
               prepare_span(trace_key, kwargs, span) if @has_prepare_span

diff --git a/lib/datadog/tracing/contrib/graphql/unified_trace_patcher.rb b/lib/datadog/tracing/contrib/graphql/unified_trace_patcher.rb
@@ -12,12 +12,15 @@ module GraphQL
         module UnifiedTracePatcher
           module_function
 
-          def patch!(schemas, options)
+          # TODO: `GraphQL::Schema.trace_with` and `YOUR_SCHEMA.trace_with` don't mix.
+          # TODO: They create duplicate spans when combined.
+          # TODO: We should measure how frequently users use `YOUR_SCHEMA.trace_with`, and hopefully we can remove it.
+          def patch!(schemas)
             if schemas.empty?
-              ::GraphQL::Schema.trace_with(UnifiedTrace, **options)
+              ::GraphQL::Schema.trace_with(UnifiedTrace)
             else
               schemas.each do |schema|
-                schema.trace_with(UnifiedTrace, **options)
+                schema.trace_with(UnifiedTrace)
               end
             end
           end