Merge branch...

Merge branch '34068-sort-by-first-seen-last-seen-and-frequency-in-sentry-error-list-in-gitlab' into 'master'

Sort Sentry error list by frequency, last seen, and first seen

See merge request gitlab-org/gitlab!20101

app/controllers/projects/error_tracking_controller.rb

@@ -111,7 +111,7 @@ class Projects::ErrorTrackingController < Projects::ApplicationController
   end
 
   def list_issues_params
-    params.permit(:search_term)
+    params.permit([:search_term, :sort])
   end
 
   def list_projects_params

app/models/error_tracking/project_error_tracking_setting.rb

@@ -5,6 +5,7 @@ module ErrorTracking
     include Gitlab::Utils::StrongMemoize
     include ReactiveCaching
 
+    SENTRY_API_ERROR_TYPE_BAD_REQUEST = 'bad_request_for_sentry_api'
     SENTRY_API_ERROR_TYPE_MISSING_KEYS = 'missing_keys_in_sentry_response'
     SENTRY_API_ERROR_TYPE_NON_20X_RESPONSE = 'non_20x_response_from_sentry'
     SENTRY_API_ERROR_INVALID_SIZE = 'invalid_size_of_sentry_response'
@@ -119,6 +120,8 @@ module ErrorTracking
       { error: e.message, error_type: SENTRY_API_ERROR_TYPE_MISSING_KEYS }
     rescue Sentry::Client::ResponseInvalidSizeError => e
       { error: e.message, error_type: SENTRY_API_ERROR_INVALID_SIZE }
+    rescue Sentry::Client::BadRequestError => e
+      { error: e.message, error_type: SENTRY_API_ERROR_TYPE_BAD_REQUEST }
     end
 
     # http://HOST/api/0/projects/ORG/PROJECT

app/services/error_tracking/list_issues_service.rb

@@ -4,6 +4,7 @@ module ErrorTracking
   class ListIssuesService < ErrorTracking::BaseService
     DEFAULT_ISSUE_STATUS = 'unresolved'
     DEFAULT_LIMIT = 20
+    DEFAULT_SORT = 'last_seen'
 
     def execute
       return error('Error Tracking is not enabled') unless enabled?
@@ -12,7 +13,8 @@ module ErrorTracking
       result = project_error_tracking_setting.list_sentry_issues(
         issue_status: issue_status,
         limit: limit,
-        search_term: search_term
+        search_term: search_term,
+        sort: sort
       )
 
       # our results are not yet ready
@@ -33,10 +35,6 @@ module ErrorTracking
 
     private
 
-    def fetch
-      project_error_tracking_setting.list_sentry_issues(issue_status: issue_status, limit: limit)
-    end
-
     def parse_response(response)
       { issues: response[:issues] }
     end
@@ -60,5 +58,9 @@ module ErrorTracking
     def can_read?
       can?(current_user, :read_sentry_issue, project)
     end
+
+    def sort
+      params[:sort] || DEFAULT_SORT
+    end
   end
 end

changelogs/unreleased/34068-sort-by-first-seen-last-seen-and-frequency-in-sentry-error-list-in-.yml

+---
+title: Add sort param to error tracking issue index
+merge_request: 20101
+author:
+type: changed

lib/sentry/client.rb

@@ -5,6 +5,14 @@ module Sentry
     Error = Class.new(StandardError)
     MissingKeysError = Class.new(StandardError)
     ResponseInvalidSizeError = Class.new(StandardError)
+    BadRequestError = Class.new(StandardError)
+
+    SENTRY_API_SORT_VALUE_MAP = {
+      # <accepted_by_client> => <accepted_by_sentry_api>
+      'frequency' => 'freq',
+      'first_seen' => 'new',
+      'last_seen' => nil
+    }.freeze
 
     attr_accessor :url, :token
 
@@ -25,12 +33,8 @@ module Sentry
       map_to_event(latest_event)
     end
 
-    def list_issues(issue_status:, limit:, search_term: '')
-      issues = get_issues(
-        issue_status: issue_status,
-        limit: limit,
-        search_term: search_term
-      )
+    def list_issues(**keyword_args)
+      issues = get_issues(keyword_args)
 
       validate_size(issues)
 
@@ -52,14 +56,14 @@ module Sentry
     def validate_size(issues)
       return if Gitlab::Utils::DeepSize.new(issues).valid?
 
-      raise Client::ResponseInvalidSizeError, "Sentry API response is too big. Limit is #{Gitlab::Utils::DeepSize.human_default_max_size}."
+      raise ResponseInvalidSizeError, "Sentry API response is too big. Limit is #{Gitlab::Utils::DeepSize.human_default_max_size}."
     end
 
     def handle_mapping_exceptions(&block)
       yield
     rescue KeyError => e
       Gitlab::Sentry.track_acceptable_exception(e)
-      raise Client::MissingKeysError, "Sentry API response is missing keys. #{e.message}"
+      raise MissingKeysError, "Sentry API response is missing keys. #{e.message}"
     end
 
     def request_params
@@ -78,13 +82,25 @@ module Sentry
       handle_response(response)
     end
 
-    def get_issues(issue_status:, limit:, search_term: '')
-      query = "is:#{issue_status} #{search_term}".strip
+    def get_issues(**keyword_args)
+      http_get(
+        issues_api_url,
+        query: list_issue_sentry_query(keyword_args)
+      )
+    end
+
+    def list_issue_sentry_query(issue_status:, limit:, sort: nil, search_term: '')
+      unless SENTRY_API_SORT_VALUE_MAP.key?(sort)
+        raise BadRequestError, 'Invalid value for sort param'
+      end
+
+      query_params = {
+        query: "is:#{issue_status} #{search_term}".strip,
+        limit: limit,
+        sort: SENTRY_API_SORT_VALUE_MAP[sort]
+      }
 
