Service for calling Sentry issues api

This commit is contained in:
Reuben Pereira 2019-01-09 21:04:27 +00:00 committed by Kamil Trzciński
parent 47698eec40
commit d69074fc72
22 changed files with 829 additions and 16 deletions

View file

@ -0,0 +1,54 @@
# frozen_string_literal: true
class Projects::ErrorTrackingController < Projects::ApplicationController
before_action :check_feature_flag!
before_action :authorize_read_sentry_issue!
before_action :push_feature_flag_to_frontend
POLLING_INTERVAL = 10_000
def index
respond_to do |format|
format.html
format.json do
set_polling_interval
render_index_json
end
end
end
private
def render_index_json
service = ErrorTracking::ListIssuesService.new(project, current_user)
result = service.execute
unless result[:status] == :success
return render json: { message: result[:message] },
status: result[:http_status] || :bad_request
end
render json: {
errors: serialize_errors(result[:issues]),
external_url: service.external_url
}
end
def set_polling_interval
Gitlab::PollingInterval.set_header(response, interval: POLLING_INTERVAL)
end
def serialize_errors(errors)
ErrorTracking::ErrorSerializer
.new(project: project, user: current_user)
.represent(errors)
end
def check_feature_flag!
render_404 unless Feature.enabled?(:error_tracking, project)
end
def push_feature_flag_to_frontend
push_frontend_feature_flag(:error_tracking, current_user)
end
end

View file

@ -2,13 +2,58 @@
module ErrorTracking
class ProjectErrorTrackingSetting < ActiveRecord::Base
include ReactiveCaching
self.reactive_cache_key = ->(setting) { [setting.class.model_name.singular, setting.project_id] }
belongs_to :project
validates :api_url, length: { maximum: 255 }, public_url: true, url: { enforce_sanitization: true }
validate :validate_api_url_path
attr_encrypted :token,
mode: :per_attribute_iv,
key: Settings.attr_encrypted_db_key_base_truncated,
algorithm: 'aes-256-gcm'
after_save :clear_reactive_cache!
def sentry_client
Sentry::Client.new(api_url, token)
end
def sentry_external_url
self.class.extract_sentry_external_url(api_url)
end
def list_sentry_issues(opts = {})
with_reactive_cache('list_issues', opts.stringify_keys) do |result|
{ issues: result }
end
end
def calculate_reactive_cache(request, opts)
case request
when 'list_issues'
sentry_client.list_issues(**opts.symbolize_keys)
end
end
# http://HOST/api/0/projects/ORG/PROJECT
# ->
# http://HOST/ORG/PROJECT
def self.extract_sentry_external_url(url)
url.sub('api/0/projects/', '')
end
private
def validate_api_url_path
unless URI(api_url).path.starts_with?('/api/0/projects')
errors.add(:api_url, 'path needs to start with /api/0/projects')
end
rescue URI::InvalidURIError
end
end
end

View file

@ -200,6 +200,7 @@ class ProjectPolicy < BasePolicy
enable :read_environment
enable :read_deployment
enable :read_merge_request
enable :read_sentry_issue
end
# We define `:public_user_access` separately because there are cases in gitlab-ee

View file

@ -0,0 +1,10 @@
# frozen_string_literal: true
module ErrorTracking
class ErrorEntity < Grape::Entity
expose :id, :title, :type, :user_count, :count,
:first_seen, :last_seen, :message, :culprit,
:external_url, :project_id, :project_name, :project_slug,
:short_id, :status, :frequency
end
end

View file

@ -0,0 +1,7 @@
# frozen_string_literal: true
module ErrorTracking
class ErrorSerializer < BaseSerializer
entity ErrorEntity
end
end

View file

