diff --git a/.rubocop_todo/lint/unused_method_argument.yml b/.rubocop_todo/lint/unused_method_argument.yml index 474483200100b6d40a64db155ac5ab9e08c5e413..9abfd548f057aafed4e63809a3cee34bc9ca5d88 100644 --- a/.rubocop_todo/lint/unused_method_argument.yml +++ b/.rubocop_todo/lint/unused_method_argument.yml @@ -354,7 +354,6 @@ Lint/UnusedMethodArgument: - 'lib/bulk_imports/clients/graphql.rb' - 'lib/bulk_imports/projects/pipelines/snippets_repository_pipeline.rb' - 'lib/declarative_enum.rb' - - 'lib/error_tracking/sentry_client.rb' - 'lib/gitlab/analytics/cycle_analytics/stage_events/stage_event.rb' - 'lib/gitlab/api_authentication/token_resolver.rb' - 'lib/gitlab/app_text_logger.rb' diff --git a/app/models/error_tracking/project_error_tracking_setting.rb b/app/models/error_tracking/project_error_tracking_setting.rb index 318538be6455ceecc2ecda46673ed4a5612a578f..abe883d6f1bd0d68dc0d9f96c79e357a97746b4c 100644 --- a/app/models/error_tracking/project_error_tracking_setting.rb +++ b/app/models/error_tracking/project_error_tracking_setting.rb @@ -23,7 +23,7 @@ class ProjectErrorTrackingSetting < ApplicationRecord self.reactive_cache_key = ->(setting) { [setting.class.model_name.singular, setting.project_id] } self.reactive_cache_work_type = :external_dependency - self.reactive_cache_hard_limit = ErrorTracking::SentryClient::RESPONSE_SIZE_LIMIT + self.reactive_cache_hard_limit = ErrorTracking::SentryClient::RESPONSE_MEMORY_SIZE_LIMIT self.table_name = 'project_error_tracking_settings' diff --git a/lib/error_tracking/sentry_client.rb b/lib/error_tracking/sentry_client.rb index dea60fff04cdfd28c316a3c39d14837849056179..ec3231d75a5706c78e6e129de8d36dc9935c07b4 100644 --- a/lib/error_tracking/sentry_client.rb +++ b/lib/error_tracking/sentry_client.rb @@ -14,6 +14,15 @@ class SentryClient ResponseInvalidSizeError = Class.new(StandardError) RESPONSE_SIZE_LIMIT = 1.megabyte + private_constant :RESPONSE_SIZE_LIMIT + + # The bytes size of a JSON payload is different from what DeepSize + # calculates which is Ruby's object size. + # + # This factor accounts for the difference. + # + # See https://gitlab.com/gitlab-org/gitlab/-/issues/393029#note_1289914133 + RESPONSE_MEMORY_SIZE_LIMIT = RESPONSE_SIZE_LIMIT * 5 attr_accessor :url, :token @@ -25,10 +34,19 @@ def initialize(api_url, token) private def validate_size(response) - return if Gitlab::Utils::DeepSize.new(response, max_size: RESPONSE_SIZE_LIMIT).valid? + bytesize = response.body.bytesize + + if bytesize > RESPONSE_SIZE_LIMIT + limit = ActiveSupport::NumberHelper.number_to_human_size(RESPONSE_SIZE_LIMIT) + message = "Sentry API response is too big. Limit is #{limit}. Got #{bytesize} bytes." + raise ResponseInvalidSizeError, message + end + + parsed = response.parsed_response + return if Gitlab::Utils::DeepSize.new(parsed, max_size: RESPONSE_MEMORY_SIZE_LIMIT).valid? - limit = ActiveSupport::NumberHelper.number_to_human_size(RESPONSE_SIZE_LIMIT) - message = "Sentry API response is too big. Limit is #{limit}." + limit = ActiveSupport::NumberHelper.number_to_human_size(RESPONSE_MEMORY_SIZE_LIMIT) + message = "Sentry API response memory footprint is too big. Limit is #{limit}." raise ResponseInvalidSizeError, message end @@ -36,7 +54,7 @@ def api_urls @api_urls ||= SentryClient::ApiUrls.new(@url) end - def handle_mapping_exceptions(&block) + def handle_mapping_exceptions yield rescue KeyError => e Gitlab::ErrorTracking.track_exception(e) @@ -98,7 +116,7 @@ def handle_request_exceptions def handle_response(response) raise_error "Sentry response status code: #{response.code}" unless response.code.between?(200, 204) - validate_size(response.parsed_response) + validate_size(response) { body: response.parsed_response, headers: response.headers } end diff --git a/spec/lib/error_tracking/sentry_client/event_spec.rb b/spec/lib/error_tracking/sentry_client/event_spec.rb index e7a9db771b67e0a54b132bd20d266c70ca338ed4..a515575446f45ac9264bc9dddafd64aa94c2ce87 100644 --- a/spec/lib/error_tracking/sentry_client/event_spec.rb +++ b/spec/lib/error_tracking/sentry_client/event_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe ErrorTracking::SentryClient do +RSpec.describe ErrorTracking::SentryClient, feature_category: :error_tracking do include SentryClientHelpers let(:sentry_url) { 'https://sentrytest.gitlab.com/api/0/projects/sentry-org/sentry-project' } diff --git a/spec/lib/error_tracking/sentry_client/issue_link_spec.rb b/spec/lib/error_tracking/sentry_client/issue_link_spec.rb index 75e7ac8304e080e17d792478d72582830b75db93..fe954f50bfb4973ce80fd95afb38ffb5cf5f7854 100644 --- a/spec/lib/error_tracking/sentry_client/issue_link_spec.rb +++ b/spec/lib/error_tracking/sentry_client/issue_link_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe ErrorTracking::SentryClient::IssueLink do +RSpec.describe ErrorTracking::SentryClient::IssueLink, feature_category: :error_tracking do include SentryClientHelpers let_it_be(:sentry_url) { 'https://sentrytest.gitlab.com/api/0/projects/sentry-org/sentry-project' } diff --git a/spec/lib/error_tracking/sentry_client/issue_spec.rb b/spec/lib/error_tracking/sentry_client/issue_spec.rb index eaa3c7ee8bcb90ddd59bf48afb98a6001b96b5f6..83c6751254d2be7fd7e2cb5406f019cd45f4a554 100644 --- a/spec/lib/error_tracking/sentry_client/issue_spec.rb +++ b/spec/lib/error_tracking/sentry_client/issue_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe ErrorTracking::SentryClient::Issue do +RSpec.describe ErrorTracking::SentryClient::Issue, feature_category: :error_tracking do include SentryClientHelpers let(:token) { 'test-token' } diff --git a/spec/lib/error_tracking/sentry_client/projects_spec.rb b/spec/lib/error_tracking/sentry_client/projects_spec.rb index 52f8cdc915e7442ddb33b8703fd4892b4749d119..17a0377691bf923b8b6b02e0282c4266fb8a65c1 100644 --- a/spec/lib/error_tracking/sentry_client/projects_spec.rb +++ b/spec/lib/error_tracking/sentry_client/projects_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe ErrorTracking::SentryClient::Projects do +RSpec.describe ErrorTracking::SentryClient::Projects, feature_category: :error_tracking do include SentryClientHelpers let(:sentry_url) { 'https://sentrytest.gitlab.com/api/0/projects/sentry-org/sentry-project' } diff --git a/spec/lib/error_tracking/sentry_client/repo_spec.rb b/spec/lib/error_tracking/sentry_client/repo_spec.rb index 445a8e35f8e2b7b14ff43f272667b8c7aed55ee7..a635231aaf36aecd12fc152d77d20e8d06c6be73 100644 --- a/spec/lib/error_tracking/sentry_client/repo_spec.rb +++ b/spec/lib/error_tracking/sentry_client/repo_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe ErrorTracking::SentryClient::Repo do +RSpec.describe ErrorTracking::SentryClient::Repo, feature_category: :error_tracking do include SentryClientHelpers let(:sentry_url) { 'https://sentrytest.gitlab.com/api/0/projects/sentry-org/sentry-project' } diff --git a/spec/support/shared_examples/lib/sentry/client_shared_examples.rb b/spec/support/shared_examples/lib/sentry/client_shared_examples.rb index 842801708d07e1be3fe10851375382614be5b083..4325d5adaa266b6f8c769bca1accef4164a33ea1 100644 --- a/spec/support/shared_examples/lib/sentry/client_shared_examples.rb +++ b/spec/support/shared_examples/lib/sentry/client_shared_examples.rb @@ -80,19 +80,45 @@ # - sentry_api_response # - sentry_url, token RSpec.shared_examples 'Sentry API response size limit' do - let(:invalid_deep_size) { instance_double(Gitlab::Utils::DeepSize, valid?: false) } + let(:response_body) { Gitlab::Json.generate(sentry_api_response) } - before do - allow(Gitlab::Utils::DeepSize) - .to receive(:new) - .with(sentry_api_response, any_args) - .and_return(invalid_deep_size) + context 'when response body is within limit' do + before do + allow(Gitlab::Utils::DeepSize).to receive(:new).and_call_original + end + + it 'checks parsed response' do + subject + + expect(Gitlab::Utils::DeepSize).to have_received(:new) + end end - it 'raises an exception when response is too large' do - expect { subject }.to raise_error( - ErrorTracking::SentryClient::ResponseInvalidSizeError, - 'Sentry API response is too big. Limit is 1 MiB.' - ) + context 'when response body is too large' do + before do + stub_const('ErrorTracking::SentryClient::RESPONSE_SIZE_LIMIT', response_body.bytesize - 1) + stub_const('ErrorTracking::SentryClient::RESPONSE_MEMORY_SIZE_LIMIT', 1) + end + + it 'raises an exception' do + expect { subject }.to raise_error( + ErrorTracking::SentryClient::ResponseInvalidSizeError, + /Sentry API response is too big\. Limit is \d+(\.\d+)? \w+\. Got \d+ bytes\./ + ) + end + end + + context 'when resulting memory size of the parsed response is too large' do + before do + stub_const('ErrorTracking::SentryClient::RESPONSE_SIZE_LIMIT', response_body.bytesize + 1) + stub_const('ErrorTracking::SentryClient::RESPONSE_MEMORY_SIZE_LIMIT', 1) + end + + it 'raises an exception' do + expect { subject }.to raise_error( + ErrorTracking::SentryClient::ResponseInvalidSizeError, + /Sentry API response memory footprint is too big. Limit is \d+(\.\d+)? \w+\./ + ) + end end end