Commit 170885ed authored by Stan Hu's avatar Stan Hu

Add Sentry logging to API calls

Closes #21043
parent 7b0b2417
......@@ -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)
......
......@@ -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
......
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
......@@ -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
......
......@@ -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
......@@ -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
......
......@@ -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
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment