diff --git a/CHANGELOG b/CHANGELOG
index 3e6066cfae046afab2b3d0c5d1d7e9cf10ce3298..ea9ebf740e1843f6fccb93ed857942224166b73c 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -4,6 +4,7 @@ v 8.12.0 (unreleased)
- Change merge_error column from string to text type
- 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
v 8.11.2 (unreleased)
diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
index 634d36a44671ed450597061fcbb3f6bd9f9903fa..70a2275592bfb091d1d4401e13a3b740bceeaa29 100644
--- a/app/controllers/application_controller.rb
+++ b/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
diff --git a/app/helpers/sentry_helper.rb b/app/helpers/sentry_helper.rb
new file mode 100644
index 0000000000000000000000000000000000000000..f8cccade15b5520ffb8405d393fab7fbc34f0ea9
--- /dev/null
+++ b/app/helpers/sentry_helper.rb
@@ -0,0 +1,27 @@
+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
diff --git a/lib/api/api.rb b/lib/api/api.rb
index 6b8bfbbdae6110e5fa5067653d10987d18b77d67..ecbd5a6e2fabca8b83759824cb9a33ecaed280ff 100644
--- a/lib/api/api.rb
+++ b/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
diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb
index d0469d6602d0df0feb595a1285a0c8420703c02b..da4b1bf9902c8493fcaa3fb49dd96e79efad0622 100644
--- a/lib/api/helpers.rb
+++ b/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
diff --git a/lib/ci/api/api.rb b/lib/ci/api/api.rb
index 17bb99a2ae50248da39cb42fe1e545daa8b7f57e..a6b9beecded18128ccd4ee5d3b7217e05918edff 100644
--- a/lib/ci/api/api.rb
+++ b/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
diff --git a/spec/requests/api/api_helpers_spec.rb b/spec/requests/api/api_helpers_spec.rb
index c65510fadecf0dc1aafabb76ac52ce064c58f4f7..bbdf8f03c2bc3578aec1c547d57853677ca2ed55 100644
--- a/spec/requests/api/api_helpers_spec.rb
+++ b/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