From 1e662293e8fc2f2eba9657dd27449e966736a14a Mon Sep 17 00:00:00 2001 From: Tiago Botelho Date: Tue, 2 Oct 2018 16:33:43 +0100 Subject: Implements Web IDE commits counter in Redis This makes a temporary implementation of the Web IDE commits counter using Redis while https://gitlab.com/gitlab-org/gitlab-ce/issues/52096 is being discussed further for a more generic approach to counters --- app/models/usage_counters.rb | 42 ---------------------- db/migrate/20180929102611_create_usage_counters.rb | 13 ------- db/schema.rb | 8 +---- lib/api/commits.rb | 2 +- lib/gitlab/usage_data.rb | 4 ++- lib/gitlab/web_ide_commits_counter.rb | 17 +++++++++ spec/factories/usage_counters.rb | 6 ---- spec/lib/gitlab/import_export/all_models.yml | 3 -- spec/lib/gitlab/web_ide_commits_counter_spec.rb | 19 ++++++++++ spec/models/usage_counters_spec.rb | 31 ---------------- spec/requests/api/commits_spec.rb | 4 +-- .../usage_counters_increment_service_spec.rb | 27 -------------- 12 files changed, 43 insertions(+), 133 deletions(-) delete mode 100644 app/models/usage_counters.rb delete mode 100644 db/migrate/20180929102611_create_usage_counters.rb create mode 100644 lib/gitlab/web_ide_commits_counter.rb delete mode 100644 spec/factories/usage_counters.rb create mode 100644 spec/lib/gitlab/web_ide_commits_counter_spec.rb delete mode 100644 spec/models/usage_counters_spec.rb delete mode 100644 spec/services/projects/usage_counters_increment_service_spec.rb diff --git a/app/models/usage_counters.rb b/app/models/usage_counters.rb deleted file mode 100644 index 90f9c2a976e..00000000000 --- a/app/models/usage_counters.rb +++ /dev/null @@ -1,42 +0,0 @@ -# frozen_string_literal: true - -class UsageCounters < ActiveRecord::Base - RECORD_LIMIT = 1.freeze - BY = 1.freeze - BLACKLIST_ATTRIBUTES = %w(id created_at updated_at).freeze - - validate :ensure_only_one, on: :create - - default_value_for :web_ide_commits, 0 - - # This method supports concurrency so that several - # requests are able to increment the counter without - # us having inconsistent data - def increment_counters(attrs) - # We want to be able to use the service to increment - # both a single and multiple counters - attrs = Array(attrs) - - attrs_with_by = - attrs.each_with_object({}) do |attr, hsh| - hsh[attr] = BY - end - - self.class.update_counters(id, attrs_with_by) - end - - # Every attribute in this table except the blacklisted - # attributes is a counter - def totals - attributes.except(*BLACKLIST_ATTRIBUTES).symbolize_keys - end - - private - - # We only want one UsageCounters per instance - def ensure_only_one - return unless UsageCounters.count >= RECORD_LIMIT - - errors.add(:base, 'There can only be one usage counters record per instance') - end -end diff --git a/db/migrate/20180929102611_create_usage_counters.rb b/db/migrate/20180929102611_create_usage_counters.rb deleted file mode 100644 index f17486bb513..00000000000 --- a/db/migrate/20180929102611_create_usage_counters.rb +++ /dev/null @@ -1,13 +0,0 @@ -# frozen_string_literal: true - -class CreateUsageCounters < ActiveRecord::Migration - DOWNTIME = false - - def change - create_table :usage_counters do |t| - t.integer :web_ide_commits - - t.timestamps_with_timezone null: false - end - end -end diff --git a/db/schema.rb b/db/schema.rb index fb57f2452d7..f92d8005dfb 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20180929102611) do +ActiveRecord::Schema.define(version: 20180917172041) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -2080,12 +2080,6 @@ ActiveRecord::Schema.define(version: 20180929102611) do add_index "uploads", ["model_id", "model_type"], name: "index_uploads_on_model_id_and_model_type", using: :btree add_index "uploads", ["uploader", "path"], name: "index_uploads_on_uploader_and_path", using: :btree - create_table "usage_counters", force: :cascade do |t| - t.integer "web_ide_commits" - t.datetime_with_timezone "created_at", null: false - t.datetime_with_timezone "updated_at", null: false - end - create_table "user_agent_details", force: :cascade do |t| t.string "user_agent", null: false t.string "ip_address", null: false diff --git a/lib/api/commits.rb b/lib/api/commits.rb index e16dd29d138..ec44936d114 100644 --- a/lib/api/commits.rb +++ b/lib/api/commits.rb @@ -109,7 +109,7 @@ module API if result[:status] == :success commit_detail = user_project.repository.commit(result[:result]) - UsageCounters.first_or_create.increment_counters(:web_ide_commits) if find_user_from_warden + Gitlab::WebIdeCommitsCounter.increment if find_user_from_warden present commit_detail, with: Entities::CommitDetail else diff --git a/lib/gitlab/usage_data.rb b/lib/gitlab/usage_data.rb index afab36e89dd..5097c3253c9 100644 --- a/lib/gitlab/usage_data.rb +++ b/lib/gitlab/usage_data.rb @@ -108,7 +108,9 @@ module Gitlab end def usage_counters - UsageCounters.first_or_create.totals + { + web_ide_commits: Gitlab::WebIdeCommitsCounter.total_count + } end def components_usage_data diff --git a/lib/gitlab/web_ide_commits_counter.rb b/lib/gitlab/web_ide_commits_counter.rb new file mode 100644 index 00000000000..1cd9b5295b9 --- /dev/null +++ b/lib/gitlab/web_ide_commits_counter.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +module Gitlab + module WebIdeCommitsCounter + WEB_IDE_COMMITS_KEY = "WEB_IDE_COMMITS_COUNT".freeze + + class << self + def increment + Gitlab::Redis::SharedState.with { |redis| redis.incr(WEB_IDE_COMMITS_KEY) } + end + + def total_count + Gitlab::Redis::SharedState.with { |redis| redis.get(WEB_IDE_COMMITS_KEY).to_i } + end + end + end +end diff --git a/spec/factories/usage_counters.rb b/spec/factories/usage_counters.rb deleted file mode 100644 index 23277fd741e..00000000000 --- a/spec/factories/usage_counters.rb +++ /dev/null @@ -1,6 +0,0 @@ -# frozen_string_literal: true - -FactoryBot.define do - factory :usage_counters, class: 'UsageCounters' do - end -end diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml index b2a0afcb827..ec2bdbe22e1 100644 --- a/spec/lib/gitlab/import_export/all_models.yml +++ b/spec/lib/gitlab/import_export/all_models.yml @@ -330,6 +330,3 @@ resource_label_events: - merge_request - epic - label -usage_counters: -- project -- web_ide_commits diff --git a/spec/lib/gitlab/web_ide_commits_counter_spec.rb b/spec/lib/gitlab/web_ide_commits_counter_spec.rb new file mode 100644 index 00000000000..c51889a1c63 --- /dev/null +++ b/spec/lib/gitlab/web_ide_commits_counter_spec.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::WebIdeCommitsCounter, :clean_gitlab_redis_shared_state do + describe '.increment' do + it 'increments the web ide commits counter by 1' do + expect do + described_class.increment + end.to change { described_class.total_count }.from(0).to(1) + end + end + + describe '.total_count' do + it 'returns the total amount of web ide commits' do + expect(described_class.total_count).to eq(0) + end + end +end diff --git a/spec/models/usage_counters_spec.rb b/spec/models/usage_counters_spec.rb deleted file mode 100644 index 0adbfe3cef7..00000000000 --- a/spec/models/usage_counters_spec.rb +++ /dev/null @@ -1,31 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -describe UsageCounters do - let!(:usage_counters) { create(:usage_counters) } - - describe 'maximum number of records' do - it 'allows for one single record to be created' do - expect do - described_class.create! - end.to raise_error(ActiveRecord::RecordInvalid, 'Validation failed: There can only be one usage counters record per instance') - end - end - - describe '#totals' do - subject { usage_counters.totals } - - it 'returns counters' do - is_expected.to include(web_ide_commits: 0) - end - end - - describe '#increment_counters' do - it 'increments specified counters by 1' do - expect do - usage_counters.increment_counters(:web_ide_commits) - end.to change { usage_counters.reload.web_ide_commits }.from(0).to(1) - end - end -end diff --git a/spec/requests/api/commits_spec.rb b/spec/requests/api/commits_spec.rb index aebc24dd9a2..06ccf383362 100644 --- a/spec/requests/api/commits_spec.rb +++ b/spec/requests/api/commits_spec.rb @@ -279,9 +279,9 @@ describe API::Commits do end it 'does not increment the usage counters using access token authentication' do - post api(url, user), valid_c_params + expect(::Gitlab::WebIdeCommitsCounter).not_to receive(:increment) - expect_any_instance_of(::UsageCounters).not_to receive(:increment_counters) + post api(url, user), valid_c_params end it 'a new file in project repo' do diff --git a/spec/services/projects/usage_counters_increment_service_spec.rb b/spec/services/projects/usage_counters_increment_service_spec.rb deleted file mode 100644 index d82a7337665..00000000000 --- a/spec/services/projects/usage_counters_increment_service_spec.rb +++ /dev/null @@ -1,27 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -describe Projects::UsageCountersIncrementService do - let(:project) { create(:usage_counters) } - - subject(:service) { described_class.new(project) } - - context '#execute' do - context 'when single attribute is passed' do - it 'increments attribute' do - expect do - service.execute(:web_ide_commits) - end.to change { project.usage_counters.reload.web_ide_commits }.from(0).to(1) - end - end - - context 'when array is passed' do - it 'increments specified attributes' do - expect do - service.execute(%i(web_ide_commits)) - end.to change { project.usage_counters.reload.web_ide_commits }.from(0).to(1) - end - end - end -end -- cgit v1.2.1