-      http_get(issues_api_url, query: {
-        query: query,
-        limit: limit
-      })
+      query_params.compact
     end
 
     def get_issue(issue_id:)

spec/controllers/projects/error_tracking_controller_spec.rb

@@ -48,20 +48,17 @@ describe Projects::ErrorTrackingController do
     describe 'format json' do
       let(:list_issues_service) { spy(:list_issues_service) }
       let(:external_url) { 'http://example.com' }
-      let(:search_term) do
-        ActionController::Parameters.new(
-          search_term: 'something'
-        ).permit!
-      end
 
       context 'no data' do
-        let(:search_term) do
+        let(:params) { project_params(format: :json) }
+
+        let(:permitted_params) do
           ActionController::Parameters.new({}).permit!
         end
 
         before do
           expect(ErrorTracking::ListIssuesService)
-            .to receive(:new).with(project, user, search_term)
+            .to receive(:new).with(project, user, permitted_params)
             .and_return(list_issues_service)
 
           expect(list_issues_service).to receive(:execute)
@@ -75,10 +72,16 @@ describe Projects::ErrorTrackingController do
         end
       end
 
-      context 'with a search_term param' do
+      context 'with a search_term and sort params' do
+        let(:params) { project_params(format: :json, search_term: 'something', sort: 'last_seen') }
+
+        let(:permitted_params) do
+          ActionController::Parameters.new(search_term: 'something', sort: 'last_seen').permit!
+        end
+
         before do
           expect(ErrorTracking::ListIssuesService)
-            .to receive(:new).with(project, user, search_term)
+            .to receive(:new).with(project, user, permitted_params)
             .and_return(list_issues_service)
         end
 
@@ -93,7 +96,7 @@ describe Projects::ErrorTrackingController do
           let(:error) { build(:error_tracking_error) }
 
           it 'returns a list of errors' do
-            get :index, params: project_params(format: :json, search_term: 'something')
+            get :index, params: params
 
             expect(response).to have_gitlab_http_status(:ok)
             expect(response).to match_response_schema('error_tracking/index')
@@ -103,7 +106,7 @@ describe Projects::ErrorTrackingController do
         end
       end
 
-      context 'without a search_term param' do
+      context 'without params' do
         before do
           expect(ErrorTracking::ListIssuesService)
             .to receive(:new).with(project, user, {})

spec/lib/sentry/client_spec.rb

@@ -5,6 +5,12 @@ require 'spec_helper'
 describe Sentry::Client do
   let(:sentry_url) { 'https://sentrytest.gitlab.com/api/0/projects/sentry-org/sentry-project' }
   let(:token) { 'test-token' }
+  let(:default_httparty_options) do
+    {
+      follow_redirects: false,
+      headers: { "Authorization" => "Bearer test-token" }
+    }
+  end
 
   let(:issues_sample_response) do
     Gitlab::Utils.deep_indifferent_access(
@@ -94,7 +100,7 @@ describe Sentry::Client do
 
     let!(:sentry_api_request) { stub_sentry_request(sentry_request_url, body: sentry_api_response) }
 
-    subject { client.list_issues(issue_status: issue_status, limit: limit, search_term: search_term) }
+    subject { client.list_issues(issue_status: issue_status, limit: limit, search_term: search_term, sort: 'last_seen') }
 
     it_behaves_like 'calls sentry api'
 
@@ -165,6 +171,35 @@ describe Sentry::Client do
       end
     end
 
+    context 'requests with sort parameter in sentry api' do
+      let(:sentry_request_url) do
+        'https://sentrytest.gitlab.com/api/0/projects/sentry-org/sentry-project/' \
+          'issues/?limit=20&query=is:unresolved&sort=freq'
+      end
+      let!(:sentry_api_request) { stub_sentry_request(sentry_request_url, body: sentry_api_response) }
+
+      subject { client.list_issues(issue_status: issue_status, limit: limit, sort: 'frequency') }
+
+      it 'calls the sentry api with sort params' do
+        expect(Gitlab::HTTP).to receive(:get).with(
+          URI("#{sentry_url}/issues/"),
+          default_httparty_options.merge(query: { limit: 20, query: "is:unresolved", sort: "freq" })
+        ).and_call_original
+
+        subject
+
+        expect(sentry_api_request).to have_been_requested
+      end
+    end
+
+    context 'with invalid sort params' do
+      subject { client.list_issues(issue_status: issue_status, limit: limit, sort: 'fish') }
+
+      it 'throws an error' do
+        expect { subject }.to raise_error(Sentry::Client::BadRequestError, 'Invalid value for sort param')
+      end
+    end
+
     context 'Older sentry versions where keys are not present' do
       let(:sentry_api_response) do
         issues_sample_response[0...1].map do |issue|

spec/services/error_tracking/list_issues_service_spec.rb

@@ -5,12 +5,13 @@ require 'spec_helper'
 describe ErrorTracking::ListIssuesService do
   set(:user) { create(:user) }
   set(:project) { create(:project) }
-  let(:params) { { search_term: 'something' } }
+  let(:params) { { search_term: 'something', sort: 'last_seen' } }
   let(:list_sentry_issues_args) do
     {
       issue_status: 'unresolved',
       limit: 20,
-      search_term: params[:search_term]
+      search_term: params[:search_term],
+      sort: params[:sort]
     }
   end