@ -0,0 +1,49 @@
# frozen_string_literal: true
module ErrorTracking
class ListIssuesService < ::BaseService
DEFAULT_ISSUE_STATUS = 'unresolved'
DEFAULT_LIMIT = 20
def execute
return error('not enabled') unless enabled?
return error('access denied') unless can_read?
result = project_error_tracking_setting
.list_sentry_issues(issue_status: issue_status, limit: limit)
# our results are not yet ready
unless result
return error('not ready', :no_content)
end
success(issues: result[:issues])
end
def external_url
project_error_tracking_setting&.sentry_external_url
end
private
def project_error_tracking_setting
project.error_tracking_setting
end
def issue_status
params[:issue_status] || DEFAULT_ISSUE_STATUS
end
def limit
params[:limit] || DEFAULT_LIMIT
end
def enabled?
project_error_tracking_setting&.enabled?
end
def can_read?
can?(current_user, :read_sentry_issue, project)
end
end
end

View file

@ -0,0 +1 @@
- page_title _('Errors')

View file

@ -442,6 +442,8 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do
end
end
resources :error_tracking, only: [:index], controller: :error_tracking
# Since both wiki and repository routing contains wildcard characters
# its preferable to keep it below all other project routes
draw :wiki

View file

@ -0,0 +1,14 @@
# frozen_string_literal: true
module Gitlab
module ErrorTracking
class Error
include ActiveModel::Model
attr_accessor :id, :title, :type, :user_count, :count,
:first_seen, :last_seen, :message, :culprit,
:external_url, :project_id, :project_name, :project_slug,
:short_id, :status, :frequency
end
end
end

104
lib/sentry/client.rb Normal file
View file

@ -0,0 +1,104 @@
# frozen_string_literal: true
module Sentry
class Client
Error = Class.new(StandardError)
attr_accessor :url, :token
def initialize(api_url, token)
@url = api_url
@token = token
end
def list_issues(issue_status:, limit:)
issues = get_issues(issue_status: issue_status, limit: limit)
map_to_errors(issues)
end
private
def request_params
{
headers: {
'Authorization' => "Bearer #{@token}"
},
follow_redirects: false
}
end
def get_issues(issue_status:, limit:)
resp = Gitlab::HTTP.get(
issues_api_url,
**request_params.merge(query: {
query: "is:#{issue_status}",
limit: limit
})
)
handle_response(resp)
end
def handle_response(response)
unless response.code == 200
raise Client::Error, "Sentry response error: #{response.code}"
end
response.as_json
end
def issues_api_url
issues_url = URI(@url + '/issues/')
issues_url.path.squeeze!('/')
issues_url
end
def map_to_errors(issues)
issues.map do |issue|
map_to_error(issue)
end
end
def issue_url(id)
issues_url = @url + "/issues/#{id}"
issues_url = ErrorTracking::ProjectErrorTrackingSetting.extract_sentry_external_url(issues_url)
uri = URI(issues_url)
uri.path.squeeze!('/')
uri.to_s
end
def map_to_error(issue)
id = issue.fetch('id')
project = issue.fetch('project')
count = issue.fetch('count', nil)
frequency = issue.dig('stats', '24h')
message = issue.dig('metadata', 'value')
external_url = issue_url(id)
Gitlab::ErrorTracking::Error.new(
id: id,
first_seen: issue.fetch('firstSeen', nil),
last_seen: issue.fetch('lastSeen', nil),
title: issue.fetch('title', nil),
type: issue.fetch('type', nil),
user_count: issue.fetch('userCount', nil),
count: count,
message: message,
culprit: issue.fetch('culprit', nil),
external_url: external_url,
short_id: issue.fetch('shortId', nil),
status: issue.fetch('status', nil),
frequency: frequency,
project_id: project.fetch('id'),
project_name: project.fetch('name', nil),
project_slug: project.fetch('slug', nil)
)
end
end
end

View file

@ -2959,6 +2959,9 @@ msgstr ""
msgid "Error while loading the merge request. Please try again."
msgstr ""
msgid "Errors"
msgstr ""
msgid "Estimated"
msgstr ""

View file

