diff --git a/.rubocop_todo/gitlab/bounded_contexts.yml b/.rubocop_todo/gitlab/bounded_contexts.yml index 6cc30892c49e5c5b9aec4ba30386e06156ede8fd..86a6fcb0ccd32968ea4cb7957baaf1857dc3a66f 100644 --- a/.rubocop_todo/gitlab/bounded_contexts.yml +++ b/.rubocop_todo/gitlab/bounded_contexts.yml @@ -1757,7 +1757,6 @@ Gitlab/BoundedContexts: - 'app/services/wiki_pages/base_service.rb' - 'app/services/wiki_pages/create_service.rb' - 'app/services/wiki_pages/destroy_service.rb' - - 'app/services/wiki_pages/event_create_service.rb' - 'app/services/wiki_pages/update_service.rb' - 'app/services/x509_certificate_revoke_service.rb' - 'app/uploaders/attachment_uploader.rb' diff --git a/.rubocop_todo/rspec/context_wording.yml b/.rubocop_todo/rspec/context_wording.yml index 79272e0e3e46ca6eb0f7eaca6093d55d1314c0ba..54b7fb383f1c86e52dc10009be2d9066f71ffbd3 100644 --- a/.rubocop_todo/rspec/context_wording.yml +++ b/.rubocop_todo/rspec/context_wording.yml @@ -2471,7 +2471,6 @@ RSpec/ContextWording: - 'spec/services/web_hook_service_spec.rb' - 'spec/services/web_hooks/log_execution_service_spec.rb' - 'spec/services/wiki_pages/base_service_spec.rb' - - 'spec/services/wiki_pages/event_create_service_spec.rb' - 'spec/services/wikis/create_attachment_service_spec.rb' - 'spec/services/work_items/create_service_spec.rb' - 'spec/services/work_items/parent_links/create_service_spec.rb' diff --git a/.rubocop_todo/rspec/named_subject.yml b/.rubocop_todo/rspec/named_subject.yml index 7f28b82543ee6d668ffba034935966594b2f67c2..679b6e3e77757e1e88b311b5d12a9fa0d1f6dbf6 100644 --- a/.rubocop_todo/rspec/named_subject.yml +++ b/.rubocop_todo/rspec/named_subject.yml @@ -3047,7 +3047,6 @@ RSpec/NamedSubject: - 'spec/services/web_hooks/destroy_service_spec.rb' - 'spec/services/webauthn/destroy_service_spec.rb' - 'spec/services/wiki_pages/base_service_spec.rb' - - 'spec/services/wiki_pages/event_create_service_spec.rb' - 'spec/services/work_items/callbacks/award_emoji_spec.rb' - 'spec/services/work_items/export_csv_service_spec.rb' - 'spec/services/work_items/import_csv_service_spec.rb' diff --git a/app/services/event_create_service.rb b/app/services/event_create_service.rb index 8ca1f6bac8ebee75230e04d33cb82e9f2252259a..eb64449183d6b87eb91891d00fc37f6306ac99ee 100644 --- a/app/services/event_create_service.rb +++ b/app/services/event_create_service.rb @@ -180,12 +180,6 @@ def wiki_event(wiki_page_meta, author, action, fingerprint) Gitlab::UsageDataCounters::HLLRedisCounter.track_event(:git_write_action, values: author.id) - track_internal_event("performed_wiki_action", - project: wiki_page_meta.project, - user: author, - additional_properties: { label: action.to_s } - ) - duplicate = Event.for_wiki_meta(wiki_page_meta).for_fingerprint(fingerprint).first return duplicate if duplicate.present? diff --git a/app/services/git/wiki_push_service.rb b/app/services/git/wiki_push_service.rb index dab961a491c18b4e190e2c4d95917d80ade0379f..abc359653516808b82f8fe0d9a12fb8ebce19c62 100644 --- a/app/services/git/wiki_push_service.rb +++ b/app/services/git/wiki_push_service.rb @@ -2,6 +2,8 @@ module Git class WikiPushService < ::BaseService + include Gitlab::InternalEventsTracking + # Maximum number of change events we will process on any single push MAX_CHANGES = 100 @@ -27,8 +29,7 @@ def process_changes push_changes.take(MAX_CHANGES).each do |change| # rubocop:disable CodeReuse/ActiveRecord next unless change.page.present? - response = create_event_for(change) - log_error(response.message) if response.error? + create_event_for(change) end end @@ -43,18 +44,16 @@ def raw_changes(change) end def create_event_for(change) - event_service.execute( - change.last_known_slug, - change.page, - change.event_action, - change.sha + wiki_page_meta = change.page.find_or_create_meta + track_internal_event('performed_wiki_action', + project: wiki_page_meta.project, + user: @current_user, + label: change.event_action.to_s, + meta: wiki_page_meta, + fingerprint: change.sha ) end - def event_service - @event_service ||= WikiPages::EventCreateService.new(current_user) - end - def on_default_branch?(change) wiki.default_branch == ::Gitlab::Git.branch_name(change[:ref]) end diff --git a/app/services/wiki_pages/base_service.rb b/app/services/wiki_pages/base_service.rb index 6e5196c662f5ca8d841ab2d4215fa73329ff080a..f014517b78d38f58db73e1e59c73285b4fda6d1c 100644 --- a/app/services/wiki_pages/base_service.rb +++ b/app/services/wiki_pages/base_service.rb @@ -9,12 +9,14 @@ module WikiPages class BaseService < ::BaseContainerService private + include Gitlab::InternalEventsTracking + def execute_hooks(page) page_data = payload(page) container.execute_hooks(page_data, :wiki_page_hooks) container.execute_integrations(page_data, :wiki_page_hooks) increment_usage(page) - create_wiki_event(page) + track_wiki_event(page) end # Passed to web-hooks, and send to external consumers. @@ -57,12 +59,18 @@ def track_event(page, event_name) ) end - def create_wiki_event(page) - response = WikiPages::EventCreateService - .new(current_user) - .execute(slug_for_page(page), page, event_action, fingerprint(page)) + def track_wiki_event(page) + return unless current_user - log_error(response.message) if response.error? + fingerprint = fingerprint(page) + wiki_page_meta = page.find_or_create_meta + track_internal_event('performed_wiki_action', + project: wiki_page_meta.project, + user: current_user, + label: event_action.to_s, + meta: wiki_page_meta, + fingerprint: fingerprint + ) end def slug_for_page(page) diff --git a/app/services/wiki_pages/event_create_service.rb b/app/services/wiki_pages/event_create_service.rb deleted file mode 100644 index 1f613bec00edf48cd9c1d567e9c141abe938241c..0000000000000000000000000000000000000000 --- a/app/services/wiki_pages/event_create_service.rb +++ /dev/null @@ -1,26 +0,0 @@ -# frozen_string_literal: true - -module WikiPages - class EventCreateService - # @param [User] author The event author - def initialize(author) - raise ArgumentError, 'author must not be nil' unless author - - @author = author - end - - def execute(slug, page, action, event_fingerprint) - wiki_page_meta = WikiPage::Meta.find_or_create(slug, page) - - event = ::EventCreateService.new.wiki_event(wiki_page_meta, author, action, event_fingerprint) - - ServiceResponse.success(payload: { event: event }) - rescue ::EventCreateService::IllegalActionError, ::ActiveRecord::ActiveRecordError => e - ServiceResponse.error(message: e.message, payload: { error: e }) - end - - private - - attr_reader :author - end -end diff --git a/config/events/performed_wiki_action.yml b/config/events/performed_wiki_action.yml index 22912eb3d7d3a45af23e1a4d038ed1cb1f06365d..1ce19aef2dfaa73f79425b2476d3d0855892ed01 100644 --- a/config/events/performed_wiki_action.yml +++ b/config/events/performed_wiki_action.yml @@ -16,3 +16,10 @@ tiers: - free - premium - ultimate +extra_trackers: + - tracking_class: Gitlab::Tracking::ContributionAnalyticsTracking + protected_properties: + meta: + description: Wikipage meta object + fingerprint: + description: SHA of the commit diff --git a/lib/gitlab/tracking/contribution_analytics_tracking.rb b/lib/gitlab/tracking/contribution_analytics_tracking.rb new file mode 100644 index 0000000000000000000000000000000000000000..26119a53d57b20f59bab35e81c4b5fe3fdc583d1 --- /dev/null +++ b/lib/gitlab/tracking/contribution_analytics_tracking.rb @@ -0,0 +1,32 @@ +# frozen_string_literal: true + +module Gitlab + module Tracking + class ContributionAnalyticsTracking + def self.track_event(event_name, **kwargs) + author = kwargs[:user] + action = kwargs[:label] + meta = kwargs[:meta] + fingerprint = kwargs[:fingerprint] + + return unless author && action && meta && fingerprint + + ::EventCreateService.new.wiki_event(meta, author, action.to_sym, fingerprint) + + rescue ::EventCreateService::IllegalActionError, ::ActiveRecord::ActiveRecordError => e + error_details = { + message: 'Failed to track wiki contribution analytics event', + error: e.message, + error_class: e.class.name, + event_name: event_name, + user_id: author&.id, + action: action + } + + error_details[:project_id] = meta.project_id if meta.respond_to?(:project_id) + + Gitlab::AppLogger.error(error_details) + end + end + end +end diff --git a/spec/lib/gitlab/tracking/contribution_analytics_tracking_spec.rb b/spec/lib/gitlab/tracking/contribution_analytics_tracking_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..5d1dbf252c0593ccd7e3a9d5cc47110c0f8e1ddd --- /dev/null +++ b/spec/lib/gitlab/tracking/contribution_analytics_tracking_spec.rb @@ -0,0 +1,83 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Tracking::ContributionAnalyticsTracking, feature_category: :service_ping do + let(:user) { build_stubbed(:user) } + let(:meta) { { foo: 'bar' } } + let(:fingerprint) { 'abc123' } + let(:label) { :create } + let(:event_name) { 'performed_wiki_action' } + + describe '.track_event' do + it 'delegates to EventCreateService#wiki_event' do + service = instance_double(EventCreateService) + expect(EventCreateService).to receive(:new).and_return(service) + expect(service).to receive(:wiki_event).with(meta, user, label, fingerprint) + + described_class.track_event(event_name, user: user, label: label, meta: meta, fingerprint: fingerprint) + end + + it 'logs structured error if EventCreateService raises IllegalActionError' do + service = instance_double(EventCreateService) + allow(EventCreateService).to receive(:new).and_return(service) + allow(service).to receive(:wiki_event).and_raise(EventCreateService::IllegalActionError, 'illegal') + + expect(Gitlab::AppLogger).to receive(:error).with( + hash_including( + message: 'Failed to track wiki contribution analytics event', + error: 'illegal', + error_class: 'EventCreateService::IllegalActionError', + event_name: event_name, + user_id: user.id, + action: label + ) + ) + + described_class.track_event(event_name, user: user, label: label, meta: meta, fingerprint: fingerprint) + end + + it 'logs structured error if EventCreateService raises ActiveRecord::ActiveRecordError' do + service = instance_double(EventCreateService) + allow(EventCreateService).to receive(:new).and_return(service) + allow(service).to receive(:wiki_event).and_raise(ActiveRecord::ActiveRecordError, 'db error') + + expect(Gitlab::AppLogger).to receive(:error).with( + hash_including( + message: 'Failed to track wiki contribution analytics event', + error: 'db error', + error_class: 'ActiveRecord::ActiveRecordError', + event_name: event_name + ) + ) + + described_class.track_event(event_name, user: user, label: label, meta: meta, fingerprint: fingerprint) + end + + context 'when required parameters are missing' do + it 'does not call EventCreateService when user is nil' do + expect(EventCreateService).not_to receive(:new) + + described_class.track_event(event_name, user: nil, label: label, meta: meta, fingerprint: fingerprint) + end + + it 'does not call EventCreateService when label is nil' do + expect(EventCreateService).not_to receive(:new) + + described_class.track_event(event_name, user: user, label: nil, meta: meta, fingerprint: fingerprint) + end + + it 'does not call EventCreateService when meta is nil' do + expect(EventCreateService).not_to receive(:new) + + described_class.track_event(event_name, user: user, label: label, meta: nil, fingerprint: fingerprint) + end + + it 'does not call EventCreateService when fingerprint is nil' do + expect(EventCreateService).not_to receive(:new) + + described_class.track_event(event_name, user: user, label: label, meta: meta, fingerprint: nil) + end + end + end +end diff --git a/spec/services/event_create_service_spec.rb b/spec/services/event_create_service_spec.rb index d2da4f93b66c7ea15a75f56f9ca14d822cb58365..21a6165f1f8d81870ed0d72c4eaef4d8a84a78ca 100644 --- a/spec/services/event_create_service_spec.rb +++ b/spec/services/event_create_service_spec.rb @@ -304,13 +304,6 @@ def create_event expect(duplicate).to eq(event) end - it_behaves_like 'internal event tracking' do - let(:event) { 'performed_wiki_action' } - let(:category) { described_class.name } - let(:project) { meta.project } - let(:additional_properties) { { label: action.to_s } } - end - it_behaves_like "it records a git write event" end diff --git a/spec/services/wiki_pages/event_create_service_spec.rb b/spec/services/wiki_pages/event_create_service_spec.rb deleted file mode 100644 index c32631cf027ae020c831e3892f655c0dc0abd38c..0000000000000000000000000000000000000000 --- a/spec/services/wiki_pages/event_create_service_spec.rb +++ /dev/null @@ -1,70 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe WikiPages::EventCreateService, feature_category: :wiki do - let_it_be(:project) { create(:project) } - let_it_be(:user) { create(:user) } - - subject { described_class.new(user) } - - describe '#execute' do - let_it_be(:page) { create(:wiki_page, project: project) } - - let(:slug) { generate(:sluggified_title) } - let(:action) { :created } - let(:fingerprint) { page.sha } - let(:response) { subject.execute(slug, page, action, fingerprint) } - - context 'the user is nil' do - subject { described_class.new(nil) } - - it 'raises an error on construction' do - expect { subject }.to raise_error ArgumentError - end - end - - context 'the action is illegal' do - let(:action) { :illegal_action } - - it 'returns an error' do - expect(response).to be_error - end - - it 'does not create an event' do - expect { response }.not_to change { Event.count } - end - end - - it 'returns a successful response' do - expect(response).to be_success - end - - context 'the action is a deletion' do - let(:action) { :destroyed } - - it 'does not synchronize the wiki metadata timestamps with the git commit' do - expect_next_instance_of(WikiPage::Meta) do |instance| - expect(instance).not_to receive(:synch_times_with_page) - end - - response - end - end - - it 'creates a wiki page event' do - expect { response }.to change { Event.count }.by(1) - end - - it 'returns an event in the payload' do - expect(response.payload).to include(event: have_attributes(author: user, wiki_page?: true, action: 'created')) - end - - it 'records the slug for the page' do - response - meta = WikiPage::Meta.find_or_create(page.slug, page) - - expect(meta.slugs.pluck(:slug)).to include(slug) - end - end -end diff --git a/spec/support/shared_examples/services/git/wiki_push_service_shared_examples.rb b/spec/support/shared_examples/services/git/wiki_push_service_shared_examples.rb index 117cab070a89596d92240c3ef65538158cfa740d..39f85c3aae28f3087e145de012ce3b16930e4fa4 100644 --- a/spec/support/shared_examples/services/git/wiki_push_service_shared_examples.rb +++ b/spec/support/shared_examples/services/git/wiki_push_service_shared_examples.rb @@ -250,15 +250,15 @@ def run_service count = 3 count.times { write_new_page } message = 'something went very very wrong' - allow_next_instance_of(WikiPages::EventCreateService, current_user) do |service| - allow(service).to receive(:execute) - .with(String, WikiPage, Symbol, String) - .and_return(ServiceResponse.error(message: message)) + allow_next_instance_of(::EventCreateService) do |service| + allow(service).to receive(:wiki_event) + .with(WikiPage::Meta, current_user, Symbol, String) + .and_raise(::ActiveRecord::ActiveRecordError.new(message)) end service = create_service(base_sha) - expect(service).to receive(:log_error).exactly(count).times.with(message) + expect(Gitlab::AppLogger).to receive(:error).exactly(count).times service.execute end @@ -281,6 +281,16 @@ def run_service end end end + + it_behaves_like 'internal event tracking' do + let(:event) { 'performed_wiki_action' } + let(:category) { described_class.name } + let(:project) { wiki.respond_to?(:project) ? wiki.project : nil } + let(:user) { current_user } + let(:additional_properties) { { label: 'created' } } + + subject { process_changes { write_new_page } } + end end describe '#perform_housekeeping', :clean_gitlab_redis_shared_state do