Merge branch 'dz-error-tracking-store-collector' into 'master'

Add support for another et collector endpoint

See merge request gitlab-org/gitlab!69660

app/services/error_tracking/collect_error_service.rb

@@ -9,9 +9,9 @@ module ErrorTracking
       error = project.error_tracking_errors.report_error(
         name: exception['type'], # Example: ActionView::MissingTemplate
         description: exception['value'], # Example: Missing template posts/show in...
-        actor: event['transaction'], # Example: PostsController#show
+        actor: actor, # Example: PostsController#show
         platform: event['platform'], # Example: ruby
-        timestamp: event['timestamp']
+        timestamp: timestamp
       )
 
       # The payload field contains all the data on error including stacktrace in jsonb.
@@ -20,7 +20,7 @@ module ErrorTracking
         environment: event['environment'],
         description: exception['value'],
         level: event['level'],
-        occurred_at: event['timestamp'],
+        occurred_at: timestamp,
         payload: event
       )
     end
@@ -34,5 +34,29 @@ module ErrorTracking
     def exception
       event['exception']['values'].first
     end
+
+    def actor
+      return event['transaction'] if event['transaction']
+
+      # Some SDK do not have transaction attribute.
+      # So we build it by combining function name and module name from
+      # the last item in stacktrace.
+      last_line = exception.dig('stacktrace', 'frames').last
+
+      "#{last_line['function']}(#{last_line['module']})"
+    end
+
+    def timestamp
+      return @timestamp if @timestamp
+
+      @timestamp = (event['timestamp'] || Time.zone.now)
+
+      # Some SDK send timestamp in numeric format like '1630945472.13'.
+      if @timestamp.to_s =~ /\A\d+(\.\d+)?\z/
+        @timestamp = Time.zone.at(@timestamp.to_f)
+      end
+
+      @timestamp
+    end
   end
 end

app/validators/json_schemas/error_tracking_event_payload.json

@@ -2,48 +2,15 @@
   "description": "Error tracking event payload",
   "type": "object",
   "required": [],
