From 9b2e17ac71ee446da0f34dada41401803af816c7 Mon Sep 17 00:00:00 2001
From: Tiago Botelho <tiagonbotelho@hotmail.com>
Date: Mon, 1 Oct 2018 12:15:31 +0100
Subject: [PATCH 1/2] Adds WebIDE commits to UsagePing

Implements UsageCounters model to track feature usage counters
and makes easy to extend for future counters
---
 app/models/usage_counters.rb                  | 42 +++++++++++++++++++
 ...5016-add-web-ide-commits-to-usage-ping.yml |  5 +++
 .../20180929102611_create_usage_counters.rb   | 13 ++++++
 db/schema.rb                                  |  8 +++-
 lib/api/commits.rb                            |  3 ++
 lib/gitlab/usage_data.rb                      |  5 +++
 spec/factories/usage_counters.rb              |  6 +++
 spec/lib/gitlab/import_export/all_models.yml  |  3 ++
 spec/lib/gitlab/usage_data_spec.rb            |  1 +
 spec/models/usage_counters_spec.rb            | 31 ++++++++++++++
 spec/requests/api/commits_spec.rb             |  6 +++
 .../usage_counters_increment_service_spec.rb  | 27 ++++++++++++
 12 files changed, 149 insertions(+), 1 deletion(-)
 create mode 100644 app/models/usage_counters.rb
 create mode 100644 changelogs/unreleased/45016-add-web-ide-commits-to-usage-ping.yml
 create mode 100644 db/migrate/20180929102611_create_usage_counters.rb
 create mode 100644 spec/factories/usage_counters.rb
 create mode 100644 spec/models/usage_counters_spec.rb
 create 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
new file mode 100644
index 00000000000..90f9c2a976e
--- /dev/null
+++ b/app/models/usage_counters.rb
@@ -0,0 +1,42 @@
+# 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/changelogs/unreleased/45016-add-web-ide-commits-to-usage-ping.yml b/changelogs/unreleased/45016-add-web-ide-commits-to-usage-ping.yml
new file mode 100644
index 00000000000..a7f24742588
--- /dev/null
+++ b/changelogs/unreleased/45016-add-web-ide-commits-to-usage-ping.yml
@@ -0,0 +1,5 @@
+---
+title: Adds Web IDE commits to usage ping
+merge_request: 22007
+author:
+type: added
diff --git a/db/migrate/20180929102611_create_usage_counters.rb b/db/migrate/20180929102611_create_usage_counters.rb
new file mode 100644
index 00000000000..f17486bb513
--- /dev/null
+++ b/db/migrate/20180929102611_create_usage_counters.rb
@@ -0,0 +1,13 @@
+# 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 f92d8005dfb..fb57f2452d7 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: 20180917172041) do
+ActiveRecord::Schema.define(version: 20180929102611) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -2080,6 +2080,12 @@ ActiveRecord::Schema.define(version: 20180917172041) 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 5aeffc8fb99..e16dd29d138 100644
--- a/lib/api/commits.rb
+++ b/lib/api/commits.rb
@@ -108,6 +108,9 @@ 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
+
           present commit_detail, with: Entities::CommitDetail
         else
           render_api_error!(result[:message], 400)
diff --git a/lib/gitlab/usage_data.rb b/lib/gitlab/usage_data.rb
index f7d8ee571cd..afab36e89dd 100644
--- a/lib/gitlab/usage_data.rb
+++ b/lib/gitlab/usage_data.rb
@@ -10,6 +10,7 @@ module Gitlab
                           .merge(features_usage_data)
                           .merge(components_usage_data)
                           .merge(cycle_analytics_usage_data)
+                          .merge(usage_counters)
       end
 
       def to_json(force_refresh: false)
@@ -106,6 +107,10 @@ module Gitlab
         }
       end
 
+      def usage_counters
+        UsageCounters.first_or_create.totals
+      end
+
       def components_usage_data
         {
           gitlab_pages: { enabled: Gitlab.config.pages.enabled, version: Gitlab::Pages::VERSION },
diff --git a/spec/factories/usage_counters.rb b/spec/factories/usage_counters.rb
new file mode 100644
index 00000000000..23277fd741e
--- /dev/null
+++ b/spec/factories/usage_counters.rb
@@ -0,0 +1,6 @@
+# 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 ec2bdbe22e1..b2a0afcb827 100644
--- a/spec/lib/gitlab/import_export/all_models.yml
+++ b/spec/lib/gitlab/import_export/all_models.yml
@@ -330,3 +330,6 @@ resource_label_events:
 - merge_request
 - epic
 - label
+usage_counters:
+- project
+- web_ide_commits
diff --git a/spec/lib/gitlab/usage_data_spec.rb b/spec/lib/gitlab/usage_data_spec.rb
index 1ec1fe10744..d669c42ab4a 100644
--- a/spec/lib/gitlab/usage_data_spec.rb
+++ b/spec/lib/gitlab/usage_data_spec.rb
@@ -46,6 +46,7 @@ describe Gitlab::UsageData do
         git
         database
         avg_cycle_analytics
+        web_ide_commits
       ))
     end
 
diff --git a/spec/models/usage_counters_spec.rb b/spec/models/usage_counters_spec.rb
new file mode 100644
index 00000000000..0adbfe3cef7
--- /dev/null
+++ b/spec/models/usage_counters_spec.rb
@@ -0,0 +1,31 @@
+# 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 f3fb88474a4..aebc24dd9a2 100644
--- a/spec/requests/api/commits_spec.rb
+++ b/spec/requests/api/commits_spec.rb
@@ -278,6 +278,12 @@ 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_any_instance_of(::UsageCounters).not_to receive(:increment_counters)
+      end
+
       it 'a new file in project repo' do
         post api(url, user), valid_c_params
 
diff --git a/spec/services/projects/usage_counters_increment_service_spec.rb b/spec/services/projects/usage_counters_increment_service_spec.rb
new file mode 100644
index 00000000000..d82a7337665
--- /dev/null
+++ b/spec/services/projects/usage_counters_increment_service_spec.rb
@@ -0,0 +1,27 @@
+# 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

From 1e662293e8fc2f2eba9657dd27449e966736a14a Mon Sep 17 00:00:00 2001
From: Tiago Botelho <tiagonbotelho@hotmail.com>
Date: Tue, 2 Oct 2018 16:33:43 +0100
Subject: [PATCH 2/2] 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 -------------------
 .../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 --
 .../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