@ -0,0 +1,142 @@
# frozen_string_literal: true
require 'rails_helper'
describe Projects::ErrorTrackingController do
set(:project) { create(:project) }
set(:user) { create(:user) }
before do
sign_in(user)
project.add_maintainer(user)
end
describe 'GET #index' do
describe 'html' do
it 'renders index with 200 status code' do
get :index, params: project_params
expect(response).to have_gitlab_http_status(:ok)
expect(response).to render_template(:index)
end
context 'with feature flag disabled' do
before do
stub_feature_flags(error_tracking: false)
end
it 'returns 404' do
get :index, params: project_params
expect(response).to have_gitlab_http_status(:not_found)
end
end
context 'with insufficient permissions' do
before do
project.add_guest(user)
end
it 'returns 404' do
get :index, params: project_params
expect(response).to have_gitlab_http_status(:not_found)
end
end
context 'with an anonymous user' do
before do
sign_out(user)
end
it 'redirects to sign-in page' do
get :index, params: project_params
expect(response).to redirect_to(new_user_session_path)
end
end
end
describe 'format json' do
shared_examples 'no data' do
it 'returns no data' do
get :index, params: project_params(format: :json)
expect(response).to have_gitlab_http_status(:ok)
expect(response).to match_response_schema('error_tracking/index')
expect(json_response['external_url']).to be_nil
expect(json_response['errors']).to eq([])
end
end
let(:list_issues_service) { spy(:list_issues_service) }
let(:external_url) { 'http://example.com' }
before do
expect(ErrorTracking::ListIssuesService)
.to receive(:new).with(project, user)
.and_return(list_issues_service)
end
context 'service result is successful' do
before do
expect(list_issues_service).to receive(:execute)
.and_return(status: :success, issues: [error])
expect(list_issues_service).to receive(:external_url)
.and_return(external_url)
end
let(:error) { build(:error_tracking_error) }
it 'returns a list of errors' do
get :index, params: project_params(format: :json)
expect(response).to have_gitlab_http_status(:ok)
expect(response).to match_response_schema('error_tracking/index')
expect(json_response['external_url']).to eq(external_url)
expect(json_response['errors']).to eq([error].as_json)
end
end
context 'service result is erroneous' do
let(:error_message) { 'error message' }
context 'without http_status' do
before do
expect(list_issues_service).to receive(:execute)
.and_return(status: :error, message: error_message)
end
it 'returns 400 with message' do
get :index, params: project_params(format: :json)
expect(response).to have_gitlab_http_status(:bad_request)
expect(json_response['message']).to eq(error_message)
end
end
context 'with explicit http_status' do
let(:http_status) { :no_content }
before do
expect(list_issues_service).to receive(:execute)
.and_return(status: :error, message: error_message, http_status: http_status)
end
it 'returns http_status with message' do
get :index, params: project_params(format: :json)
expect(response).to have_gitlab_http_status(http_status)
expect(json_response['message']).to eq(error_message)
end
end
end
end
end
private
def project_params(opts = {})
opts.reverse_merge(namespace_id: project.namespace, project_id: project)
end
end

View file

@ -0,0 +1,24 @@
# frozen_string_literal: true
FactoryBot.define do
factory :error_tracking_error, class: Gitlab::ErrorTracking::Error do
id 'id'
title 'title'
type 'error'
user_count 1
count 2
first_seen { Time.now }
last_seen { Time.now }
message 'message'
culprit 'culprit'
external_url 'http://example.com/id'
project_id 'project1'
project_name 'project name'
project_slug 'project_name'
short_id 'ID'
status 'unresolved'
frequency []
skip_create
end
end

View file

@ -3,7 +3,7 @@
FactoryBot.define do
factory :project_error_tracking_setting, class: ErrorTracking::ProjectErrorTrackingSetting do
project
api_url 'https://gitlab.com'
api_url 'https://gitlab.com/api/0/projects/sentry-org/sentry-project'
enabled true
token 'access_token_123'
end

View file