-  "modules": {
-    "type": "object"
-  },
   "properties": {
-    "event_id": {
-      "type": "string"
-    },
-    "level": {
-      "type": "string"
-    },
-    "timestamp": {
-      "type": "string"
-    },
-    "release": {
-      "type": "string"
-    },
     "environment": {
       "type": "string"
     },
-    "server_name": {
-      "type": "string"
-    },
-    "message": {
-      "type": "string"
-    },
-    "contexts": {
-      "type": "object"
-    },
     "platform": {
       "type": "string"
     },
     "sdk": {
-      "type": "object",
-      "required": [],
-      "properties": {
-        "name": {
-          "type": "string"
-        },
-        "version": {
-          "type": "string"
-        }
-      }
+      "type": "object"
     },
     "exception": {
       "type": "object",

lib/api/error_tracking_collector.rb

@@ -8,6 +8,8 @@ module API
     feature_category :error_tracking
 
     content_type :envelope, 'application/x-sentry-envelope'
+    content_type :json, 'application/json'
+    content_type :txt, 'text/plain'
     default_format :envelope
 
     before do
@@ -33,17 +35,24 @@ module API
       end
 
       def active_client_key?
+        public_key = extract_public_key
+
+        find_client_key(public_key)
+      end
+
+      def extract_public_key
+        # Some SDK send public_key as a param. In this case we don't need to parse headers.
+        return params[:sentry_key] if params[:sentry_key].present?
+
         begin
-          public_key = ::ErrorTracking::Collector::SentryAuthParser.parse(request)[:public_key]
+          ::ErrorTracking::Collector::SentryAuthParser.parse(request)[:public_key]
         rescue StandardError
           bad_request!('Failed to parse sentry request')
         end
-
-        find_client_key(public_key)
       end
     end
 
-    desc 'Submit error tracking event to the project' do
+    desc 'Submit error tracking event to the project as envelope' do
       detail 'This feature was introduced in GitLab 14.1.'
     end
     params do
@@ -89,5 +98,38 @@ module API
       # it is safe only for submission of new events.
       no_content!
     end
+
+    desc 'Submit error tracking event to the project' do
+      detail 'This feature was introduced in GitLab 14.1.'
+    end
+    params do
+      requires :id, type: String, desc: 'The ID of a project'
+    end
+    post 'error_tracking/collector/api/:id/store' do
+      # There is a reason why we have such uncommon path.
+      # We depend on a client side error tracking software which
+      # modifies URL for its own reasons.
+      #
+      # When we give user a URL like this
+      #   HOST/api/v4/error_tracking/collector/123
+      #
+      # Then error tracking software will convert it like this:
+      #   HOST/api/v4/error_tracking/collector/api/123/store/
+
+      begin
+        parsed_body = Gitlab::Json.parse(request.body.read)
+      rescue StandardError
+        bad_request!('Failed to parse sentry request')
+      end
+
+      ::ErrorTracking::CollectErrorService
+        .new(project, nil, event: parsed_body)
+        .execute
+
+      # Collector should never return any information back.
+      # Because DSN and public key are designed for public use,
+      # it is safe only for submission of new events.
+      no_content!
+    end
   end
 end

spec/requests/api/error_tracking_collector_spec.rb

@@ -7,15 +7,6 @@ RSpec.describe API::ErrorTrackingCollector do
   let_it_be(:setting) { create(:project_error_tracking_setting, :integrated, project: project) }
   let_it_be(:client_key) { create(:error_tracking_client_key, project: project) }
 
-  describe "POST /error_tracking/collector/api/:id/envelope" do
-    let_it_be(:raw_event) { fixture_file('error_tracking/event.txt') }
-    let_it_be(:url) { "/error_tracking/collector/api/#{project.id}/envelope" }
-
-    let(:params) { raw_event }
-    let(:headers) { { 'X-Sentry-Auth' => "Sentry sentry_key=#{client_key.public_key}" } }
-
-    subject { post api(url), params: params, headers: headers }
-
   RSpec.shared_examples 'not found' do
     it 'reponds with 404' do
       subject
@@ -32,6 +23,23 @@ RSpec.describe API::ErrorTrackingCollector do
     end
   end
 
+  RSpec.shared_examples 'successful request' do
+    it 'writes to the database and returns no content' do
+      expect { subject }.to change { ErrorTracking::ErrorEvent.count }.by(1)
+
+      expect(response).to have_gitlab_http_status(:no_content)
+    end
+  end
+
+  describe "POST /error_tracking/collector/api/:id/envelope" do
+    let_it_be(:raw_event) { fixture_file('error_tracking/event.txt') }
+    let_it_be(:url) { "/error_tracking/collector/api/#{project.id}/envelope" }
+
+    let(:params) { raw_event }
+    let(:headers) { { 'X-Sentry-Auth' => "Sentry sentry_key=#{client_key.public_key}" } }
+
+    subject { post api(url), params: params, headers: headers }
+
     context 'error tracking feature is disabled' do
       before do
         setting.update!(enabled: false)
@@ -88,10 +96,53 @@ RSpec.describe API::ErrorTrackingCollector do
       end
     end
 
-    it 'writes to the database and returns no content' do
-      expect { subject }.to change { ErrorTracking::ErrorEvent.count }.by(1)
+    it_behaves_like 'successful request'
+  end
 
-      expect(response).to have_gitlab_http_status(:no_content)
+  describe "POST /error_tracking/collector/api/:id/store" do
+    let_it_be(:raw_event) { fixture_file('error_tracking/parsed_event.json') }
+    let_it_be(:url) { "/error_tracking/collector/api/#{project.id}/store" }
+
+    let(:params) { raw_event }
+    let(:headers) { { 'X-Sentry-Auth' => "Sentry sentry_key=#{client_key.public_key}" } }
+
+    subject { post api(url), params: params, headers: headers }
+
+    it_behaves_like 'successful request'
+
+    context 'empty headers' do
+      let(:headers) { {} }
+
+      it_behaves_like 'bad request'
+    end
+
+    context 'empty body' do
+      let(:params) { '' }
+
+      it_behaves_like 'bad request'
+    end
+
+    context 'sentry_key as param and empty headers' do
+      let(:url) { "/error_tracking/collector/api/#{project.id}/store?sentry_key=#{sentry_key}" }
+      let(:headers) { {} }
+
+      context 'key is wrong' do
+        let(:sentry_key) { 'glet_1fedb514e17f4b958435093deb02048c' }
+
+        it_behaves_like 'not found'
+      end
+
+      context 'key is empty' do
+        let(:sentry_key) { '' }
+
+        it_behaves_like 'bad request'
+      end
+
+      context 'key is correct' do
+        let(:sentry_key) { client_key.public_key }
+
+        it_behaves_like 'successful request'
+      end
     end
   end
 end

spec/services/error_tracking/collect_error_service_spec.rb

@@ -40,5 +40,29 @@ RSpec.describe ErrorTracking::CollectErrorService do
       expect(event.environment).to eq 'development'
       expect(event.payload).to eq parsed_event
     end
+
+    context 'unusual payload' do
+      let(:modified_event) { parsed_event }
+
+      context 'missing transaction' do
+        it 'builds actor from stacktrace' do
+          modified_event.delete('transaction')
+
+          event = described_class.new(project, nil, event: modified_event).execute
+
+          expect(event.error.actor).to eq 'find()'
+        end
+      end
+
+      context 'timestamp is numeric' do
+        it 'parses timestamp' do
+          modified_event['timestamp'] = '1631015580.50'
+
+          event = described_class.new(project, nil, event: modified_event).execute
+
+          expect(event.occurred_at).to eq '2021-09-07T11:53:00.5'
+        end
+      end
+    end
   end
 end