Merge branch '498768-graphql-subscriptions-ignore-unauthorized-error' into 'master'

Ignore unauthorized subscription errors in SLIs

See merge request !173506



Merged-by: Bob Van Landuyt <bob@gitlab.com>
Approved-by: Bob Van Landuyt <bob@gitlab.com>
Reviewed-by: Bob Van Landuyt <bob@gitlab.com>
Reviewed-by: Heinrich Lee Yu <heinrich@gitlab.com>
Co-authored-by: Heinrich Lee Yu <heinrich@gitlab.com>

app/graphql/subscriptions/base_subscription.rb

@@ -5,6 +5,8 @@ class BaseSubscription < GraphQL::Schema::Subscription
     object_class Types::BaseObject
     field_class Types::BaseField
 
+    UNAUTHORIZED_ERROR_MESSAGE = 'Unauthorized subscription'
+
     def initialize(object:, context:, field:)
       super
 
@@ -33,7 +35,7 @@ def authorized?(*)
     def unauthorized!
       unsubscribe if context.query.subscription_update?
 
-      raise GraphQL::ExecutionError, 'Unauthorized subscription'
+      raise GraphQL::ExecutionError, UNAUTHORIZED_ERROR_MESSAGE
     end
 
     def current_user

lib/gitlab/graphql/tracers/instrumentation_tracer.rb

@@ -7,6 +7,10 @@ module Tracers
       module InstrumentationTracer
         MUTATION_REGEXP = /^mutation/
 
+        IGNORED_ERRORS = [
+          ::Subscriptions::BaseSubscription::UNAUTHORIZED_ERROR_MESSAGE
+        ].freeze
+
         # All queries pass through a multiplex, even if only one query is executed
         # https://github.com/rmosolgo/graphql-ruby/blob/43e377b5b743a9102381d6ad3adaaed13ff5b6dd/lib/graphql/schema.rb#L1303
         #
@@ -31,15 +35,16 @@ def execute_multiplex(multiplex:)
 
         def export_query_info(query:, duration_s:, exception:)
           operation = ::Gitlab::Graphql::KnownOperations.default.from_query(query)
-          has_errors = exception || query.result['errors'].present?
+
+          error_type = error_type(query: query, exception: exception)
 
           ::Gitlab::ApplicationContext.with_context(caller_id: operation.to_caller_id) do
             log_execute_query(query: query, duration_s: duration_s, exception: exception)
-            increment_query_sli(operation: operation, duration_s: duration_s, has_errors: has_errors)
+            increment_query_sli(operation: operation, duration_s: duration_s, error_type: error_type)
           end
         end
 
-        def increment_query_sli(operation:, duration_s:, has_errors:)
+        def increment_query_sli(operation:, duration_s:, error_type:)
           query_urgency = operation.query_urgency
           labels = {
             endpoint_id: operation.to_caller_id,
@@ -49,10 +54,10 @@ def increment_query_sli(operation:, duration_s:, has_errors:)
 
           Gitlab::Metrics::RailsSlis.graphql_query_error_rate.increment(
             labels: labels,
-            error: has_errors
+            error: error_type == :error
           )
 
-          return if has_errors
+          return if error_type
 
           Gitlab::Metrics::RailsSlis.graphql_query_apdex.increment(
             labels: labels,
@@ -60,6 +65,18 @@ def increment_query_sli(operation:, duration_s:, has_errors:)
           )
         end
 
+        def error_type(query:, exception:)
+          errors = query.result['errors']&.pluck('message')
+
+          return if errors.blank?
+
+          if exception || (errors - IGNORED_ERRORS).present?
+            :error
+          else
+            :ignored
+          end
+        end
+
         def log_execute_query(query: nil, duration_s: 0, exception: nil)
           # execute_query should always have :query, but we're just being defensive
           return unless query

spec/channels/graphql_channel_spec.rb

@@ -110,5 +110,30 @@
         end
       end
     end
+
+    describe 'metrics' do
+      before do
+        stub_action_cable_connection current_user: create(:user)
+      end
+
+      it 'does not track unauthorized subscriptions as errors' do
+        expect(Gitlab::Metrics::RailsSlis.graphql_query_apdex).not_to receive(:increment)
+
+        expect(Gitlab::Metrics::RailsSlis.graphql_query_error_rate).to receive(:increment).with({
+          labels: anything,
+          error: false
+        })
+
+        subscribe(subscribe_params)
+
+        expect(transmissions.first['result']).to match(a_hash_including(
+          'errors' => [
+            a_hash_including(
+              'message' => 'Unauthorized subscription'
+            )
+          ]
+        ))
+      end
+    end
   end
 end

spec/requests/api/graphql/instrumentation_tracer_spec.rb

@@ -152,6 +152,65 @@
 
       expect(json_response.size).to eq(2)
     end
+
+    context 'with IGNORED_ERRORS' do
+      before do
+        stub_const('Gitlab::Graphql::Tracers::InstrumentationTracer::IGNORED_ERRORS', [
+          'an ignored error'
+        ])
+      end
+
+      it 'does not mark request as having an error when it is ignored' do
+        expect(Gitlab::Metrics::RailsSlis.graphql_query_apdex).not_to receive(:increment)
+
+        expect_next_instance_of(Resolvers::EchoResolver) do |resolver|
+          expect(resolver).to receive(:resolve).and_raise(GraphQL::ExecutionError, 'an ignored error')
+        end
+
+        expect(Gitlab::Metrics::RailsSlis.graphql_query_error_rate).to receive(:increment).with({
+          labels: unknown_query_labels,
+          error: be_falsey
+        })
+
+        post_graphql(graphql_query_for('echo', { 'text' => 'test' }, []))
+      end
+
+      context 'when request has multiple errors' do
+        it 'marks request as having an error when at least one error is not ignored' do
+          expect(Gitlab::Metrics::RailsSlis.graphql_query_apdex).not_to receive(:increment)
+
+          expect(Resolvers::EchoResolver).to receive(:new).and_wrap_original do |method, **kwargs|
+            kwargs[:context].add_error(GraphQL::ExecutionError.new('a real error'))
+            kwargs[:context].add_error(GraphQL::ExecutionError.new('an ignored error'))
+            method.call(**kwargs)
+          end
+
+          expect(Gitlab::Metrics::RailsSlis.graphql_query_error_rate).to receive(:increment).with({
+            labels: unknown_query_labels,
+            error: true
+          })
+
+          post_graphql(graphql_query_for('echo', { 'text' => 'test' }, []))
+        end
+
+        it 'does not mark request as having an error when all errors are ignored' do
+          expect(Gitlab::Metrics::RailsSlis.graphql_query_apdex).not_to receive(:increment)
+
+          expect(Resolvers::EchoResolver).to receive(:new).and_wrap_original do |method, **kwargs|
+            kwargs[:context].add_error(GraphQL::ExecutionError.new('an ignored error'))
+            kwargs[:context].add_error(GraphQL::ExecutionError.new('an ignored error'))
+            method.call(**kwargs)
+          end
+
+          expect(Gitlab::Metrics::RailsSlis.graphql_query_error_rate).to receive(:increment).with({
+            labels: unknown_query_labels,
+            error: be_falsey
+          })
+
+          post_graphql(graphql_query_for('echo', { 'text' => 'test' }, []))
+        end
+      end
+    end
   end
 
   it "recognizes known queries from our frontend" do