@ -0,0 +1,21 @@
{
"type": "object",
"required" : [
"external_url",
"last_seen",
"message",
"type"
],
"properties" : {
"id": { "type": "string"},
"first_seen": { "type": "string", "format": "date-time" },
"last_seen": { "type": "string", "format": "date-time" },
"type": { "type": "string" },
"message": { "type": "string" },
"culprit": { "type": "string" },
"count": { "type": "integer"},
"external_url": { "type": "string" },
"user_count": { "type": "integer"}
},
"additionalProperties": true
}

View file

@ -0,0 +1,15 @@
{
"type": "object",
"required": [
"external_url",
"errors"
],
"properties": {
"external_url": { "type": ["string", "null"] },
"errors": {
"type": "array",
"items": { "$ref": "error.json" }
}
},
"additionalProperties": false
}

View file

@ -0,0 +1,42 @@
[{
"lastSeen": "2018-12-31T12:00:11Z",
"numComments": 0,
"userCount": 0,
"stats": {
"24h": [
[
1546437600,
0
]
]
},
"culprit": "sentry.tasks.reports.deliver_organization_user_report",
"title": "gaierror: [Errno -2] Name or service not known",
"id": "11",
"assignedTo": null,
"logger": null,
"type": "error",
"annotations": [],
"metadata": {
"type": "gaierror",
"value": "[Errno -2] Name or service not known"
},
"status": "unresolved",
"subscriptionDetails": null,
"isPublic": false,
"hasSeen": false,
"shortId": "INTERNAL-4",
"shareId": null,
"firstSeen": "2018-12-17T12:00:14Z",
"count": "21",
"permalink": "35.228.54.90/sentry/internal/issues/11/",
"level": "error",
"isSubscribed": true,
"isBookmarked": false,
"project": {
"slug": "internal",
"id": "1",
"name": "Internal"
},
"statusDetails": {}
}]

View file

@ -0,0 +1,119 @@
# frozen_string_literal: true
require 'spec_helper'
describe Sentry::Client do
let(:issue_status) { 'unresolved' }
let(:limit) { 20 }
let(:sentry_url) { 'https://sentrytest.gitlab.com/api/0/projects/sentry-org/sentry-project' }
let(:token) { 'test-token' }
let(:sample_response) do
Gitlab::Utils.deep_indifferent_access(
JSON.parse(File.read(Rails.root.join('spec/fixtures/sentry/issues_sample_response.json')))
)
end
subject(:client) { described_class.new(sentry_url, token) }
describe '#list_issues' do
subject { client.list_issues(issue_status: issue_status, limit: limit) }
before do
stub_sentry_request(sentry_url + '/issues/?limit=20&query=is:unresolved', body: sample_response)
end
it 'returns objects of type ErrorTracking::Error' do
expect(subject.length).to eq(1)
expect(subject[0]).to be_a(Gitlab::ErrorTracking::Error)
end
context 'error object created from sentry response' do
using RSpec::Parameterized::TableSyntax
where(:error_object, :sentry_response) do
:id | :id
:first_seen | :firstSeen
:last_seen | :lastSeen
:title | :title
:type | :type
:user_count | :userCount
:count | :count
:message | [:metadata, :value]
:culprit | :culprit
:short_id | :shortId
:status | :status
:frequency | [:stats, '24h']
:project_id | [:project, :id]
:project_name | [:project, :name]
:project_slug | [:project, :slug]
end
with_them do
it { expect(subject[0].public_send(error_object)).to eq(sample_response[0].dig(*sentry_response)) }
end
context 'external_url' do
it 'is constructed correctly' do
expect(subject[0].external_url).to eq('https://sentrytest.gitlab.com/sentry-org/sentry-project/issues/11')
end
end
end
context 'redirects' do
let(:redirect_to) { 'https://redirected.example.com' }
let(:other_url) { 'https://other.example.org' }
let!(:redirected_req_stub) { stub_sentry_request(other_url) }
let!(:redirect_req_stub) do
stub_sentry_request(
sentry_url + '/issues/?limit=20&query=is:unresolved',
status: 302,
headers: { location: redirect_to }
)
end
it 'does not follow redirects' do
expect { subject }.to raise_exception(Sentry::Client::Error, 'Sentry response error: 302')
expect(redirect_req_stub).to have_been_requested
expect(redirected_req_stub).not_to have_been_requested
end
end
# Sentry API returns 404 if there are extra slashes in the URL!
context 'extra slashes in URL' do
let(:sentry_url) { 'https://sentrytest.gitlab.com/api/0/projects//sentry-org/sentry-project/' }
let(:client) { described_class.new(sentry_url, token) }
let!(:valid_req_stub) do
stub_sentry_request(
'https://sentrytest.gitlab.com/api/0/projects/sentry-org/sentry-project/' \
'issues/?limit=20&query=is:unresolved'
)
end
it 'removes extra slashes in api url' do
expect(Gitlab::HTTP).to receive(:get).with(
URI('https://sentrytest.gitlab.com/api/0/projects/sentry-org/sentry-project/issues/'),
anything
).and_call_original
client.list_issues(issue_status: issue_status, limit: limit)
expect(valid_req_stub).to have_been_requested
end
end
end
private
def stub_sentry_request(url, body: {}, status: 200, headers: {})
WebMock.stub_request(:get, url)
.to_return(
status: status,
headers: { 'Content-Type' => 'application/json' }.merge(headers),
body: body.to_json
)
end
end

