Commit bc7a706a authored by Kushal Pandya's avatar Kushal Pandya

Merge branch '31007-limit-activity-events' into 'master'

Aggregate push events when there are too many

See merge request gitlab-org/gitlab!18239
parents d41ef313 d74f511a
......@@ -290,7 +290,8 @@ module ApplicationSettingsHelper
:snowplow_cookie_domain,
:snowplow_enabled,
:snowplow_site_id,
:push_event_hooks_limit
:push_event_hooks_limit,
:push_event_activities_limit
]
end
......
......@@ -217,6 +217,9 @@ class ApplicationSetting < ApplicationRecord
validates :push_event_hooks_limit,
numericality: { greater_than_or_equal_to: 0 }
validates :push_event_activities_limit,
numericality: { greater_than_or_equal_to: 0 }
SUPPORTED_KEY_TYPES.each do |type|
validates :"#{type}_key_restriction", presence: true, key_restriction: { type: type }
end
......
......@@ -83,6 +83,7 @@ module ApplicationSettingImplementation
project_export_enabled: true,
protected_ci_variables: false,
push_event_hooks_limit: 3,
push_event_activities_limit: 3,
raw_blob_request_limit: 300,
recaptcha_enabled: false,
login_recaptcha_protection_enabled: false,
......
......@@ -26,6 +26,8 @@ class PushEvent < Event
delegate :commit_count, to: :push_event_payload
alias_method :commits_count, :commit_count
delegate :ref_count, to: :push_event_payload
# Returns events of pushes that either pushed to an existing ref or created a
# new one.
def self.created_or_pushed
......
# frozen_string_literal: true
class BulkPushEventPayloadService
def initialize(event, push_data)
@event = event
@push_data = push_data
end
def execute
@event.build_push_event_payload(
action: @push_data[:action],
commit_count: 0,
ref_count: @push_data[:ref_count],
ref_type: @push_data[:ref_type]
)
@event.push_event_payload.tap(&:save!)
end
end
......@@ -73,15 +73,27 @@ class EventCreateService
end
def push(project, current_user, push_data)
create_push_event(PushEventPayloadService, project, current_user, push_data)
end
def bulk_push(project, current_user, push_data)
create_push_event(BulkPushEventPayloadService, project, current_user, push_data)
end
private
def create_record_event(record, current_user, status)
create_event(record.resource_parent, current_user, status, target_id: record.id, target_type: record.class.name)
end
def create_push_event(service_class, project, current_user, push_data)
# We're using an explicit transaction here so that any errors that may occur
# when creating push payload data will result in the event creation being
# rolled back as well.
event = Event.transaction do
new_event = create_event(project, current_user, Event::PUSHED)
PushEventPayloadService
.new(new_event, push_data)
.execute
service_class.new(new_event, push_data).execute
new_event
end
......@@ -92,12 +104,6 @@ class EventCreateService
Users::ActivityService.new(current_user, 'push').execute
end
private
def create_record_event(record, current_user, status)
create_event(record.resource_parent, current_user, status, target_id: record.id, target_type: record.class.name)
end
def create_event(resource_parent, current_user, status, attributes = {})
attributes.reverse_merge!(
action: status,
......
......@@ -48,6 +48,8 @@ module Git
# Push events in the activity feed only show information for the
# last commit.
def create_events
return unless params.fetch(:create_push_event, true)
EventCreateService.new.push(project, current_user, event_push_data)
end
......
......@@ -16,8 +16,8 @@ module Git
def process_changes_by_action(ref_type, changes)
changes_by_action = group_changes_by_action(changes)
changes_by_action.each do |_, changes|
process_changes(ref_type, changes, execute_project_hooks: execute_project_hooks?(changes)) if changes.any?
changes_by_action.each do |action, changes|
process_changes(ref_type, action, changes, execute_project_hooks: execute_project_hooks?(changes)) if changes.any?
end
end
......@@ -38,9 +38,11 @@ module Git
(changes.size <= Gitlab::CurrentSettings.push_event_hooks_limit) || Feature.enabled?(:git_push_execute_all_project_hooks, project)
end
def process_changes(ref_type, changes, execute_project_hooks:)
def process_changes(ref_type, action, changes, execute_project_hooks:)
push_service_class = push_service_class_for(ref_type)
create_bulk_push_event = changes.size > Gitlab::CurrentSettings.push_event_activities_limit
changes.each do |change|
push_service_class.new(
project,
......@@ -48,9 +50,20 @@ module Git
change: change,
push_options: params[:push_options],
create_pipelines: change[:index] < PIPELINE_PROCESS_LIMIT || Feature.enabled?(:git_push_create_all_pipelines, project),
execute_project_hooks: execute_project_hooks
execute_project_hooks: execute_project_hooks,
create_push_event: !create_bulk_push_event
).execute
end
create_bulk_push_event(ref_type, action, changes) if create_bulk_push_event
end
def create_bulk_push_event(ref_type, action, changes)
EventCreateService.new.bulk_push(
project,
current_user,
Gitlab::DataBuilder::Push.build_bulk(action: action, ref_type: ref_type, changes: changes)
)
end
def push_service_class_for(ref_type)
......
......@@ -25,5 +25,10 @@
= f.number_field :push_event_hooks_limit, class: 'form-control'
.form-text.text-muted
= _("Number of changes (branches or tags) in a single push to determine whether webhooks and services will be fired or not. Webhooks and services won't be submitted if it surpasses that value.")
.form-group
= f.label :push_event_activities_limit, class: 'label-bold'
= f.number_field :push_event_activities_limit, class: 'form-control'
.form-text.text-muted
= _('Number of changes (branches or tags) in a single push to determine whether individual push events or bulk push event will be created. Bulk push event will be created if it surpasses that value.')
= f.submit 'Save changes', class: "btn btn-success"
......@@ -6,7 +6,9 @@
.event-title.d-flex.flex-wrap
= inline_event_icon(event)
%span.event-type.d-inline-block.append-right-4.pushed #{event.action_name} #{event.ref_type}
- many_refs = event.ref_count.to_i > 1
%span.event-type.d-inline-block.append-right-4.pushed= many_refs ? "#{event.action_name} #{event.ref_count} #{event.ref_type.pluralize}" : "#{event.action_name} #{event.ref_type}"
- unless many_refs
%span.append-right-4
- commits_link = project_commits_path(project, event.ref_name)
- should_link = event.tag? ? project.repository.tag_exists?(event.ref_name) : project.repository.branch_exists?(event.ref_name)
......
---
title: Aggregate push events when there are too many
merge_request: 18239
author:
type: changed
# frozen_string_literal: true
class AddPushEventActivitiesLimitToApplicationSettings < ActiveRecord::Migration[5.2]
include Gitlab::Database::MigrationHelpers
DOWNTIME = false
disable_ddl_transaction!
def up
add_column_with_default(:application_settings, :push_event_activities_limit, :integer, default: 3)
end
def down
remove_column(:application_settings, :push_event_activities_limit)
end
end
# frozen_string_literal: true
class AddRefCountToPushEventPayloads < ActiveRecord::Migration[5.2]
include Gitlab::Database::MigrationHelpers
DOWNTIME = false
def change
add_column :push_event_payloads, :ref_count, :integer
end
end
......@@ -339,6 +339,7 @@ ActiveRecord::Schema.define(version: 2019_10_16_072826) do
t.integer "throttle_incident_management_notification_period_in_seconds", default: 3600
t.integer "throttle_incident_management_notification_per_period", default: 3600
t.integer "push_event_hooks_limit", default: 3, null: false
t.integer "push_event_activities_limit", default: 3, null: false
t.index ["custom_project_templates_group_id"], name: "index_application_settings_on_custom_project_templates_group_id"
t.index ["file_template_project_id"], name: "index_application_settings_on_file_template_project_id"
t.index ["instance_administration_project_id"], name: "index_applicationsettings_on_instance_administration_project_id"
......@@ -3158,6 +3159,7 @@ ActiveRecord::Schema.define(version: 2019_10_16_072826) do
t.binary "commit_to"
t.text "ref"
t.string "commit_title", limit: 70
t.integer "ref_count"
t.index ["event_id"], name: "index_push_event_payloads_on_event_id", unique: true
end
......
......@@ -290,6 +290,7 @@ are listed in the descriptions of the relevant settings.
| `protected_ci_variables` | boolean | no | Environment variables are protected by default. |
| `pseudonymizer_enabled` | boolean | no | **(PREMIUM)** When enabled, GitLab will run a background job that will produce pseudonymized CSVs of the GitLab database that will be uploaded to your configured object storage directory.
| `push_event_hooks_limit` | integer | no | Number of changes (branches or tags) in a single push to determine whether webhooks and services will be fired or not. Webhooks and services won't be submitted if it surpasses that value. |
| `push_event_activities_limit` | integer | no | Number of changes (branches or tags) in a single push to determine whether individual push events or bulk push events will be created. [Bulk push events will be created](../user/admin_area/settings/push_event_activities_limit.md) if it surpasses that value. |
| `recaptcha_enabled` | boolean | no | (**If enabled, requires:** `recaptcha_private_key` and `recaptcha_site_key`) Enable reCAPTCHA. |
| `recaptcha_private_key` | string | required by: `recaptcha_enabled` | Private key for reCAPTCHA. |
| `recaptcha_site_key` | string | required by: `recaptcha_enabled` | Site key for reCAPTCHA. |
......
......@@ -22,6 +22,7 @@ include:
- [Custom templates repository](instance_template_repository.md) **(PREMIUM)**
- [Protected paths](protected_paths.md) **(CORE ONLY)**
- [Help messages for the `/help` page and the login page](help_page.md)
- [Push event activities limit and bulk push events](push_event_activities_limit.md)
NOTE: **Note:**
You can change the [first day of the week](../../profile/preferences.md) for the entire GitLab instance
......
---
type: reference
---
# Push event activities limit and bulk push events
> [Introduced](https://gitlab.com/gitlab-org/gitlab/issues/31007) in GitLab 12.4.
This allows you to set the number of changes (branches or tags) in a single push
to determine whether individual push events or bulk push event will be created.
Bulk push events will be created if it surpasses that value.
For example, if 4 branches are pushed and the limit is currently set to 3,
you'll see the following in the activity feed:
![Bulk push event](img/bulk_push_event_v12_4.png)
With this feature, when a single push includes a lot of changes (e.g. 1,000
branches), only 1 bulk push event will be created instead of creating 1,000 push
events. This helps in maintaining good system performance and preventing spam on
the activity feed.
This setting can be modified in **Admin Area > Settings > Network > Performance Optimization**.
This can also be configured via the [Application settings API](../../../api/settings.md#list-of-settings-that-can-be-accessed-via-api-calls)
as `push_event_activities_limit`. The default value is 3, but it can be greater
than or equal 0.
![Push event activities limit](img/push_event_activities_limit_v12_4.png)
......@@ -933,8 +933,8 @@ module API
end
class PushEventPayload < Grape::Entity
expose :commit_count, :action, :ref_type, :commit_from, :commit_to
expose :ref, :commit_title
expose :commit_count, :action, :ref_type, :commit_from, :commit_to, :ref,
:commit_title, :ref_count
end
class Event < Grape::Entity
......
......@@ -102,6 +102,7 @@ module API
optional :project_export_enabled, type: Boolean, desc: 'Enable project export'
optional :prometheus_metrics_enabled, type: Boolean, desc: 'Enable Prometheus metrics'
optional :push_event_hooks_limit, type: Integer, desc: "Number of changes (branches or tags) in a single push to determine whether webhooks and services will be fired or not. Webhooks and services won't be submitted if it surpasses that value."
optional :push_event_activities_limit, type: Integer, desc: 'Number of changes (branches or tags) in a single push to determine whether individual push events or bulk push event will be created. Bulk push event will be created if it surpasses that value.'
optional :recaptcha_enabled, type: Boolean, desc: 'Helps prevent bots from creating accounts'
given recaptcha_enabled: ->(val) { val } do
requires :recaptcha_site_key, type: String, desc: 'Generate site key at http://www.google.com/recaptcha'
......
......@@ -107,6 +107,14 @@ module Gitlab
}
end
def build_bulk(action:, ref_type:, changes:)
{
action: action,
ref_count: changes.count,
ref_type: ref_type
}
end
# This method provides a sample data generated with
# existing project and commits to test webhooks
def build_sample(project, user)
......
......@@ -11199,6 +11199,9 @@ msgstr ""
msgid "Number of LOCs per commit"
msgstr ""
msgid "Number of changes (branches or tags) in a single push to determine whether individual push events or bulk push event will be created. Bulk push event will be created if it surpasses that value."
msgstr ""
msgid "Number of changes (branches or tags) in a single push to determine whether webhooks and services will be fired or not. Webhooks and services won't be submitted if it surpasses that value."
msgstr ""
......
......@@ -90,4 +90,12 @@ describe Gitlab::DataBuilder::Push do
.not_to raise_error
end
end
describe '.build_bulk' do
subject do
described_class.build_bulk(action: :created, ref_type: :branch, changes: [double, double])
end
it { is_expected.to eq(action: :created, ref_count: 2, ref_type: :branch) }
end
end
......@@ -47,6 +47,7 @@ PushEventPayload:
- commit_to
- ref
- commit_title
- ref_count
Note:
- id
- note
......
......@@ -60,6 +60,10 @@ describe ApplicationSetting do
it { is_expected.not_to allow_value('three').for(:push_event_hooks_limit) }
it { is_expected.not_to allow_value(nil).for(:push_event_hooks_limit) }
it { is_expected.to allow_value(3).for(:push_event_activities_limit) }
it { is_expected.not_to allow_value('three').for(:push_event_activities_limit) }
it { is_expected.not_to allow_value(nil).for(:push_event_activities_limit) }
context "when user accepted let's encrypt terms of service" do
before do
setting.update(lets_encrypt_terms_of_service_accepted: true)
......
......@@ -122,6 +122,7 @@ describe API::Events do
expect(payload_hash['action']).to eq(payload.action)
expect(payload_hash['ref_type']).to eq(payload.ref_type)
expect(payload_hash['commit_to']).to eq(payload.commit_to)
expect(payload_hash['ref_count']).to eq(payload.ref_count)
end
end
......
......@@ -73,7 +73,8 @@ describe API::Settings, 'Settings' do
local_markdown_version: 3,
allow_local_requests_from_web_hooks_and_services: true,
allow_local_requests_from_system_hooks: false,
push_event_hooks_limit: 2
push_event_hooks_limit: 2,
push_event_activities_limit: 2
}
expect(response).to have_gitlab_http_status(200)
......@@ -104,6 +105,7 @@ describe API::Settings, 'Settings' do
expect(json_response['allow_local_requests_from_web_hooks_and_services']).to eq(true)
expect(json_response['allow_local_requests_from_system_hooks']).to eq(false)
expect(json_response['push_event_hooks_limit']).to eq(2)
expect(json_response['push_event_activities_limit']).to eq(2)
end
end
......
# frozen_string_literal: true
require 'spec_helper'
describe BulkPushEventPayloadService do
let(:event) { create(:push_event) }
let(:push_data) do
{
action: :created,
ref_count: 4,
ref_type: :branch
}
end
subject { described_class.new(event, push_data) }
it 'creates a PushEventPayload' do
push_event_payload = subject.execute
expect(push_event_payload).to be_persisted
expect(push_event_payload.action).to eq(push_data[:action].to_s)
expect(push_event_payload.commit_count).to eq(0)
expect(push_event_payload.ref_count).to eq(push_data[:ref_count])
expect(push_event_payload.ref_type).to eq(push_data[:ref_type].to_s)
end
end
......@@ -113,40 +113,21 @@ describe EventCreateService do
end
end
describe '#push', :clean_gitlab_redis_shared_state do
let(:project) { create(:project) }
let(:user) { create(:user) }
let(:push_data) do
{
commits: [
{
id: '1cf19a015df3523caf0a1f9d40c98a267d6a2fc2',
message: 'This is a commit'
}
],
before: '0000000000000000000000000000000000000000',
after: '1cf19a015df3523caf0a1f9d40c98a267d6a2fc2',
total_commits_count: 1,
ref: 'refs/heads/my-branch'
}
end
shared_examples_for 'service for creating a push event' do |service_class|
it 'creates a new event' do
expect { service.push(project, user, push_data) }.to change { Event.count }
expect { subject }.to change { Event.count }
end
it 'creates the push event payload' do
expect(PushEventPayloadService).to receive(:new)
expect(service_class).to receive(:new)
.with(an_instance_of(PushEvent), push_data)
.and_call_original
service.push(project, user, push_data)
subject
end
it 'updates user last activity' do
expect { service.push(project, user, push_data) }
.to change { user.last_activity_on }.to(Date.today)
expect { subject }.to change { user.last_activity_on }.to(Date.today)
end
it 'caches the last push event for the user' do
......@@ -154,7 +135,7 @@ describe EventCreateService do
.to receive(:cache_last_push_event)
.with(an_instance_of(PushEvent))
service.push(project, user, push_data)
subject
end
it 'does not create any event data when an error is raised' do
......@@ -163,17 +144,56 @@ describe EventCreateService do
allow(payload_service).to receive(:execute)
.and_raise(RuntimeError)
allow(PushEventPayloadService).to receive(:new)
allow(service_class).to receive(:new)
.and_return(payload_service)
expect { service.push(project, user, push_data) }
.to raise_error(RuntimeError)
expect { subject }.to raise_error(RuntimeError)
expect(Event.count).to eq(0)
expect(PushEventPayload.count).to eq(0)
end
end
describe '#push', :clean_gitlab_redis_shared_state do
let(:project) { create(:project) }
let(:user) { create(:user) }
let(:push_data) do
{
commits: [
{
id: '1cf19a015df3523caf0a1f9d40c98a267d6a2fc2',
message: 'This is a commit'
}
],
before: '0000000000000000000000000000000000000000',
after: '1cf19a015df3523caf0a1f9d40c98a267d6a2fc2',
total_commits_count: 1,
ref: 'refs/heads/my-branch'
}
end
subject { service.push(project, user, push_data) }
it_behaves_like 'service for creating a push event', PushEventPayloadService
end
describe '#bulk_push', :clean_gitlab_redis_shared_state do
let(:project) { create(:project) }
let(:user) { create(:user) }
let(:push_data) do
{
action: :created,
ref_count: 4,
ref_type: :branch
}
end
subject { service.bulk_push(project, user, push_data) }
it_behaves_like 'service for creating a push event', BulkPushEventPayloadService
end
describe 'Project' do
let(:user) { create :user }
let(:project) { create(:project) }
......
......@@ -12,8 +12,8 @@ describe Git::BaseHooksService do
let(:newrev) { "8a2a6eb295bb170b34c24c76c49ed0e9b2eaf34b" } # gitlab-test: git rev-parse refs/tags/v1.1.0
let(:ref) { 'refs/tags/v1.1.0' }
describe '#execute_project_hooks' do
class TestService < described_class
let(:test_service) do
Class.new(described_class) do
def hook_name
:push_hooks
end
......@@ -22,8 +22,9 @@ describe Git::BaseHooksService do
[]
end
end
end
let(:project) { create(:project, :repository) }
subject { test_service.new(project, user, params) }
let(:params) do
{
......@@ -35,9 +36,30 @@ describe Git::BaseHooksService do
}
end
subject { TestService.new(project, user, params) }
describe 'push event' do
it 'creates push event' do
expect_next_instance_of(EventCreateService) do |service|
expect(service).to receive(:push)
end
subject.execute
end
context 'create_push_event is set to false' do
before do
params[:create_push_event] = false
end
it 'does not create push event' do
expect(EventCreateService).not_to receive(:new)
subject.execute
end
end
end
context '#execute_hooks' do
describe 'project hooks and services' do
context 'hooks' do
before do
expect(project).to receive(:has_active_hooks?).and_return(active)
end
......@@ -65,7 +87,7 @@ describe Git::BaseHooksService do
end
end
context '#execute_services' do
context 'services' do
before do
expect(project).to receive(:has_active_services?).and_return(active)
end
......
......@@ -13,6 +13,12 @@ describe Git::ProcessRefChangesService do
let(:service) { double(execute: true) }
let(:git_changes) { double(branch_changes: [], tag_changes: []) }
def multiple_changes(change, count)
Array.new(count).map.with_index do |n, index|
{ index: index, oldrev: change[:oldrev], newrev: change[:newrev], ref: "#{change[:ref]}#{n}" }
end
end
let(:changes) do
[
{ index: 0, oldrev: Gitlab::Git::BLANK_SHA, newrev: '789012', ref: "#{ref_prefix}/create" },
......@@ -28,7 +34,7 @@ describe Git::ProcessRefChangesService do
it "calls #{push_service_class}" do
expect(push_service_class)
.to receive(:new)
.with(project, project.owner, hash_including(execute_project_hooks: true))
.with(project, project.owner, hash_including(execute_project_hooks: true, create_push_event: true))
.exactly(changes.count).times
.and_return(service)
......@@ -36,12 +42,6 @@ describe Git::ProcessRefChangesService do
end
context 'changes exceed push_event_hooks_limit' do
def multiple_changes(change, count)
Array.new(count).map.with_index do |n, index|
{ index: index, oldrev: change[:oldrev], newrev: change[:newrev], ref: "#{change[:ref]}#{n}" }
end
end
let(:push_event_hooks_limit) { 3 }
let(:changes) do
......@@ -88,6 +88,40 @@ describe Git::ProcessRefChangesService do
end
end
context 'changes exceed push_event_activities_limit per action' do
let(:push_event_activities_limit) { 3 }
let(:changes) do
[
{ oldrev: Gitlab::Git::BLANK_SHA, newrev: '789012', ref: "#{ref_prefix}/create" },
{ oldrev: '123456', newrev: '789012', ref: "#{ref_prefix}/update" },
{ oldrev: '123456', newrev: Gitlab::Git::BLANK_SHA, ref: "#{ref_prefix}/delete" }
].map do |change|
multiple_changes(change, push_event_activities_limit + 1)
end.flatten
end
before do
stub_application_setting(push_event_activities_limit: push_event_activities_limit)
end
it "calls #{push_service_class} with create_push_event set to false" do
expect(push_service_class)
.to receive(:new)
.with(project, project.owner, hash_including(create_push_event: false))
.exactly(changes.count).times
.and_return(service)
subject.execute
end
it 'creates events per action' do
allow(push_service_class).to receive(:new).and_return(service)
expect { subject.execute }.to change { Event.count }.by(3)
end
end
context 'pipeline creation' do
context 'with valid .gitlab-ci.yml' do
before do
......
......@@ -28,6 +28,23 @@ describe 'events/event/_push.html.haml' do
expect(rendered).not_to have_link(event.ref_name)
end
end
context 'ref_count is more than 1' do
let(:payload) do
build_stubbed(
:push_event_payload,
event: event,
ref_count: 4,
ref_type: :branch
)
end
it 'includes the count in the text' do
render partial: 'events/event/push', locals: { event: event }
expect(rendered).to include('4 branches')
end
end
end
context 'with a tag' do
......@@ -53,5 +70,22 @@ describe 'events/event/_push.html.haml' do
expect(rendered).not_to have_link(event.ref_name)
end
end
context 'ref_count is more than 1' do
let(:payload) do
build_stubbed(
:push_event_payload,
event: event,
ref_count: 4,
ref_type: :tag
)
end
it 'includes the count in the text' do
render partial: 'events/event/push', locals: { event: event }
expect(rendered).to include('4 tags')
end
end
end
end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment