Merge pull request #1151 from tildeio/graphql-2.5

fix graphql instrumentation

Author: zvkemp

gemfiles/Gemfile.additional

@@ -4,7 +4,7 @@ gem "delayed_job_active_record", require: false
 gem "excon"
 gem "faraday"
 gem "graphiti", git: "https://github.com/graphiti-api/graphiti.git"
-gem "graphql", "~> 2.0.18"
+gem "graphql", "~> 2.5.0"
 gem "httpclient"
 gem "rexml"
 gem "sequel", "> 0"

lib/skylight/normalizers/graphql/base.rb

@@ -59,10 +59,40 @@ class Validate < Base
     register_graphql
   end
 
-  class ExecuteMultiplex < Base
+  class Execute < Base
     register_graphql
 
-    def normalize_after(trace, _span, _name, payload)
+    # This is a new, combined normalizer for execute_query and execute_multiplex,
+    # to be used for graphql >= 2.5
+    #
+    def normalize(trace, name, payload)
+      if payload[:query]
+        _execute_query_normalize(trace, name, payload)
+      else
+        [CAT, "graphql.#{key}", nil]
+      end
+    end
+
+    def normalize_after(trace, span, name, payload)
+      if payload[:multiplex]
+        _execute_multiplex_normalize_after(trace, span, name, payload)
+      end
+    end
+
+    private
+
+    def _execute_query_normalize(trace, _name, payload)
+      query_name = extract_query_name(payload[:query])
+
+      meta = { mute_children: true } if query_name == ANONYMOUS
+      # This is probably always overriden by execute_multiplex#normalize_after,
+      # but in the case of a single query, it will be the same value anyway.
+      trace.endpoint = "graphql:#{query_name}"
+
+      [CAT, "graphql.#{key}: #{query_name}", nil, meta]
+    end
+
+    def _execute_multiplex_normalize_after(trace, _span, _name, payload)
       # This is in normalize_after because the queries may not have
       # an assigned operation name before they are executed.
       # For example, if you send a single query with a defined operation name, e.g.:
@@ -93,24 +123,18 @@ def normalize_after(trace, _span, _name, payload)
     end
   end
 
-  class AnalyzeQuery < Base
+  class ExecuteQuery < Execute
     register_graphql
   end
 
-  class ExecuteQuery < Base
+  class ExecuteMultiplex < Execute
     register_graphql
 
-    def normalize(trace, _name, payload)
-      query_name = extract_query_name(payload[:query])
-
-      meta = { mute_children: true } if query_name == ANONYMOUS
-
-      # This is probably always overriden by execute_multiplex#normalize_after,
-      # but in the case of a single query, it will be the same value anyway.
-      trace.endpoint = "graphql:#{query_name}"
+    # see behavior in Execute#_multiplex_normalize_after
+  end
 
-      [CAT, "graphql.#{key}: #{query_name}", nil, meta]
-    end
+  class AnalyzeQuery < Base
+    register_graphql
   end
 
   class ExecuteQueryLazy < ExecuteQuery

lib/skylight/probes/graphql.rb

@@ -35,6 +35,48 @@ def execute_multiplex(**metadata, &blk)
           end
         end
 
+        # GraphQL versions >= 2.5 are missing this notification
+        module ExecuteQueryNotification
+          def execute_query(**metadata, &blk)
+            if @notifications_engine
+              @notifications_engine.instrument("execute_query.graphql", metadata, &blk)
+            elsif @notifications
+              @notifications.instrument("execute_query.graphql", metadata, &blk)
+            else
+              # safety fallback in case graphql's authors unexpectedly rename @notifications_engine or @notifications
+              super
+            end
+          end
+        end
+
+        # GraphQL versions >= 2.5 are missing this notification
+        module ExecuteQueryLazyNotification
+          def execute_query_lazy(**metadata, &blk)
+            if @notifications_engine
+              @notifications_engine.instrument("execute_query_lazy.graphql", metadata, &blk)
+            elsif @notifications
+              @notifications.instrument("execute_query_lazy.graphql", metadata, &blk)
+            else
+              # safety fallback in case graphql's authors unexpectedly rename @notifications_engine or @notifications
+              super
+            end
+          end
+        end
+
+        # GraphQL versions >= 2.5 are missing this notification
+        module AnalyzeQueryNotification 
+          def analyze_query(**metadata, &blk)
+            if @notifications_engine
+              @notifications_engine.instrument("analyze_query.graphql", metadata, &blk)
+            elsif @notifications
+              @notifications.instrument("analyze_query.graphql", metadata, &blk)
+            else
+              # safety fallback in case graphql's authors unexpectedly rename @notifications_engine or @notifications
+              super
+            end
+          end
+        end
+        
         module ClassMethods
           def new_trace(*, **)
             unless @__sk_instrumentation_installed
@@ -44,6 +86,18 @@ def new_trace(*, **)
                 trace_with(ExecuteMultiplexNotification)
               end
 
+              unless ::GraphQL::Tracing::ActiveSupportNotificationsTrace.instance_methods.include?(:execute_query)
+                trace_with(ExecuteQueryNotification)
+              end
+
+              unless ::GraphQL::Tracing::ActiveSupportNotificationsTrace.instance_methods.include?(:execute_query_lazy)
+                trace_with(ExecuteQueryLazyNotification)
+              end
+
+              unless ::GraphQL::Tracing::ActiveSupportNotificationsTrace.instance_methods.include?(:analyze_query)
+                trace_with(AnalyzeQueryNotification)
+              end
+
               @__sk_instrumentation_installed = true
             end
 

spec/integration/graphql_spec.rb

@@ -11,8 +11,12 @@
 end
 
 if enable
-  def test_interpreter_schema?
-    defined?(GraphQL::Execution::Interpreter)
+  def graphql_2_5?
+    GraphQL::VERSION >= Gem::Version.new("2.5.0")
+  end
+
+  def multiplex_event
+    graphql_2_5? ? "graphql.execute" : "graphql.execute_multiplex"
   end
 
   describe "graphql integration" do
@@ -309,21 +313,15 @@ def make_graphql_request(query:, variables: {}, **params)
           call env("/", method: :POST, params: { query: query, variables: variables, **params })
         end
 
-        # Handles expected analysis events for legacy style (GraphQL 1.9.x)
-        # and new interpreter style (GraphQL >= 1.9.x when using GraphQL::Execution::Interpreter).
         def expected_analysis_events(query_count = 1)
-          events = [%w[app.graphql graphql.lex], %w[app.graphql graphql.parse], %w[app.graphql graphql.validate]].freeze
-
-          analyze_event = %w[app.graphql graphql.analyze_query]
-          event_style = expectation_event_style
-          case event_style
-          when :grouped
-            events.cycle(query_count).to_a.tap { |a| a.concat([analyze_event].cycle(query_count).to_a) }
-          when :inline
-            [*events, analyze_event].cycle(query_count)
-          else
-            raise "Unexpected expectation_event_style: #{event_style}"
-          end.to_a
+          events = graphql_2_5? ? [] : [%w[app.graphql graphql.lex]]
+          events |= [
+            %w[app.graphql graphql.parse], 
+            %w[app.graphql graphql.validate],
+            %w[app.graphql graphql.analyze_query]
+          ]
+
+          events.cycle(query_count).to_a
         end
 
         let(:query_inner) { "#{TestApp.format_field_name("someDragonflies")} { name }" }
