diff --git a/config/events/gitlab_cli_command_used.yml b/config/events/gitlab_cli_command_used.yml deleted file mode 100644 index b1187c6493da846d948bdb2e101b50d53986bec3..0000000000000000000000000000000000000000 --- a/config/events/gitlab_cli_command_used.yml +++ /dev/null @@ -1,24 +0,0 @@ ---- -description: Command is executed from the GitLab CLI -internal_events: true -action: gitlab_cli_command_used -identifiers: - - project - - namespace - - user -additional_properties: - label: - description: Command used - e.g. 'mr' - property: - description: Subcommand used - e.g. 'view' - command_and_subcommand: - description: Command and subcommand used - e.g. 'mr view' -product_group: code_review -product_categories: - - gitlab_cli -milestone: '18.0' -introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/185183 -tiers: - - free - - premium - - ultimate diff --git a/config/metrics/schema/internal_events.json b/config/metrics/schema/internal_events.json index 6cbb4bbce2ca9dd2e2ecf1b847599cd933d56eb0..018bd5db85442cd05d06c79e0cf101c032ffc707 100644 --- a/config/metrics/schema/internal_events.json +++ b/config/metrics/schema/internal_events.json @@ -54,6 +54,7 @@ "operator": { "type": "string", "enum": [ + "total", "sum(value)" ] } diff --git a/lib/gitlab/internal_events.rb b/lib/gitlab/internal_events.rb index ca3e3a7334153890c20bc7886e752fe3d3957b53..c2e9b18b7884fb812ca9a891ac628988723528fc 100644 --- a/lib/gitlab/internal_events.rb +++ b/lib/gitlab/internal_events.rb @@ -11,6 +11,7 @@ class << self include Gitlab::Utils::StrongMemoize include Gitlab::UsageDataCounters::RedisCounter include Gitlab::UsageDataCounters::RedisSum + include Gitlab::UsageDataCounters::RedisHashCounter def track_event(event_name, category: nil, additional_properties: {}, **kwargs) unless Gitlab::Tracking::EventDefinition.internal_event_exists?(event_name) @@ -75,7 +76,9 @@ def update_redis_values(event_name, additional_properties, kwargs) next unless matches_filter - if event_selection_rule.total_counter? + if event_selection_rule.unique_total? + update_unique_hash_totals_counter(event_selection_rule, **kwargs, **additional_properties) + elsif event_selection_rule.total_counter? update_total_counter(event_selection_rule) elsif event_selection_rule.sum? update_sums(event_selection_rule, **kwargs, **additional_properties) @@ -106,6 +109,15 @@ def update_sums(event_selection_rule, properties) increment_sum_by(event_selection_rule.redis_key_for_date, properties[:value], expiry: expiry) end + def update_unique_hash_totals_counter(event_selection_rule, properties) + return unless properties.has_key?(event_selection_rule.unique_identifier_name) + + value = properties[event_selection_rule.unique_identifier_name] + expiry = event_selection_rule.time_framed? ? KEY_EXPIRY_LENGTH : nil + + hash_increment(event_selection_rule.redis_key_for_date, value, expiry: expiry) + end + def update_unique_counter(event_selection_rule, properties) identifier_name = event_selection_rule.unique_identifier_name diff --git a/lib/gitlab/usage/event_selection_rule.rb b/lib/gitlab/usage/event_selection_rule.rb index ccdae165d6b11cffdec824ebf83f00408860e356..83e0631a7f78bf6ecdd7d86c74157237fcbd4cc2 100644 --- a/lib/gitlab/usage/event_selection_rule.rb +++ b/lib/gitlab/usage/event_selection_rule.rb @@ -51,7 +51,11 @@ def total_counter? end def sum? - operator.present? + operator == 'sum(value)' + end + + def unique_total? + unique_identifier_name && operator == 'total' end def matches?(additional_properties) @@ -114,7 +118,7 @@ def time_constraint(time_frame) def path_part_of_redis_key path = name - if sum? + if operator.present? # operator should be serialized and used in the path here path = "#{path}-operator:#{operator}" end diff --git a/lib/gitlab/usage/metric_definition.rb b/lib/gitlab/usage/metric_definition.rb index 987ee1b51e9034a61321df2b2dc3366f211d10ee..0d1d3ef2412bc30782177d3bfbb607ecb3769242 100644 --- a/lib/gitlab/usage/metric_definition.rb +++ b/lib/gitlab/usage/metric_definition.rb @@ -44,13 +44,12 @@ def event_selection_rules end def instrumentation_class - if internal_events? - return "TotalSumMetric" if event_selection_rules.first&.sum? + return @attributes[:instrumentation_class] if @attributes[:instrumentation_class] + return unless internal_events? + return "TotalSumMetric" if event_selection_rules.first&.sum? + return "UniqueTotalsMetric" if event_selection_rules.first&.unique_total? - events.each_value.first.nil? ? "TotalCountMetric" : "UniqueCountMetric" - else - @attributes[:instrumentation_class] - end + events.each_value.first.nil? ? "TotalCountMetric" : "UniqueCountMetric" end # This method can be removed when the refactoring is complete. It is only here to diff --git a/lib/gitlab/usage/metrics/instrumentations/unique_totals_metric.rb b/lib/gitlab/usage/metrics/instrumentations/unique_totals_metric.rb new file mode 100644 index 0000000000000000000000000000000000000000..cbacebeeaaf3162d338da0f48c32108f4e44d824 --- /dev/null +++ b/lib/gitlab/usage/metrics/instrumentations/unique_totals_metric.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true + +module Gitlab + module Usage + module Metrics + module Instrumentations + # Usage example + # + # In metric YAML definition: + # + # instrumentation_class: UniqueTotalsMetric + # event: + # name: gitlab_cli_command_used + # unique: label + # operator: 'total' + # + class UniqueTotalsMetric < BaseMetric + include Gitlab::UsageDataCounters::RedisHashCounter + + def value + metric_definition.event_selection_rules.to_h do |rule| + keys = rule.redis_keys_for_time_frame(time_frame) + + values = keys.each_with_object({}) do |key, totals| + values_for_key = redis_usage_data { get_hash(key) } + + totals.merge!(values_for_key) { |_, value_1, value_2| value_1 + value_2 } + end + + [rule.unique_identifier_name.to_s, values] + end + end + end + end + end + end +end diff --git a/lib/gitlab/usage_data_counters/redis_hash_counter.rb b/lib/gitlab/usage_data_counters/redis_hash_counter.rb new file mode 100644 index 0000000000000000000000000000000000000000..9672617a712739fd5b3b321e12f60348d9fc5a6a --- /dev/null +++ b/lib/gitlab/usage_data_counters/redis_hash_counter.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +# WARNING: Do not use module directly. Use InternalEvents.track_event instead. +# https://docs.gitlab.com/ee/development/internal_analytics/internal_event_instrumentation/ + +module Gitlab + module UsageDataCounters + module RedisHashCounter + def hash_increment(redis_counter_key, hash_key, expiry: nil) + Gitlab::Redis::SharedState.with do |redis| + redis.hincrby(redis_counter_key, hash_key, 1) + + unless expiry.nil? + existing_expiry = redis.ttl(redis_counter_key) > 0 + redis.expire(redis_counter_key, expiry) unless existing_expiry + end + end + end + + def get_hash(redis_counter_key) + Gitlab::Redis::SharedState.with { |redis| redis.hgetall(redis_counter_key).transform_values(&:to_i) } + end + end + end +end diff --git a/spec/lib/gitlab/internal_events_spec.rb b/spec/lib/gitlab/internal_events_spec.rb index 284022c125a19da5439819f5704d1e1492099e59..a4df90ebd147fdcb28e50c2709e28362c8fed9b3 100644 --- a/spec/lib/gitlab/internal_events_spec.rb +++ b/spec/lib/gitlab/internal_events_spec.rb @@ -10,6 +10,7 @@ allow(Gitlab::AppJsonLogger).to receive(:warn) allow(Gitlab::ErrorTracking).to receive(:track_and_raise_for_dev_exception) allow(redis).to receive(:expire) + allow(redis).to receive(:hincrby) allow(redis).to receive(:incr) allow(redis).to receive(:incrbyfloat) allow(redis).to receive(:multi).and_yield(redis) @@ -42,6 +43,21 @@ def expect_redis_hll_tracking(value_override = nil, property_name_override = nil expect(redis).to have_received(:expire).with(key_expectations, described_class::KEY_EXPIRY_LENGTH) end + def expect_redis_hash_counter_tracking(value_override = nil, property_name_override = nil) + expected_value = value_override || additional_properties[:label] + expected_property_name = property_name_override || :label + + key_expectations = satisfy do |key| + key.include?(event_name) && + key.include?(expected_property_name.to_s) && + key.include?('operator:total') && + key.end_with?(week_suffix) + end + + expect(redis).to have_received(:hincrby).with(key_expectations, expected_value, 1) + expect(redis).to have_received(:ttl).with(key_expectations) + end + def expect_no_redis_hll_tracking expect(redis).not_to have_received(:pfadd) end @@ -791,4 +807,147 @@ def self.track_event(event_name, **kwargs); end end end end + + context 'when unique total counter is defined' do + let(:event_selection_rules) do + [ + Gitlab::Usage::EventSelectionRule.new(name: event_name, time_framed: false), + Gitlab::Usage::EventSelectionRule.new(name: event_name, time_framed: true), + Gitlab::Usage::EventSelectionRule.new( + name: event_name, + time_framed: true, + unique_identifier_name: :label, + operator: 'total' + ) + ] + end + + let(:additional_properties) { { label: 'label_value' } } + + it 'updates Redis hash counter, standard Redis counter and Snowplow', :aggregate_failures do + described_class.track_event( + event_name, + additional_properties: additional_properties, + user: user, + project: project + ) + + expect_redis_tracking + expect_redis_hash_counter_tracking + expect_snowplow_tracking(project.namespace, additional_properties) + end + + context 'when no expiry is needed' do + let(:event_selection_rules) do + [ + Gitlab::Usage::EventSelectionRule.new( + name: event_name, + time_framed: false, + unique_identifier_name: :label, + operator: 'total' + ) + ] + end + + it 'does not set expiry' do + described_class.track_event( + event_name, + additional_properties: additional_properties, + user: user, + project: project + ) + + expect(redis).to have_received(:hincrby).with(a_string_including(event_name), 'label_value', 1) + expect(redis).not_to have_received(:ttl) + expect(redis).not_to have_received(:expire) + end + end + + context 'when property is missing' do + let(:additional_properties) { {} } + + it 'does not update Redis hash counter' do + described_class.track_event( + event_name, + additional_properties: additional_properties, + user: user, + project: project + ) + + expect(redis).not_to have_received(:hincrby) + end + end + + context 'with a filter defined' do + let(:event_selection_rules) do + [ + Gitlab::Usage::EventSelectionRule.new( + name: event_name, + time_framed: true, + unique_identifier_name: :label, + operator: 'total', + filter: { category: 'package' } + ) + ] + end + + context 'when event matches the filter' do + let(:additional_properties) do + { + label: 'label_value', + category: 'package' + } + end + + it 'updates Redis hash counter' do + described_class.track_event( + event_name, + additional_properties: additional_properties, + user: user, + project: project + ) + + expect(redis).to have_received(:hincrby) + end + end + + context 'when event does not match the filter' do + let(:additional_properties) do + { + label: 'label_value', + category: 'not_package' + } + end + + it 'does not update Redis hash counter' do + described_class.track_event( + event_name, + additional_properties: additional_properties, + user: user, + project: project + ) + + expect(redis).not_to have_received(:hincrby) + end + end + end + + context 'when existing TTL is present' do + before do + allow(redis).to receive(:ttl).and_return(1) + end + + it 'does not override the existing expiry' do + described_class.track_event( + event_name, + additional_properties: additional_properties, + user: user, + project: project + ) + + expect(redis).to have_received(:hincrby) + expect(redis).not_to have_received(:expire) + end + end + end end diff --git a/spec/lib/gitlab/usage/metric_definition_spec.rb b/spec/lib/gitlab/usage/metric_definition_spec.rb index 52c26f7bb45758038ce57078a99fd7455e3f2e4f..84236c307af6954b63cf1f3e00c7103d14d90821 100644 --- a/spec/lib/gitlab/usage/metric_definition_spec.rb +++ b/spec/lib/gitlab/usage/metric_definition_spec.rb @@ -78,6 +78,16 @@ def write_metric(metric, path, content) expect(definition.instrumentation_class).to eq('TotalSumMetric') end end + + context 'for uniq sum' do + let(:attributes) do + { key_path: 'metric1', data_source: 'internal_events', events: [{ name: 'a', unique: 'user.id', operator: 'total' }] } + end + + it 'returns UniqueTotalsMetric' do + expect(definition.instrumentation_class).to eq('UniqueTotalsMetric') + end + end end end diff --git a/spec/lib/gitlab/usage/metrics/instrumentations/unique_totals_metric_spec.rb b/spec/lib/gitlab/usage/metrics/instrumentations/unique_totals_metric_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..d61d9185b97b5745a1eab2902581a78303a97026 --- /dev/null +++ b/spec/lib/gitlab/usage/metrics/instrumentations/unique_totals_metric_spec.rb @@ -0,0 +1,63 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Usage::Metrics::Instrumentations::UniqueTotalsMetric, :clean_gitlab_redis_shared_state, + feature_category: :product_analytics do + let(:metric_definition) do + instance_double(Gitlab::Usage::MetricDefinition, event_selection_rules: rules) + end + + let(:rule1) { instance_double(Gitlab::Usage::EventSelectionRule) } + let(:rule2) { instance_double(Gitlab::Usage::EventSelectionRule) } + let(:rules) { [rule1, rule2] } + let(:time_frame) { '28d' } + let(:keys1) { %w[key_2024-19 key_2024-21] } + let(:keys2) { %w[key_2024-20 key_2024-21] } + let(:hash_data1) { { 'label1' => 3, 'label2' => 5 } } + let(:hash_data2) { { 'label2' => 2, 'label3' => 7 } } + + subject(:metric) { described_class.new(metric_definition: metric_definition, time_frame: time_frame) } + + before do + allow(rule1).to receive(:redis_keys_for_time_frame).with(time_frame).and_return(keys1) + allow(rule1).to receive(:unique_identifier_name).and_return(:identifier1) + + allow(rule2).to receive(:redis_keys_for_time_frame).with(time_frame).and_return(keys2) + allow(rule2).to receive(:unique_identifier_name).and_return(:identifier2) + + allow(Gitlab::Usage::MetricDefinition).to receive(:new).and_return(metric_definition) + allow(metric).to receive(:redis_usage_data).and_yield + allow(metric).to receive(:get_hash).with('key_2024-19').and_return(hash_data1) + allow(metric).to receive(:get_hash).with('key_2024-20').and_return(hash_data2) + allow(metric).to receive(:get_hash).with('key_2024-21').and_return({}) + end + + describe '#value' do + it 'returns hash counts for each event selection rule' do + result = metric.value + + expect(result).to be_a(Hash) + expect(result.keys).to contain_exactly('identifier1', 'identifier2') + expect(result['identifier1']).to eq(hash_data1) + expect(result['identifier2']).to eq(hash_data2) + end + + context 'when keys have overlapping hash values' do + before do + allow(metric).to receive(:get_hash).with('key_2024-19').and_return(hash_data1) + allow(metric).to receive(:get_hash).with('key_2024-21').and_return(hash_data2) + end + + it 'sums the values for duplicate hash keys' do + result = metric.value + + expect(result['identifier1']).to eq({ + 'label1' => 3, + 'label2' => 7, + 'label3' => 7 + }) + end + end + end +end diff --git a/spec/lib/gitlab/usage_data_counters/redis_hash_counter_spec.rb b/spec/lib/gitlab/usage_data_counters/redis_hash_counter_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..cc2120c20c40db02518b1424c527be57f0c89b53 --- /dev/null +++ b/spec/lib/gitlab/usage_data_counters/redis_hash_counter_spec.rb @@ -0,0 +1,99 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::UsageDataCounters::RedisHashCounter, :clean_gitlab_redis_shared_state, + feature_category: :application_instrumentation do + let(:redis_key) { 'hash_counter_test' } + let(:hash_key) { 'test_value' } + + subject(:counter) { Class.new.extend(described_class) } + + describe '.hash_increment' do + it 'increments the hash counter' do + expect(counter.get_hash(redis_key)[hash_key]).to be_nil + counter.hash_increment(redis_key, hash_key) + expect(counter.get_hash(redis_key)[hash_key]).to eq(1) + end + + it 'can increment the hash counter multiple times' do + counter.hash_increment(redis_key, hash_key) + counter.hash_increment(redis_key, hash_key) + counter.hash_increment(redis_key, hash_key) + + expect(counter.get_hash(redis_key)[hash_key]).to eq(3) + end + + it 'can increment multiple hash keys in the same redis key' do + counter.hash_increment(redis_key, 'value1') + counter.hash_increment(redis_key, 'value2') + counter.hash_increment(redis_key, 'value1') + + hash = counter.get_hash(redis_key) + expect(hash['value1']).to eq(2) + expect(hash['value2']).to eq(1) + end + + it 'does not have an expiration timestamp by default' do + counter.hash_increment(redis_key, hash_key) + + expect(Gitlab::Redis::SharedState.with { |redis| redis.ttl(redis_key) }).to eq(-1) + end + + context 'when expiry is passed as an argument' do + let(:expiry) { 7.days } + + it 'increments the counter' do + expect do + counter.hash_increment(redis_key, hash_key, expiry: expiry) + end.to change { counter.get_hash(redis_key).fetch(hash_key, 0) }.by(1) + end + + it 'adds an expiration timestamp to the key' do + counter.hash_increment(redis_key, hash_key, expiry: expiry) + + expect(Gitlab::Redis::SharedState.with { |redis| redis.ttl(redis_key) }).to be > 0 + end + + it 'does not reset the expiration timestamp when counter is increased again' do + counter.hash_increment(redis_key, hash_key, expiry: expiry) + + original_expiry = Gitlab::Redis::SharedState.with { |redis| redis.ttl(redis_key) } + + counter.hash_increment(redis_key, hash_key, expiry: 14.days) + + # The expiry should remain the same (the original one) + expect(Gitlab::Redis::SharedState.with { |redis| redis.ttl(redis_key) }).to eq(original_expiry) + end + end + end + + describe '.get_hash' do + before do + counter.hash_increment(redis_key, 'value1') + counter.hash_increment(redis_key, 'value2') + counter.hash_increment(redis_key, 'value1') + end + + it 'returns a hash of all keys and their counts' do + result = counter.get_hash(redis_key) + + expect(result).to be_a(Hash) + expect(result['value1']).to eq(2) + expect(result['value2']).to eq(1) + end + + it 'returns integer values' do + result = counter.get_hash(redis_key) + + expect(result['value1']).to be_a(Integer) + expect(result['value2']).to be_a(Integer) + end + + it 'returns empty hash for non-existent keys' do + result = counter.get_hash('non_existent_key') + + expect(result).to eq({}) + end + end +end diff --git a/spec/support/shared_examples/config/metrics/every_metric_definition_shared_examples.rb b/spec/support/shared_examples/config/metrics/every_metric_definition_shared_examples.rb index 3bc3a4cb2d6a0ebb3ea6aa55cc739535ecf6e6a0..d26719718edcdf49a952293958ef6d85e5249937 100644 --- a/spec/support/shared_examples/config/metrics/every_metric_definition_shared_examples.rb +++ b/spec/support/shared_examples/config/metrics/every_metric_definition_shared_examples.rb @@ -127,6 +127,7 @@ def object_with_schema?(key_path) def assert_uses_all_nested_classes(parent_module) parent_module.constants(false).each do |const_name| next if const_name == :TotalSumMetric # TODO: Remove when first metric is implemented + next if const_name == :UniqueTotalsMetric # TODO: Remove when first metric is implemented constant = parent_module.const_get(const_name, false) next if parent_metric_classes.include?(constant) ||