diff --git a/app/assets/javascripts/boards/stores/actions.js b/app/assets/javascripts/boards/stores/actions.js index b52a9a71ae8..0f1b72146c9 100644 --- a/app/assets/javascripts/boards/stores/actions.js +++ b/app/assets/javascripts/boards/stores/actions.js @@ -173,8 +173,9 @@ export default { addList: ({ commit, dispatch, getters }, list) => { commit(types.RECEIVE_ADD_LIST_SUCCESS, updateListPosition(list)); + dispatch('fetchItemsForList', { - listId: getters.getListByTitle(ListTypeTitles.backlog).id, + listId: getters.getListByTitle(ListTypeTitles.backlog)?.id, }); }, @@ -294,7 +295,7 @@ export default { commit(types.REMOVE_LIST_FAILURE, listsBackup); } else { dispatch('fetchItemsForList', { - listId: getters.getListByTitle(ListTypeTitles.backlog).id, + listId: getters.getListByTitle(ListTypeTitles.backlog)?.id, }); } }, @@ -305,6 +306,8 @@ export default { }, fetchItemsForList: ({ state, commit }, { listId, fetchNext = false }) => { + if (!listId) return null; + if (!fetchNext) { commit(types.RESET_ITEMS_FOR_LIST, listId); } diff --git a/app/controllers/admin/cohorts_controller.rb b/app/controllers/admin/cohorts_controller.rb index 723f4145c43..e750b5c5ad4 100644 --- a/app/controllers/admin/cohorts_controller.rb +++ b/app/controllers/admin/cohorts_controller.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class Admin::CohortsController < Admin::ApplicationController - include Analytics::UniqueVisitsHelper + include RedisTracking feature_category :devops_reports @@ -21,6 +21,6 @@ class Admin::CohortsController < Admin::ApplicationController end def track_cohorts_visit - track_visit('i_analytics_cohorts') if trackable_html_request? + track_unique_redis_hll_event('i_analytics_cohorts') if trackable_html_request? end end diff --git a/app/controllers/admin/usage_trends_controller.rb b/app/controllers/admin/usage_trends_controller.rb index 7073f71a1a8..0b315517594 100644 --- a/app/controllers/admin/usage_trends_controller.rb +++ b/app/controllers/admin/usage_trends_controller.rb @@ -1,9 +1,9 @@ # frozen_string_literal: true class Admin::UsageTrendsController < Admin::ApplicationController - include Analytics::UniqueVisitsHelper + include RedisTracking - track_unique_visits :index, target_id: 'i_analytics_instance_statistics' + track_redis_hll_event :index, name: 'i_analytics_instance_statistics' feature_category :devops_reports diff --git a/app/controllers/dashboard/todos_controller.rb b/app/controllers/dashboard/todos_controller.rb index 782c8c293fd..25ac0af9731 100644 --- a/app/controllers/dashboard/todos_controller.rb +++ b/app/controllers/dashboard/todos_controller.rb @@ -3,7 +3,6 @@ class Dashboard::TodosController < Dashboard::ApplicationController include ActionView::Helpers::NumberHelper include PaginatedCollection - include Analytics::UniqueVisitsHelper before_action :authorize_read_project!, only: :index before_action :authorize_read_group!, only: :index diff --git a/app/controllers/projects/cycle_analytics_controller.rb b/app/controllers/projects/cycle_analytics_controller.rb index 3142ef0f404..db5ba51ee01 100644 --- a/app/controllers/projects/cycle_analytics_controller.rb +++ b/app/controllers/projects/cycle_analytics_controller.rb @@ -4,12 +4,12 @@ class Projects::CycleAnalyticsController < Projects::ApplicationController include ActionView::Helpers::DateHelper include ActionView::Helpers::TextHelper include CycleAnalyticsParams - include Analytics::UniqueVisitsHelper include GracefulTimeoutHandling + include RedisTracking before_action :authorize_read_cycle_analytics! - track_unique_visits :show, target_id: 'p_analytics_valuestream' + track_redis_hll_event :show, name: 'p_analytics_valuestream' feature_category :planning_analytics diff --git a/app/controllers/projects/graphs_controller.rb b/app/controllers/projects/graphs_controller.rb index ad39b317b31..7a7961c28bb 100644 --- a/app/controllers/projects/graphs_controller.rb +++ b/app/controllers/projects/graphs_controller.rb @@ -2,14 +2,14 @@ class Projects::GraphsController < Projects::ApplicationController include ExtractsPath - include Analytics::UniqueVisitsHelper + include RedisTracking # Authorize before_action :require_non_empty_project before_action :assign_ref_vars before_action :authorize_read_repository_graphs! - track_unique_visits :charts, target_id: 'p_analytics_repo' + track_redis_hll_event :charts, name: 'p_analytics_repo' feature_category :source_code_management diff --git a/app/controllers/projects/pipelines_controller.rb b/app/controllers/projects/pipelines_controller.rb index 576492110d8..b4196878c4f 100644 --- a/app/controllers/projects/pipelines_controller.rb +++ b/app/controllers/projects/pipelines_controller.rb @@ -2,7 +2,7 @@ class Projects::PipelinesController < Projects::ApplicationController include ::Gitlab::Utils::StrongMemoize - include Analytics::UniqueVisitsHelper + include RedisTracking before_action :disable_query_limiting, only: [:create, :retry] before_action :pipeline, except: [:index, :new, :create, :charts, :config_variables] @@ -24,7 +24,7 @@ class Projects::PipelinesController < Projects::ApplicationController around_action :allow_gitaly_ref_name_caching, only: [:index, :show] - track_unique_visits :charts, target_id: 'p_analytics_pipelines' + track_redis_hll_event :charts, name: 'p_analytics_pipelines' wrap_parameters Ci::Pipeline diff --git a/app/helpers/analytics/unique_visits_helper.rb b/app/helpers/analytics/unique_visits_helper.rb deleted file mode 100644 index d5abfccf9c0..00000000000 --- a/app/helpers/analytics/unique_visits_helper.rb +++ /dev/null @@ -1,31 +0,0 @@ -# frozen_string_literal: true - -module Analytics - module UniqueVisitsHelper - include Gitlab::Tracking::Helpers - extend ActiveSupport::Concern - - def visitor_id - return cookies[:visitor_id] if cookies[:visitor_id].present? - return unless current_user - - uuid = SecureRandom.uuid - cookies[:visitor_id] = { value: uuid, expires: 24.months } - uuid - end - - def track_visit(target_id) - return unless visitor_id - - Gitlab::Analytics::UniqueVisits.new.track_visit(target_id, values: visitor_id) - end - - class_methods do - def track_unique_visits(controller_actions, target_id:) - after_action only: controller_actions, if: -> { trackable_html_request? } do - track_visit(target_id) - end - end - end - end -end diff --git a/app/models/concerns/atomic_internal_id.rb b/app/models/concerns/atomic_internal_id.rb index 80cf6260b0b..88f577c3e23 100644 --- a/app/models/concerns/atomic_internal_id.rb +++ b/app/models/concerns/atomic_internal_id.rb @@ -159,9 +159,8 @@ module AtomicInternalId # Defines class methods: # # - with_{scope}_{column}_supply - # This method can be used to allocate a block of IID values during - # bulk operations (importing/copying, etc). This can be more efficient - # than creating instances one-by-one. + # This method can be used to allocate a stream of IID values during + # bulk operations (importing/copying, etc). # # Pass in a block that receives a `Supply` instance. To allocate a new # IID value, call `Supply#next_value`. @@ -181,14 +180,8 @@ module AtomicInternalId scope_attrs = ::AtomicInternalId.scope_attrs(scope_value) usage = ::AtomicInternalId.scope_usage(self) - generator = InternalId::InternalIdGenerator.new(subject, scope_attrs, usage, init) - - generator.with_lock do - supply = Supply.new(generator.record.last_value) - block.call(supply) - ensure - generator.track_greatest(supply.current_value) if supply - end + supply = Supply.new(-> { InternalId.generate_next(subject, scope_attrs, usage, init) }) + block.call(supply) end end end @@ -236,14 +229,14 @@ module AtomicInternalId end class Supply - attr_reader :current_value + attr_reader :generator - def initialize(start_value) - @current_value = start_value + def initialize(generator) + @generator = generator end def next_value - @current_value += 1 + @generator.call end end end diff --git a/app/models/internal_id.rb b/app/models/internal_id.rb index b56bac58705..f114094d69c 100644 --- a/app/models/internal_id.rb +++ b/app/models/internal_id.rb @@ -16,7 +16,7 @@ # * Add `usage` value to enum # * (Optionally) add columns to `internal_ids` if needed for scope. class InternalId < ApplicationRecord - include Gitlab::Utils::StrongMemoize + extend Gitlab::Utils::StrongMemoize belongs_to :project belongs_to :namespace @@ -25,6 +25,10 @@ class InternalId < ApplicationRecord validates :usage, presence: true + scope :filter_by, -> (scope, usage) do + where(**scope, usage: usage) + end + # Increments #last_value and saves the record # # The operation locks the record and gathers a `ROW SHARE` lock (in PostgreSQL). @@ -53,18 +57,15 @@ class InternalId < ApplicationRecord class << self def track_greatest(subject, scope, usage, new_value, init) - InternalIdGenerator.new(subject, scope, usage, init) - .track_greatest(new_value) + build_generator(subject, scope, usage, init).track_greatest(new_value) end def generate_next(subject, scope, usage, init) - InternalIdGenerator.new(subject, scope, usage, init) - .generate + build_generator(subject, scope, usage, init).generate end def reset(subject, scope, usage, value) - InternalIdGenerator.new(subject, scope, usage) - .reset(value) + build_generator(subject, scope, usage).reset(value) end # Flushing records is generally safe in a sense that those @@ -77,11 +78,36 @@ class InternalId < ApplicationRecord where(filter).delete_all end + + def internal_id_transactions_increment(operation:, usage:) + self.internal_id_transactions_total.increment( + operation: operation, + usage: usage.to_s, + in_transaction: ActiveRecord::Base.connection.transaction_open?.to_s + ) + end + + def internal_id_transactions_total + strong_memoize(:internal_id_transactions_total) do + name = :gitlab_internal_id_transactions_total + comment = 'Counts all the internal ids happening within transaction' + + Gitlab::Metrics.counter(name, comment) + end + end + + private + + def build_generator(subject, scope, usage, init = nil) + if Feature.enabled?(:generate_iids_without_explicit_locking) + ImplicitlyLockingInternalIdGenerator.new(subject, scope, usage, init) + else + InternalIdGenerator.new(subject, scope, usage, init) + end + end end class InternalIdGenerator - extend Gitlab::Utils::StrongMemoize - # Generate next internal id for a given scope and usage. # # For currently supported usages, see #usage enum. @@ -117,7 +143,7 @@ class InternalId < ApplicationRecord # init: Block that gets called to initialize InternalId record if not present # Make sure to not throw exceptions in the absence of records (if this is expected). def generate - self.class.internal_id_transactions_increment(operation: :generate, usage: usage) + InternalId.internal_id_transactions_increment(operation: :generate, usage: usage) subject.transaction do # Create a record in internal_ids if one does not yet exist @@ -134,7 +160,7 @@ class InternalId < ApplicationRecord def reset(value) return false unless value - self.class.internal_id_transactions_increment(operation: :reset, usage: usage) + InternalId.internal_id_transactions_increment(operation: :reset, usage: usage) updated = InternalId @@ -149,8 +175,9 @@ class InternalId < ApplicationRecord # and set its new_value if it is higher than the current last_value # # Note this will acquire a ROW SHARE lock on the InternalId record + def track_greatest(new_value) - self.class.internal_id_transactions_increment(operation: :track_greatest, usage: usage) + InternalId.internal_id_transactions_increment(operation: :track_greatest, usage: usage) subject.transaction do record.track_greatest_and_save!(new_value) @@ -162,7 +189,7 @@ class InternalId < ApplicationRecord end def with_lock(&block) - self.class.internal_id_transactions_increment(operation: :with_lock, usage: usage) + InternalId.internal_id_transactions_increment(operation: :with_lock, usage: usage) record.with_lock(&block) end @@ -199,22 +226,118 @@ class InternalId < ApplicationRecord rescue ActiveRecord::RecordNotUnique lookup end + end - def self.internal_id_transactions_increment(operation:, usage:) - self.internal_id_transactions_total.increment( - operation: operation, - usage: usage.to_s, - in_transaction: ActiveRecord::Base.connection.transaction_open?.to_s - ) + class ImplicitlyLockingInternalIdGenerator + # Generate next internal id for a given scope and usage. + # + # For currently supported usages, see #usage enum. + # + # The method implements a locking scheme that has the following properties: + # 1) Generated sequence of internal ids is unique per (scope and usage) + # 2) The method is thread-safe and may be used in concurrent threads/processes. + # 3) The generated sequence is gapless. + # 4) In the absence of a record in the internal_ids table, one will be created + # and last_value will be calculated on the fly. + # + # subject: The instance or class we're generating an internal id for. + # scope: Attributes that define the scope for id generation. + # Valid keys are `project/project_id` and `namespace/namespace_id`. + # usage: Symbol to define the usage of the internal id, see InternalId.usages + # init: Proc that accepts the subject and the scope and returns Integer|NilClass + attr_reader :subject, :scope, :scope_attrs, :usage, :init + + def initialize(subject, scope, usage, init = nil) + @subject = subject + @scope = scope + @usage = usage + @init = init + + raise ArgumentError, 'Scope is not well-defined, need at least one column for scope (given: 0)' if scope.empty? + + unless InternalId.usages.has_key?(usage.to_s) + raise ArgumentError, "Usage '#{usage}' is unknown. Supported values are #{InternalId.usages.keys} from InternalId.usages" + end end - def self.internal_id_transactions_total - strong_memoize(:internal_id_transactions_total) do - name = :gitlab_internal_id_transactions_total - comment = 'Counts all the internal ids happening within transaction' + # Generates next internal id and returns it + # init: Block that gets called to initialize InternalId record if not present + # Make sure to not throw exceptions in the absence of records (if this is expected). + def generate + InternalId.internal_id_transactions_increment(operation: :generate, usage: usage) - Gitlab::Metrics.counter(name, comment) + next_iid = update_record!(subject, scope, usage, arel_table[:last_value] + 1) + + return next_iid if next_iid + + create_record!(subject, scope, usage, init) do |iid| + iid.last_value += 1 end + rescue ActiveRecord::RecordNotUnique + retry + end + + # Reset tries to rewind to `value-1`. This will only succeed, + # if `value` stored in database is equal to `last_value`. + # value: The expected last_value to decrement + def reset(value) + return false unless value + + InternalId.internal_id_transactions_increment(operation: :reset, usage: usage) + + iid = update_record!(subject, scope.merge(last_value: value), usage, arel_table[:last_value] - 1) + iid == value - 1 + end + + # Create a record in internal_ids if one does not yet exist + # and set its new_value if it is higher than the current last_value + def track_greatest(new_value) + InternalId.internal_id_transactions_increment(operation: :track_greatest, usage: usage) + + function = Arel::Nodes::NamedFunction.new('GREATEST', [ + arel_table[:last_value], + new_value.to_i + ]) + + next_iid = update_record!(subject, scope, usage, function) + return next_iid if next_iid + + create_record!(subject, scope, usage, init) do |object| + object.last_value = [object.last_value, new_value].max + end + rescue ActiveRecord::RecordNotUnique + retry + end + + private + + def update_record!(subject, scope, usage, new_value) + stmt = Arel::UpdateManager.new + stmt.table(arel_table) + stmt.set(arel_table[:last_value] => new_value) + stmt.wheres = InternalId.filter_by(scope, usage).arel.constraints + + ActiveRecord::Base.connection.insert(stmt, 'Update InternalId', 'last_value') + end + + def create_record!(subject, scope, usage, init) + raise ArgumentError, 'Cannot initialize without init!' unless init + + instance = subject.is_a?(::Class) ? nil : subject + + subject.transaction(requires_new: true) do + last_value = init.call(instance, scope) || 0 + + internal_id = InternalId.create!(**scope, usage: usage, last_value: last_value) do |subject| + yield subject if block_given? + end + + internal_id.last_value + end + end + + def arel_table + InternalId.arel_table end end end diff --git a/config/feature_flags/development/generate_iids_without_explicit_locking.yml b/config/feature_flags/development/generate_iids_without_explicit_locking.yml new file mode 100644 index 00000000000..d2a7aeb8619 --- /dev/null +++ b/config/feature_flags/development/generate_iids_without_explicit_locking.yml @@ -0,0 +1,8 @@ +--- +name: generate_iids_without_explicit_locking +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/65590 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/335431 +milestone: '14.2' +type: development +group: group::database +default_enabled: false diff --git a/doc/topics/set_up_organization.md b/doc/topics/set_up_organization.md index 2b263143b46..3758435d297 100644 --- a/doc/topics/set_up_organization.md +++ b/doc/topics/set_up_organization.md @@ -10,6 +10,7 @@ Configure your organization and its users. Determine user roles and give everyone access to the projects they need. - [Members](../user/project/members/index.md) +- [Workspace](../user/workspace/index.md) _(Coming soon)_ - [Groups](../user/group/index.md) - [User account options](../user/profile/index.md) - [SSH keys](../ssh/index.md) diff --git a/doc/user/workspace/img/1.1-Instance_overview.png b/doc/user/workspace/img/1.1-Instance_overview.png new file mode 100644 index 00000000000..7612cc7c092 Binary files /dev/null and b/doc/user/workspace/img/1.1-Instance_overview.png differ diff --git a/doc/user/workspace/img/1.2-Groups_overview.png b/doc/user/workspace/img/1.2-Groups_overview.png new file mode 100644 index 00000000000..b4f034bba16 Binary files /dev/null and b/doc/user/workspace/img/1.2-Groups_overview.png differ diff --git a/doc/user/workspace/img/1.3-Admin.png b/doc/user/workspace/img/1.3-Admin.png new file mode 100644 index 00000000000..018ed8a1bfc Binary files /dev/null and b/doc/user/workspace/img/1.3-Admin.png differ diff --git a/doc/user/workspace/img/Admin_Settings.png b/doc/user/workspace/img/Admin_Settings.png new file mode 100644 index 00000000000..5c4656ff342 Binary files /dev/null and b/doc/user/workspace/img/Admin_Settings.png differ diff --git a/doc/user/workspace/index.md b/doc/user/workspace/index.md new file mode 100644 index 00000000000..f098cef6053 --- /dev/null +++ b/doc/user/workspace/index.md @@ -0,0 +1,34 @@ +--- +stage: Manage +group: Access +info: To determine the technical writer assigned to the Stage/Group associated with this page, see https://about.gitlab.com/handbook/engineering/ux/technical-writing/#assignments +--- + +# Workspace + +Workspace will be the top-level [namespace](../group/index.md#namespaces) for you to manage +everything GitLab, including: + +- Defining and applying settings to all of your groups, subgroups, and projects. +- Aggregating data from all your groups, subgroups, and projects. + +Workspace will take many of the features from the +[Admin Area](../admin_area/index.md), and there will be one workspace per: + +- Instance, for self-managed instances. +- Namespace, for GitLab.com. + +NOTE: +Workspace is currently in development. + +## Concept previews + +The following provide a preview to the Workspace concept. + +![Workspace Overview](img/1.1-Instance_overview.png) + +![Groups Overview](img/1.2-Groups_overview.png) + +![Admin Overview](img/1.3-Admin.png) + +![Admin Overview](img/Admin_Settings.png) diff --git a/lib/gitlab/analytics/unique_visits.rb b/lib/gitlab/analytics/unique_visits.rb index 723486231b1..3546a7e3ddb 100644 --- a/lib/gitlab/analytics/unique_visits.rb +++ b/lib/gitlab/analytics/unique_visits.rb @@ -3,10 +3,6 @@ module Gitlab module Analytics class UniqueVisits - def track_visit(*args, **kwargs) - Gitlab::UsageDataCounters::HLLRedisCounter.track_event(*args, **kwargs) - end - # Returns number of unique visitors for given targets in given time frame # # @param [String, Array[]] targets ids of targets to count visits on. Special case for :any diff --git a/spec/frontend/boards/mock_data.js b/spec/frontend/boards/mock_data.js index 73372eafb89..6ac4db8cdaa 100644 --- a/spec/frontend/boards/mock_data.js +++ b/spec/frontend/boards/mock_data.js @@ -291,7 +291,7 @@ export const setMockEndpoints = (opts = {}) => { export const mockList = { id: 'gid://gitlab/List/1', - title: 'Backlog', + title: 'Open', position: -Infinity, listType: 'backlog', collapsed: false, diff --git a/spec/frontend/boards/stores/actions_spec.js b/spec/frontend/boards/stores/actions_spec.js index ebed33e5b34..5e16e389ddc 100644 --- a/spec/frontend/boards/stores/actions_spec.js +++ b/spec/frontend/boards/stores/actions_spec.js @@ -715,6 +715,19 @@ describe('fetchItemsForList', () => { [listId]: pageInfo, }; + describe('when list id is undefined', () => { + it('does not call the query', async () => { + jest.spyOn(gqlClient, 'query').mockResolvedValue(queryResponse); + + await actions.fetchItemsForList( + { state, getters: () => {}, commit: () => {} }, + { listId: undefined }, + ); + + expect(gqlClient.query).toHaveBeenCalledTimes(0); + }); + }); + it('should commit mutations REQUEST_ITEMS_FOR_LIST and RECEIVE_ITEMS_FOR_LIST_SUCCESS on success', (done) => { jest.spyOn(gqlClient, 'query').mockResolvedValue(queryResponse); diff --git a/spec/helpers/analytics/unique_visits_helper_spec.rb b/spec/helpers/analytics/unique_visits_helper_spec.rb deleted file mode 100644 index b4b370c169d..00000000000 --- a/spec/helpers/analytics/unique_visits_helper_spec.rb +++ /dev/null @@ -1,34 +0,0 @@ -# frozen_string_literal: true - -require "spec_helper" - -RSpec.describe Analytics::UniqueVisitsHelper do - include Devise::Test::ControllerHelpers - - describe '#track_visit' do - let(:target_id) { 'p_analytics_valuestream' } - let(:current_user) { create(:user) } - - it 'does not track visit if user is not logged in' do - expect_any_instance_of(Gitlab::Analytics::UniqueVisits).not_to receive(:track_visit) - - helper.track_visit(target_id) - end - - it 'tracks visit if user is logged in' do - sign_in(current_user) - - expect_any_instance_of(Gitlab::Analytics::UniqueVisits).to receive(:track_visit) - - helper.track_visit(target_id) - end - - it 'tracks visit if user is not logged in, but has the cookie already' do - helper.request.cookies[:visitor_id] = { value: SecureRandom.uuid, expires: 24.months } - - expect_any_instance_of(Gitlab::Analytics::UniqueVisits).to receive(:track_visit) - - helper.track_visit(target_id) - end - end -end diff --git a/spec/lib/gitlab/analytics/unique_visits_spec.rb b/spec/lib/gitlab/analytics/unique_visits_spec.rb deleted file mode 100644 index f4d5c0b1eca..00000000000 --- a/spec/lib/gitlab/analytics/unique_visits_spec.rb +++ /dev/null @@ -1,81 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe Gitlab::Analytics::UniqueVisits, :clean_gitlab_redis_shared_state do - let(:unique_visits) { Gitlab::Analytics::UniqueVisits.new } - let(:target1_id) { 'g_analytics_contribution' } - let(:target2_id) { 'g_analytics_insights' } - let(:target3_id) { 'g_analytics_issues' } - let(:target4_id) { 'g_compliance_dashboard' } - let(:target5_id) { 'i_compliance_credential_inventory' } - let(:visitor1_id) { 'dfb9d2d2-f56c-4c77-8aeb-6cddc4a1f857' } - let(:visitor2_id) { '1dd9afb2-a3ee-4de1-8ae3-a405579c8584' } - let(:visitor3_id) { '34rfjuuy-ce56-sa35-ds34-dfer567dfrf2' } - - around do |example| - # We need to freeze to a reference time - # because visits are grouped by the week number in the year - # Without freezing the time, the test may behave inconsistently - # depending on which day of the week test is run. - reference_time = Time.utc(2020, 6, 1) - travel_to(reference_time) { example.run } - end - - describe '#track_visit' do - it 'tracks the unique weekly visits for targets' do - unique_visits.track_visit(target1_id, values: visitor1_id, time: 7.days.ago) - unique_visits.track_visit(target1_id, values: visitor1_id, time: 7.days.ago) - unique_visits.track_visit(target1_id, values: visitor2_id, time: 7.days.ago) - - unique_visits.track_visit(target2_id, values: visitor2_id, time: 7.days.ago) - unique_visits.track_visit(target2_id, values: visitor1_id, time: 8.days.ago) - unique_visits.track_visit(target2_id, values: visitor1_id, time: 15.days.ago) - - unique_visits.track_visit(target4_id, values: visitor3_id, time: 7.days.ago) - - unique_visits.track_visit(target5_id, values: visitor3_id, time: 15.days.ago) - unique_visits.track_visit(target5_id, values: visitor2_id, time: 15.days.ago) - - expect(unique_visits.unique_visits_for(targets: target1_id)).to eq(2) - expect(unique_visits.unique_visits_for(targets: target2_id)).to eq(1) - expect(unique_visits.unique_visits_for(targets: target4_id)).to eq(1) - - expect(unique_visits.unique_visits_for(targets: target2_id, start_date: 15.days.ago)).to eq(1) - - expect(unique_visits.unique_visits_for(targets: target3_id)).to eq(0) - - expect(unique_visits.unique_visits_for(targets: target5_id, start_date: 15.days.ago)).to eq(2) - - expect(unique_visits.unique_visits_for(targets: :analytics)).to eq(2) - expect(unique_visits.unique_visits_for(targets: :analytics, start_date: 15.days.ago)).to eq(1) - expect(unique_visits.unique_visits_for(targets: :analytics, start_date: 30.days.ago)).to eq(0) - - expect(unique_visits.unique_visits_for(targets: :analytics, start_date: 4.weeks.ago, end_date: Date.current)).to eq(2) - - expect(unique_visits.unique_visits_for(targets: :compliance)).to eq(1) - expect(unique_visits.unique_visits_for(targets: :compliance, start_date: 15.days.ago)).to eq(2) - expect(unique_visits.unique_visits_for(targets: :compliance, start_date: 30.days.ago)).to eq(0) - - expect(unique_visits.unique_visits_for(targets: :compliance, start_date: 4.weeks.ago, end_date: Date.current)).to eq(2) - end - - it 'sets the keys in Redis to expire automatically after 12 weeks' do - unique_visits.track_visit(target1_id, values: visitor1_id) - - Gitlab::Redis::SharedState.with do |redis| - redis.scan_each(match: "{#{target1_id}}-*").each do |key| - expect(redis.ttl(key)).to be_within(5.seconds).of(12.weeks) - end - end - end - - it 'raises an error if an invalid target id is given' do - invalid_target_id = "x_invalid" - - expect do - unique_visits.track_visit(invalid_target_id, values: visitor1_id) - end.to raise_error(Gitlab::UsageDataCounters::HLLRedisCounter::UnknownEvent) - end - end -end diff --git a/spec/models/concerns/atomic_internal_id_spec.rb b/spec/models/concerns/atomic_internal_id_spec.rb index 35b0f107676..b803e699b25 100644 --- a/spec/models/concerns/atomic_internal_id_spec.rb +++ b/spec/models/concerns/atomic_internal_id_spec.rb @@ -240,18 +240,12 @@ RSpec.describe AtomicInternalId do end describe '.with_project_iid_supply' do - let(:iid) { 100 } - - it 'wraps generate and track_greatest in a concurrency-safe lock' do - expect_next_instance_of(InternalId::InternalIdGenerator) do |g| - expect(g).to receive(:with_lock).and_call_original - expect(g.record).to receive(:last_value).and_return(iid) - expect(g).to receive(:track_greatest).with(iid + 4) - end - - ::Milestone.with_project_iid_supply(milestone.project) do |supply| - 4.times { supply.next_value } - end + it 'supplies a stream of iid values' do + expect do + ::Milestone.with_project_iid_supply(milestone.project) do |supply| + 4.times { supply.next_value } + end + end.to change { InternalId.find_by(project: milestone.project, usage: :milestones)&.last_value.to_i }.by(4) end end end diff --git a/spec/models/internal_id_spec.rb b/spec/models/internal_id_spec.rb index 390d1552c16..696b5b48cbf 100644 --- a/spec/models/internal_id_spec.rb +++ b/spec/models/internal_id_spec.rb @@ -39,12 +39,144 @@ RSpec.describe InternalId do end end - describe '.generate_next' do - subject { described_class.generate_next(id_subject, scope, usage, init) } + shared_examples_for 'a monotonically increasing id generator' do + describe '.generate_next' do + subject { described_class.generate_next(id_subject, scope, usage, init) } - context 'in the absence of a record' do - it 'creates a record if not yet present' do - expect { subject }.to change { described_class.count }.from(0).to(1) + context 'in the absence of a record' do + it 'creates a record if not yet present' do + expect { subject }.to change { described_class.count }.from(0).to(1) + end + + it 'stores record attributes' do + subject + + described_class.first.tap do |record| + expect(record.project).to eq(project) + expect(record.usage).to eq(usage.to_s) + end + end + + context 'with existing issues' do + before do + create_list(:issue, 2, project: project) + described_class.delete_all + end + + it 'calculates last_value values automatically' do + expect(subject).to eq(project.issues.size + 1) + end + end + end + + it 'generates a strictly monotone, gapless sequence' do + seq = Array.new(10).map do + described_class.generate_next(issue, scope, usage, init) + end + normalized = seq.map { |i| i - seq.min } + + expect(normalized).to eq((0..seq.size - 1).to_a) + end + + context 'there are no instances to pass in' do + let(:id_subject) { Issue } + + it 'accepts classes instead' do + expect(subject).to eq(1) + end + end + + context 'when executed outside of transaction' do + it 'increments counter with in_transaction: "false"' do + allow(ActiveRecord::Base.connection).to receive(:transaction_open?) { false } + + expect(InternalId.internal_id_transactions_total).to receive(:increment) + .with(operation: :generate, usage: 'issues', in_transaction: 'false').and_call_original + + subject + end + end + + context 'when executed within transaction' do + it 'increments counter with in_transaction: "true"' do + expect(InternalId.internal_id_transactions_total).to receive(:increment) + .with(operation: :generate, usage: 'issues', in_transaction: 'true').and_call_original + + InternalId.transaction { subject } + end + end + end + + describe '.reset' do + subject { described_class.reset(issue, scope, usage, value) } + + context 'in the absence of a record' do + let(:value) { 2 } + + it 'does not revert back the value' do + expect { subject }.not_to change { described_class.count } + expect(subject).to be_falsey + end + end + + context 'when valid iid is used to reset' do + let!(:value) { generate_next } + + context 'and iid is a latest one' do + it 'does rewind and next generated value is the same' do + expect(subject).to be_truthy + expect(generate_next).to eq(value) + end + end + + context 'and iid is not a latest one' do + it 'does not rewind' do + generate_next + + expect(subject).to be_falsey + expect(generate_next).to be > value + end + end + + def generate_next + described_class.generate_next(issue, scope, usage, init) + end + end + + context 'when executed outside of transaction' do + let(:value) { 2 } + + it 'increments counter with in_transaction: "false"' do + allow(ActiveRecord::Base.connection).to receive(:transaction_open?) { false } + + expect(InternalId.internal_id_transactions_total).to receive(:increment) + .with(operation: :reset, usage: 'issues', in_transaction: 'false').and_call_original + + subject + end + end + + context 'when executed within transaction' do + let(:value) { 2 } + + it 'increments counter with in_transaction: "true"' do + expect(InternalId.internal_id_transactions_total).to receive(:increment) + .with(operation: :reset, usage: 'issues', in_transaction: 'true').and_call_original + + InternalId.transaction { subject } + end + end + end + + describe '.track_greatest' do + let(:value) { 9001 } + + subject { described_class.track_greatest(id_subject, scope, usage, value, init) } + + context 'in the absence of a record' do + it 'creates a record if not yet present' do + expect { subject }.to change { described_class.count }.from(0).to(1) + end end it 'stores record attributes' do @@ -53,200 +185,69 @@ RSpec.describe InternalId do described_class.first.tap do |record| expect(record.project).to eq(project) expect(record.usage).to eq(usage.to_s) + expect(record.last_value).to eq(value) end end context 'with existing issues' do before do - create_list(:issue, 2, project: project) + create(:issue, project: project) described_class.delete_all end - it 'calculates last_value values automatically' do - expect(subject).to eq(project.issues.size + 1) + it 'still returns the last value to that of the given value' do + expect(subject).to eq(value) end end - context 'with concurrent inserts on table' do - it 'looks up the record if it was created concurrently' do - args = { **scope, usage: described_class.usages[usage.to_s] } - record = double - expect(described_class).to receive(:find_by).with(args).and_return(nil) # first call, record not present - expect(described_class).to receive(:find_by).with(args).and_return(record) # second call, record was created by another process - expect(described_class).to receive(:create!).and_raise(ActiveRecord::RecordNotUnique, 'record not unique') - expect(record).to receive(:increment_and_save!) + context 'when value is less than the current last_value' do + it 'returns the current last_value' do + described_class.create!(**scope, usage: usage, last_value: 10_001) + + expect(subject).to eq 10_001 + end + end + + context 'there are no instances to pass in' do + let(:id_subject) { Issue } + + it 'accepts classes instead' do + expect(subject).to eq(value) + end + end + + context 'when executed outside of transaction' do + it 'increments counter with in_transaction: "false"' do + allow(ActiveRecord::Base.connection).to receive(:transaction_open?) { false } + + expect(InternalId.internal_id_transactions_total).to receive(:increment) + .with(operation: :track_greatest, usage: 'issues', in_transaction: 'false').and_call_original subject end end - end - it 'generates a strictly monotone, gapless sequence' do - seq = Array.new(10).map do - described_class.generate_next(issue, scope, usage, init) - end - normalized = seq.map { |i| i - seq.min } + context 'when executed within transaction' do + it 'increments counter with in_transaction: "true"' do + expect(InternalId.internal_id_transactions_total).to receive(:increment) + .with(operation: :track_greatest, usage: 'issues', in_transaction: 'true').and_call_original - expect(normalized).to eq((0..seq.size - 1).to_a) - end - - context 'there are no instances to pass in' do - let(:id_subject) { Issue } - - it 'accepts classes instead' do - expect(subject).to eq(1) - end - end - - context 'when executed outside of transaction' do - it 'increments counter with in_transaction: "false"' do - allow(ActiveRecord::Base.connection).to receive(:transaction_open?) { false } - - expect(InternalId::InternalIdGenerator.internal_id_transactions_total).to receive(:increment) - .with(operation: :generate, usage: 'issues', in_transaction: 'false').and_call_original - - subject - end - end - - context 'when executed within transaction' do - it 'increments counter with in_transaction: "true"' do - expect(InternalId::InternalIdGenerator.internal_id_transactions_total).to receive(:increment) - .with(operation: :generate, usage: 'issues', in_transaction: 'true').and_call_original - - InternalId.transaction { subject } + InternalId.transaction { subject } + end end end end - describe '.reset' do - subject { described_class.reset(issue, scope, usage, value) } + context 'when the feature flag is disabled' do + stub_feature_flags(generate_iids_without_explicit_locking: false) - context 'in the absence of a record' do - let(:value) { 2 } - - it 'does not revert back the value' do - expect { subject }.not_to change { described_class.count } - expect(subject).to be_falsey - end - end - - context 'when valid iid is used to reset' do - let!(:value) { generate_next } - - context 'and iid is a latest one' do - it 'does rewind and next generated value is the same' do - expect(subject).to be_truthy - expect(generate_next).to eq(value) - end - end - - context 'and iid is not a latest one' do - it 'does not rewind' do - generate_next - - expect(subject).to be_falsey - expect(generate_next).to be > value - end - end - - def generate_next - described_class.generate_next(issue, scope, usage, init) - end - end - - context 'when executed outside of transaction' do - let(:value) { 2 } - - it 'increments counter with in_transaction: "false"' do - allow(ActiveRecord::Base.connection).to receive(:transaction_open?) { false } - - expect(InternalId::InternalIdGenerator.internal_id_transactions_total).to receive(:increment) - .with(operation: :reset, usage: 'issues', in_transaction: 'false').and_call_original - - subject - end - end - - context 'when executed within transaction' do - let(:value) { 2 } - - it 'increments counter with in_transaction: "true"' do - expect(InternalId::InternalIdGenerator.internal_id_transactions_total).to receive(:increment) - .with(operation: :reset, usage: 'issues', in_transaction: 'true').and_call_original - - InternalId.transaction { subject } - end - end + it_behaves_like 'a monotonically increasing id generator' end - describe '.track_greatest' do - let(:value) { 9001 } + context 'when the feature flag is enabled' do + stub_feature_flags(generate_iids_without_explicit_locking: true) - subject { described_class.track_greatest(id_subject, scope, usage, value, init) } - - context 'in the absence of a record' do - it 'creates a record if not yet present' do - expect { subject }.to change { described_class.count }.from(0).to(1) - end - end - - it 'stores record attributes' do - subject - - described_class.first.tap do |record| - expect(record.project).to eq(project) - expect(record.usage).to eq(usage.to_s) - expect(record.last_value).to eq(value) - end - end - - context 'with existing issues' do - before do - create(:issue, project: project) - described_class.delete_all - end - - it 'still returns the last value to that of the given value' do - expect(subject).to eq(value) - end - end - - context 'when value is less than the current last_value' do - it 'returns the current last_value' do - described_class.create!(**scope, usage: usage, last_value: 10_001) - - expect(subject).to eq 10_001 - end - end - - context 'there are no instances to pass in' do - let(:id_subject) { Issue } - - it 'accepts classes instead' do - expect(subject).to eq(value) - end - end - - context 'when executed outside of transaction' do - it 'increments counter with in_transaction: "false"' do - allow(ActiveRecord::Base.connection).to receive(:transaction_open?) { false } - - expect(InternalId::InternalIdGenerator.internal_id_transactions_total).to receive(:increment) - .with(operation: :track_greatest, usage: 'issues', in_transaction: 'false').and_call_original - - subject - end - end - - context 'when executed within transaction' do - it 'increments counter with in_transaction: "true"' do - expect(InternalId::InternalIdGenerator.internal_id_transactions_total).to receive(:increment) - .with(operation: :track_greatest, usage: 'issues', in_transaction: 'true').and_call_original - - InternalId.transaction { subject } - end - end + it_behaves_like 'a monotonically increasing id generator' end describe '#increment_and_save!' do diff --git a/spec/support/shared_examples/models/atomic_internal_id_shared_examples.rb b/spec/support/shared_examples/models/atomic_internal_id_shared_examples.rb index 42f82987989..03f565e0aac 100644 --- a/spec/support/shared_examples/models/atomic_internal_id_shared_examples.rb +++ b/spec/support/shared_examples/models/atomic_internal_id_shared_examples.rb @@ -165,9 +165,9 @@ RSpec.shared_examples 'AtomicInternalId' do |validate_presence: true| 3.times { supply.next_value } end - current_value = described_class.public_send(method_name, scope_value, &:current_value) - - expect(current_value).to eq(iid + 3) + described_class.public_send(method_name, scope_value) do |supply| + expect(supply.next_value).to eq(iid + 4) + end end end