@@ -349,7 +347,7 @@ def expected_analysis_events(query_count = 1)
             expect(data).to eq(
               [
                 ["app.rack.request", nil],
-                %w[app.graphql graphql.execute_multiplex],
+                ["app.graphql", multiplex_event],
                 *expected_analysis_events,
                 ["app.graphql", "graphql.execute_query: #{query_name}"],
                 ["app.graphql", "graphql.execute_query_lazy: #{query_name}"]
@@ -377,7 +375,7 @@ def expected_analysis_events(query_count = 1)
             expect(data).to eq(
               [
                 ["app.rack.request", nil],
-                %w[app.graphql graphql.execute_multiplex],
+                ["app.graphql", multiplex_event],
                 *expected_analysis_events,
                 ["app.graphql", "graphql.execute_query: #{query_name}"],
                 ["db.sql.query", "SELECT FROM species"],
@@ -414,7 +412,7 @@ def expected_analysis_events(query_count = 1)
             expect(data).to eq(
               [
                 ["app.rack.request", nil],
-                %w[app.graphql graphql.execute_multiplex],
+                ["app.graphql", multiplex_event],
                 *expected_analysis_events,
                 ["app.graphql", "graphql.execute_query: #{query_name}"],
                 ["db.sql.query", "SELECT FROM species"],
@@ -448,7 +446,7 @@ def expected_analysis_events(query_count = 1)
             expect(data).to eq(
               [
                 ["app.rack.request", nil],
-                %w[app.graphql graphql.execute_multiplex],
+                ["app.graphql", multiplex_event],
                 *expected_analysis_events(3),
                 ["app.graphql", "graphql.execute_query: #{query_name}"],
                 ["app.graphql", "graphql.execute_query: #{query_name}"],
@@ -479,7 +477,7 @@ def expected_analysis_events(query_count = 1)
             expect(data).to eq(
               [
                 ["app.rack.request", nil],
-                %w[app.graphql graphql.execute_multiplex],
+                ["app.graphql", multiplex_event],
                 *expected_analysis_events(3),
                 *%w[query-0 query-1 query-2]
                   .map do |qn|
@@ -518,7 +516,7 @@ def expected_analysis_events(query_count = 1)
             expect(data).to eq(
               [
                 ["app.rack.request", nil],
-                %w[app.graphql graphql.execute_multiplex],
+                ["app.graphql", multiplex_event],
                 *expected_analysis_events(4),
                 ["app.graphql", "graphql.execute_query: #{query_name}"],
                 ["app.graphql", "graphql.execute_query: #{query_name}"],
@@ -557,7 +555,7 @@ def expected_analysis_events(query_count = 1)
             expect(data).to eq(
               [
                 ["app.rack.request", nil],
-                %w[app.graphql graphql.execute_multiplex],
+                ["app.graphql", multiplex_event],
                 *expected_analysis_events(2),
                 ["app.graphql", "graphql.execute_query: myFavoriteDragonflies"],
                 ["db.sql.query", "SELECT FROM species"],
@@ -595,7 +593,7 @@ def expected_analysis_events(query_count = 1)
             expect(data).to eq(
               [
                 ["app.rack.request", nil],
-                %w[app.graphql graphql.execute_multiplex],
+                ["app.graphql", multiplex_event],
                 *expected_analysis_events(2)[0..-2],
                 ["app.graphql", "graphql.execute_query: myFavoriteDragonflies"],
                 ["db.sql.query", "SELECT FROM species"],
@@ -630,7 +628,7 @@ def expected_analysis_events(query_count = 1)
             expect(data).to eq(
               [
                 ["app.rack.request", nil],
-                %w[app.graphql graphql.execute_multiplex],
+                ["app.graphql", multiplex_event],
                 *expected_analysis_events(2).reject { |_, e| e["graphql.analyze"] },
                 %w[app.graphql graphql.execute_query_lazy.multiplex]
               ]
@@ -672,7 +670,7 @@ def expected_analysis_events(query_count = 1)
             expect(data).to eq(
               [
                 ["app.rack.request", nil],
-                %w[app.graphql graphql.execute_multiplex],
+                ["app.graphql", multiplex_event],
                 *expected_analysis_events,
                 ["app.graphql", "graphql.execute_query: #{mutation_name}"],
                 ["db.sql.query", "SELECT FROM genera"],
@@ -688,14 +686,7 @@ def expected_analysis_events(query_count = 1)
       end
 
       configs = []
-
-      configs << { schema: :InterpreterSchema, expectation_event_style: :inline } if test_interpreter_schema?
-
-      # GraphQL::Execution::Interpreter became the default as of 1.12, so we do not need to
-      # test an additional schema for versions >= 1.12.
-      if Gem::Version.new(GraphQL::VERSION) < Gem::Version.new("1.12")
-        configs << { schema: :TestAppSchema, expectation_event_style: :grouped }
-      end
+      configs << { schema: :InterpreterSchema, expectation_event_style: :inline }
 
       configs.each do |config|
         context config[:schema].to_s do