From e9c42179686dab6e4d732c8a321aadf5fd5f34da Mon Sep 17 00:00:00 2001
From: Jonas Larsen <jlarsen@gitlab.com>
Date: Tue, 28 May 2024 19:08:12 +0000
Subject: [PATCH] Encapsulate attributes in MetricDefinition
---
lib/gitlab/tracking/event_definition.rb | 11 +--
lib/gitlab/usage/metric.rb | 2 +-
lib/gitlab/usage/metric_definition.rb | 78 +++++++++++++++----
lib/service_ping/build_payload.rb | 4 +-
.../gitlab/tracking/event_definition_spec.rb | 2 +-
spec/lib/gitlab/usage/metric_spec.rb | 4 +-
.../gitlab/usage/service_ping_report_spec.rb | 4 +-
.../code_review_events_spec.rb | 8 +-
.../matchers/internal_events_matchers.rb | 2 +-
9 files changed, 76 insertions(+), 39 deletions(-)
diff --git a/lib/gitlab/tracking/event_definition.rb b/lib/gitlab/tracking/event_definition.rb
index 2f3174329c740..fa880e41e4ba7 100644
--- a/lib/gitlab/tracking/event_definition.rb
+++ b/lib/gitlab/tracking/event_definition.rb
@@ -90,15 +90,10 @@ def find_event_selection_rules
{ name: attributes[:action], time_framed?: true, filter: {} }
]
Gitlab::Usage::MetricDefinition.definitions.each_value do |metric_definition|
- metric_definition.attributes[:events]&.each do |event_selection_rule|
- if event_selection_rule[:name] == attributes[:action]
- result << {
- name: attributes[:action],
- time_framed?: %w[7d 28d].include?(metric_definition.attributes[:time_frame]),
- filter: event_selection_rule[:filter] || {}
- }
- end
+ matching_event_selection_rules = metric_definition.event_selection_rules.select do |event_selection_rule|
+ event_selection_rule[:name] == attributes[:action]
end
+ result.concat(matching_event_selection_rules)
end
result.uniq
end
diff --git a/lib/gitlab/usage/metric.rb b/lib/gitlab/usage/metric.rb
index 1efd8ded77cc5..ffa73f799ab3c 100644
--- a/lib/gitlab/usage/metric.rb
+++ b/lib/gitlab/usage/metric.rb
@@ -39,7 +39,7 @@ def unflatten_key_path(value)
def instrumentation_object
instrumentation_class = "Gitlab::Usage::Metrics::Instrumentations::#{definition.instrumentation_class}"
- @instrumentation_object ||= instrumentation_class.constantize.new(definition.attributes)
+ @instrumentation_object ||= instrumentation_class.constantize.new(definition.raw_attributes)
end
end
end
diff --git a/lib/gitlab/usage/metric_definition.rb b/lib/gitlab/usage/metric_definition.rb
index 65a50391c1899..cb3af222b141a 100644
--- a/lib/gitlab/usage/metric_definition.rb
+++ b/lib/gitlab/usage/metric_definition.rb
@@ -11,7 +11,6 @@ class MetricDefinition
InvalidError = Class.new(RuntimeError)
attr_reader :path
- attr_reader :attributes
def initialize(path, opts = {})
@path = path
@@ -19,28 +18,49 @@ def initialize(path, opts = {})
end
def key
- attributes[:key_path]
+ @attributes[:key_path]
end
alias_method :key_path, :key
-
def events
events_from_new_structure || events_from_old_structure || {}
end
+ def event_selection_rules
+ return [] unless @attributes[:events]
+
+ @attributes[:events].map do |event|
+ {
+ name: event[:name],
+ time_framed?: time_framed?,
+ filter: event[:filter] || {}
+ }
+ end
+ end
+
def instrumentation_class
if internal_events?
events.each_value.first.nil? ? "TotalCountMetric" : "RedisHLLMetric"
else
- attributes[:instrumentation_class]
+ @attributes[:instrumentation_class]
end
end
+ # This method can be removed when the refactoring is complete. It is only here to
+ # limit access to @attributes in a gradual manner.
+ def raw_attributes
+ @attributes
+ end
+
def status
- attributes[:status]
+ @attributes[:status]
end
def value_json_schema
- attributes[:value_json_schema]
+ @attributes[:value_json_schema]
+ end
+
+ def value_type
+ @attributes[:value_type]
end
def to_context
@@ -50,7 +70,7 @@ def to_context
end
def to_h
- attributes
+ @attributes
end
def json_schema
@@ -62,15 +82,15 @@ def json_schema
def json_schema_path
return '' unless has_json_schema?
- Rails.root.join(attributes[:value_json_schema])
+ Rails.root.join(@attributes[:value_json_schema])
end
def has_json_schema?
- attributes[:value_type] == 'object' && attributes[:value_json_schema].present?
+ @attributes[:value_type] == 'object' && @attributes[:value_json_schema].present?
end
def validation_errors
- SCHEMA.validate(attributes.deep_stringify_keys).map do |error|
+ SCHEMA.validate(@attributes.deep_stringify_keys).map do |error|
<<~ERROR_MSG
--------------- VALIDATION ERROR ---------------
Metric file: #{path}
@@ -81,16 +101,40 @@ def validation_errors
end
end
+ def product_group
+ @attributes[:product_group]
+ end
+
+ def time_frame
+ @attributes[:time_frame]
+ end
+
+ def time_framed?
+ %w[7d 28d].include?(time_frame)
+ end
+
+ def active?
+ status == 'active'
+ end
+
+ def broken?
+ status == 'broken'
+ end
+
def available?
- AVAILABLE_STATUSES.include?(attributes[:status])
+ AVAILABLE_STATUSES.include?(status)
end
def valid_service_ping_status?
- VALID_SERVICE_PING_STATUSES.include?(attributes[:status])
+ VALID_SERVICE_PING_STATUSES.include?(status)
+ end
+
+ def data_category
+ @attributes[:data_category]
end
def data_source
- attributes[:data_source]
+ @attributes[:data_source]
end
def internal_events?
@@ -113,12 +157,12 @@ def all
end
def not_removed
- all.select { |definition| definition.attributes[:status] != 'removed' }.index_by(&:key_path)
+ all.select { |definition| definition.status != 'removed' }.index_by(&:key_path)
end
def with_instrumentation_class
all.select do |definition|
- (definition.internal_events? || definition.attributes[:instrumentation_class].present?) && definition.available?
+ (definition.internal_events? || definition.instrumentation_class.present?) && definition.available?
end
end
@@ -164,14 +208,14 @@ def load_all_from_path!(definitions, glob_path)
private
def events_from_new_structure
- events = attributes[:events]
+ events = @attributes[:events]
return unless events
events.to_h { |event| [event[:name], event[:unique]&.to_sym] }
end
def events_from_old_structure
- options_events = attributes.dig(:options, :events)
+ options_events = @attributes.dig(:options, :events)
return unless options_events
options_events.index_with { nil }
diff --git a/lib/service_ping/build_payload.rb b/lib/service_ping/build_payload.rb
index 0f19cd55a4cb8..0fffd16795b51 100644
--- a/lib/service_ping/build_payload.rb
+++ b/lib/service_ping/build_payload.rb
@@ -47,9 +47,7 @@ def has_metric_definition?(key_path)
end
def metric_category(key_path)
- metric_definitions[key_path]
- &.attributes
- &.fetch(:data_category, ::ServicePing::PermitDataCategories::OPTIONAL_CATEGORY)
+ metric_definitions[key_path]&.data_category || ::ServicePing::PermitDataCategories::OPTIONAL_CATEGORY
end
def metric_definitions
diff --git a/spec/lib/gitlab/tracking/event_definition_spec.rb b/spec/lib/gitlab/tracking/event_definition_spec.rb
index 6b3d777eae3f9..6eeb49b1de7e4 100644
--- a/spec/lib/gitlab/tracking/event_definition_spec.rb
+++ b/spec/lib/gitlab/tracking/event_definition_spec.rb
@@ -65,7 +65,7 @@ def write_metric(metric, path, content)
it 'has event definitions for all events used in Internal Events metric definitions', :aggregate_failures do
from_metric_definitions = Gitlab::Usage::MetricDefinition.not_removed
.values
- .select { |m| m.attributes[:data_source] == 'internal_events' }
+ .select(&:internal_events?)
.flat_map { |m| m.events&.keys }
.compact
.uniq
diff --git a/spec/lib/gitlab/usage/metric_spec.rb b/spec/lib/gitlab/usage/metric_spec.rb
index 42d2f394ce3de..8cb052421f981 100644
--- a/spec/lib/gitlab/usage/metric_spec.rb
+++ b/spec/lib/gitlab/usage/metric_spec.rb
@@ -25,7 +25,7 @@
let(:issue_count_metric_definiton) do
double(:issue_count_metric_definiton,
- attributes.merge({ attributes: attributes })
+ attributes.merge({ raw_attributes: attributes })
)
end
@@ -49,7 +49,7 @@
let(:instrumentation_class) { "UnavailableMetric" }
let(:issue_count_metric_definiton) do
double(:issue_count_metric_definiton,
- attributes.merge({ attributes: attributes, instrumentation_class: instrumentation_class })
+ attributes.merge({ raw_attributes: attributes, instrumentation_class: instrumentation_class })
)
end
diff --git a/spec/lib/gitlab/usage/service_ping_report_spec.rb b/spec/lib/gitlab/usage/service_ping_report_spec.rb
index 098661986395f..2890522306c94 100644
--- a/spec/lib/gitlab/usage/service_ping_report_spec.rb
+++ b/spec/lib/gitlab/usage/service_ping_report_spec.rb
@@ -143,7 +143,7 @@ def build_payload_from_queries(payload, accumulator = [], key_path = [])
end
def type_cast_to_defined_type(value, metric_definition)
- case metric_definition&.attributes&.fetch(:value_type)
+ case metric_definition&.value_type
when "string"
value.to_s
when "number"
@@ -181,7 +181,7 @@ def type_cast_to_defined_type(value, metric_definition)
metric_definition = metric_definitions[key_path.join('.')]
# Skip broken metrics since they are usually overriden to return -1
- next if metric_definition&.attributes&.fetch(:status) == 'broken'
+ next if metric_definition&.broken?
value = type_cast_to_defined_type(value, metric_definition)
payload_value = service_ping_payload.dig(*key_path)
diff --git a/spec/lib/gitlab/usage_data_counters/code_review_events_spec.rb b/spec/lib/gitlab/usage_data_counters/code_review_events_spec.rb
index 61e78c1c27f6d..434506d40bf14 100644
--- a/spec/lib/gitlab/usage_data_counters/code_review_events_spec.rb
+++ b/spec/lib/gitlab/usage_data_counters/code_review_events_spec.rb
@@ -20,8 +20,8 @@
]
all_code_review_events = Gitlab::Usage::MetricDefinition.all.flat_map do |definition|
- next [] unless definition.attributes[:key_path].include?('.code_review.') &&
- definition.attributes[:status] == 'active' &&
+ next [] unless definition.key_path.include?('.code_review.') &&
+ definition.active? &&
definition.events.count == 1
definition.events.keys
@@ -37,8 +37,8 @@
end
def code_review_aggregated_metric?(definition)
- definition.attributes[:product_group] == 'code_review' &&
- definition.attributes[:status] == 'active' &&
+ definition.product_group == 'code_review' &&
+ definition.active? &&
definition.events.count > 1
end
end
diff --git a/spec/support/matchers/internal_events_matchers.rb b/spec/support/matchers/internal_events_matchers.rb
index 1fac2e185b22b..29b9a20be8eb0 100644
--- a/spec/support/matchers/internal_events_matchers.rb
+++ b/spec/support/matchers/internal_events_matchers.rb
@@ -384,7 +384,7 @@ def build_matcher
# to be passed to a change matcher
def metric_value_tracker(key_path, metric_definition)
proc do
- stub_usage_data_connections if metric_definition.attributes[:data_source] == 'database'
+ stub_usage_data_connections if metric_definition.data_source == 'database'
metric = Gitlab::Usage::Metric.new(metric_definition)
instrumentation_object = stub_metric_timeframe(metric)
--
GitLab