View file

@ -3,33 +3,106 @@
require 'spec_helper'
describe ErrorTracking::ProjectErrorTrackingSetting do
include ReactiveCachingHelpers
set(:project) { create(:project) }
subject { create(:project_error_tracking_setting, project: project) }
describe 'Associations' do
it { is_expected.to belong_to(:project) }
end
describe 'Validations' do
subject { create(:project_error_tracking_setting, project: project) }
context 'when api_url is over 255 chars' do
before do
subject.api_url = 'https://' + 'a' * 250
end
it 'fails validation' do
subject.api_url = 'https://' + 'a' * 250
expect(subject).not_to be_valid
expect(subject.errors.messages[:api_url]).to include('is too long (maximum is 255 characters)')
end
end
context 'With unsafe url' do
let(:project_error_tracking_setting) { create(:project_error_tracking_setting, project: project) }
it 'fails validation' do
project_error_tracking_setting.api_url = "https://replaceme.com/'><script>alert(document.cookie)</script>"
subject.api_url = "https://replaceme.com/'><script>alert(document.cookie)</script>"
expect(project_error_tracking_setting).not_to be_valid
expect(subject).not_to be_valid
end
end
context 'URL path' do
it 'fails validation with wrong path' do
subject.api_url = 'http://gitlab.com/project1/something'
expect(subject).not_to be_valid
expect(subject.errors.messages[:api_url]).to include('path needs to start with /api/0/projects')
end
it 'passes validation with correct path' do
subject.api_url = 'http://gitlab.com/api/0/projects/project1/something'
expect(subject).to be_valid
end
end
end
describe '#sentry_external_url' do
let(:sentry_url) { 'https://sentrytest.gitlab.com/api/0/projects/sentry-org/sentry-project' }
before do
subject.api_url = sentry_url
end
it 'returns the correct url' do
expect(subject.class).to receive(:extract_sentry_external_url).with(sentry_url).and_call_original
result = subject.sentry_external_url
expect(result).to eq('https://sentrytest.gitlab.com/sentry-org/sentry-project')
end
end
describe '#sentry_client' do
it 'returns sentry client' do
expect(subject.sentry_client).to be_a(Sentry::Client)
end
end
describe '#list_sentry_issues' do
let(:issues) { [:list, :of, :issues] }
let(:opts) do
{ issue_status: 'unresolved', limit: 10 }
end
let(:result) do
subject.list_sentry_issues(**opts)
end
context 'when cached' do
let(:sentry_client) { spy(:sentry_client) }
before do
stub_reactive_cache(subject, issues, opts)
synchronous_reactive_cache(subject)
expect(subject).to receive(:sentry_client).and_return(sentry_client)
end
it 'returns cached issues' do
expect(sentry_client).to receive(:list_issues).with(opts)
.and_return(issues)
expect(result).to eq(issues: issues)
end
end
context 'when not cached' do
it 'returns nil' do
expect(subject).not_to receive(:sentry_client)
expect(result).to be_nil
end
end
end

