Merge branch 'add-sentry-logging-to-api' into 'master'

CHANGELOG

@@ -5,6 +5,7 @@ v 8.12.0 (unreleased)
   - Add `web_url` field to issue, merge request, and snippet API objects (Ben Boeckel)
   - Optimistic locking for Issues and Merge Requests (title and description overriding prevention)
   - Add `wiki_page_events` to project hook APIs (Ben Boeckel)
+  - Add Sentry logging to API calls
   - Added tests for diff notes
   - Added 'only_allow_merge_if_build_succeeds' project setting in the API. !5930 (Duck)
 

app/controllers/application_controller.rb

@@ -6,6 +6,7 @@ class ApplicationController < ActionController::Base
   include Gitlab::GonHelper
   include GitlabRoutingHelper
   include PageLayoutHelper
+  include SentryHelper
   include WorkhorseHelper
 
   before_action :authenticate_user_from_private_token!
@@ -46,28 +47,6 @@ class ApplicationController < ActionController::Base
 
   protected
 
-  def sentry_context
-    if Rails.env.production? && current_application_settings.sentry_enabled
-      if current_user
-        Raven.user_context(
-          id: current_user.id,
-          email: current_user.email,
-          username: current_user.username,
-        )
-      end
-
-      Raven.tags_context(program: sentry_program_context)
-    end
-  end
-
-  def sentry_program_context
-    if Sidekiq.server?
-      'sidekiq'
-    else
-      'rails'
-    end
-  end
-
   # This filter handles both private tokens and personal access tokens
   def authenticate_user_from_private_token!
     token_string = params[:private_token].presence || request.headers['PRIVATE-TOKEN'].presence

app/helpers/sentry_helper.rb

+module SentryHelper
+  def sentry_enabled?
+    Rails.env.production? && current_application_settings.sentry_enabled?
+  end
+
+  def sentry_context
+    return unless sentry_enabled?
+
+    if current_user
+      Raven.user_context(
+        id: current_user.id,
+        email: current_user.email,
+        username: current_user.username,
+      )
+    end
+
+    Raven.tags_context(program: sentry_program_context)
+  end
+
+  def sentry_program_context
+    if Sidekiq.server?
+      'sidekiq'
+    else
+      'rails'
+    end
+  end
+end

lib/api/api.rb

@@ -18,22 +18,14 @@ module API
     end
 
     rescue_from :all do |exception|
-      # lifted from https://github.com/rails/rails/blob/master/actionpack/lib/action_dispatch/middleware/debug_exceptions.rb#L60
-      # why is this not wrapped in something reusable?
-      trace = exception.backtrace
-
-      message = "\n#{exception.class} (#{exception.message}):\n"
-      message << exception.annoted_source_code.to_s if exception.respond_to?(:annoted_source_code)
-      message << "  " << trace.join("\n  ")
-
-      API.logger.add Logger::FATAL, message
-      rack_response({ 'message' => '500 Internal Server Error' }.to_json, 500)
+      handle_api_exception(exception)
     end
 
     format :json
     content_type :txt, "text/plain"
 
     # Ensure the namespace is right, otherwise we might load Grape::API::Helpers
+    helpers ::SentryHelper
     helpers ::API::Helpers
 
     mount ::API::AccessRequests

lib/api/helpers.rb

@@ -279,6 +279,24 @@ module API
       error!({ 'message' => message }, status)
     end
 
+    def handle_api_exception(exception)
+      if sentry_enabled? && report_exception?(exception)
+        define_params_for_grape_middleware
+        sentry_context
+        Raven.capture_exception(exception)
+      end
+
+      # lifted from https://github.com/rails/rails/blob/master/actionpack/lib/action_dispatch/middleware/debug_exceptions.rb#L60
+      trace = exception.backtrace
+
+      message = "\n#{exception.class} (#{exception.message}):\n"
+      message << exception.annoted_source_code.to_s if exception.respond_to?(:annoted_source_code)
+      message << "  " << trace.join("\n  ")
+
+      API.logger.add Logger::FATAL, message
+      rack_response({ 'message' => '500 Internal Server Error' }.to_json, 500)
+    end
+
     # Projects helpers
 
     def filter_projects(projects)
@@ -419,5 +437,19 @@ module API
         Entities::Issue
       end
     end
+
+    # The Grape Error Middleware only has access to env but no params. We workaround this by
+    # defining a method that returns the right value.
+    def define_params_for_grape_middleware
+      self.define_singleton_method(:params) { Rack::Request.new(env).params.symbolize_keys }
+    end
+
+    # We could get a Grape or a standard Ruby exception. We should only report anything that
+    # is clearly an error.
+    def report_exception?(exception)
+      return true unless exception.respond_to?(:status)
+
+      exception.status == 500
+    end
   end
 end

lib/ci/api/api.rb

@@ -9,22 +9,14 @@ module Ci
       end
 
       rescue_from :all do |exception|
-        # lifted from https://github.com/rails/rails/blob/master/actionpack/lib/action_dispatch/middleware/debug_exceptions.rb#L60
-        # why is this not wrapped in something reusable?
-        trace = exception.backtrace
-
-        message = "\n#{exception.class} (#{exception.message}):\n"
-        message << exception.annoted_source_code.to_s if exception.respond_to?(:annoted_source_code)
-        message << "  " << trace.join("\n  ")
-
-        API.logger.add Logger::FATAL, message
-        rack_response({ 'message' => '500 Internal Server Error' }, 500)
+        handle_api_exception(exception)
       end
 
       content_type :txt,  'text/plain'
       content_type :json, 'application/json'
       format :json
 
+      helpers ::SentryHelper
       helpers ::Ci::API::Helpers
       helpers ::API::Helpers
       helpers Gitlab::CurrentSettings

spec/requests/api/api_helpers_spec.rb

@@ -3,6 +3,7 @@ require 'spec_helper'
 describe API::Helpers, api: true do
   include API::Helpers
   include ApiHelpers
+  include SentryHelper
 
   let(:user) { create(:user) }
   let(:admin) { create(:admin) }
@@ -234,4 +235,30 @@ describe API::Helpers, api: true do
       expect(to_boolean(nil)).to be_nil
     end
   end
+
+  describe '.handle_api_exception' do
+    before do
+      allow_any_instance_of(self.class).to receive(:sentry_enabled?).and_return(true)
+      allow_any_instance_of(self.class).to receive(:rack_response)
+    end
+
+    it 'does not report a MethodNotAllowed exception to Sentry' do
+      exception = Grape::Exceptions::MethodNotAllowed.new({ 'X-GitLab-Test' => '1' })
+      allow(exception).to receive(:backtrace).and_return(caller)
+
+      expect(Raven).not_to receive(:capture_exception).with(exception)
+
+      handle_api_exception(exception)
+    end
+
+    it 'does report RuntimeError to Sentry' do
+      exception = RuntimeError.new('test error')
+      allow(exception).to receive(:backtrace).and_return(caller)
+
+      expect_any_instance_of(self.class).to receive(:sentry_context)
+      expect(Raven).to receive(:capture_exception).with(exception)
+
+      handle_api_exception(exception)
+    end
+  end
 end