View file

@ -24,7 +24,7 @@ describe ProjectPolicy do
download_code fork_project create_project_snippet update_issue
admin_issue admin_label admin_list read_commit_status read_build
read_container_image read_pipeline read_environment read_deployment
read_merge_request download_wiki_code
read_merge_request download_wiki_code read_sentry_issue
]
end

View file

@ -0,0 +1,87 @@
# frozen_string_literal: true
require 'spec_helper'
describe ErrorTracking::ListIssuesService do
set(:user) { create(:user) }
set(:project) { create(:project) }
let(:sentry_url) { 'https://sentrytest.gitlab.com/api/0/projects/sentry-org/sentry-project' }
let(:token) { 'test-token' }
let(:result) { subject.execute }
let(:error_tracking_setting) do
create(:project_error_tracking_setting, api_url: sentry_url, token: token, project: project)
end
subject { described_class.new(project, user) }
before do
expect(project).to receive(:error_tracking_setting).at_least(:once).and_return(error_tracking_setting)
project.add_reporter(user)
end
describe '#execute' do
context 'with authorized user' do
context 'when list_sentry_issues returns issues' do
let(:issues) { [:list, :of, :issues] }
before do
expect(error_tracking_setting)
.to receive(:list_sentry_issues).and_return(issues: issues)
end
it 'returns the issues' do
expect(result).to eq(status: :success, issues: issues)
end
end
context 'when list_sentry_issues returns nil' do
before do
expect(error_tracking_setting)
.to receive(:list_sentry_issues).and_return(nil)
end
it 'result is not ready' do
expect(result).to eq(
status: :error, http_status: :no_content, message: 'not ready')
end
end
end
context 'with unauthorized user' do
let(:unauthorized_user) { create(:user) }
subject { described_class.new(project, unauthorized_user) }
it 'returns error' do
result = subject.execute
expect(result).to include(status: :error, message: 'access denied')
end
end
context 'with error tracking disabled' do
before do
error_tracking_setting.enabled = false
end
it 'raises error' do
result = subject.execute
expect(result).to include(status: :error, message: 'not enabled')
end
end
end
describe '#sentry_external_url' do
let(:external_url) { 'https://sentrytest.gitlab.com/sentry-org/sentry-project' }
it 'calls ErrorTracking::ProjectErrorTrackingSetting' do
expect(error_tracking_setting).to receive(:sentry_external_url).and_call_original
subject.external_url
end
end
end

View file

@ -17,7 +17,7 @@ describe Projects::Operations::UpdateService do
{
error_tracking_setting_attributes: {
enabled: false,
api_url: 'http://url',
api_url: 'http://gitlab.com/api/0/projects/org/project',
token: 'token'
}
}
@ -32,7 +32,7 @@ describe Projects::Operations::UpdateService do
project.reload
expect(project.error_tracking_setting).not_to be_enabled
expect(project.error_tracking_setting.api_url).to eq('http://url')
expect(project.error_tracking_setting.api_url).to eq('http://gitlab.com/api/0/projects/org/project')
expect(project.error_tracking_setting.token).to eq('token')
end
end
@ -42,7 +42,7 @@ describe Projects::Operations::UpdateService do
{
error_tracking_setting_attributes: {
enabled: true,
api_url: 'http://url',
api_url: 'http://gitlab.com/api/0/projects/org/project',
token: 'token'
}
}
@ -52,7 +52,7 @@ describe Projects::Operations::UpdateService do
expect(result[:status]).to eq(:success)
expect(project.error_tracking_setting).to be_enabled
expect(project.error_tracking_setting.api_url).to eq('http://url')
expect(project.error_tracking_setting.api_url).to eq('http://gitlab.com/api/0/projects/org/project')
expect(project.error_tracking_setting.token).to eq('token')
end
end