From 64a7524abb7a3aeb64078c8334da15b3a9786f71 Mon Sep 17 00:00:00 2001
From: Niko Belokolodov <nbelokolodov@gitlab.com>
Date: Wed, 24 Jul 2024 22:04:19 +0000
Subject: [PATCH] Allow more than 3 additional properties in Internal Events
---
config/events/schema.json | 2 +-
lib/api/helpers.rb | 2 +-
lib/api/usage_data.rb | 11 +--
lib/gitlab/internal_events.rb | 32 ++++---
lib/gitlab/tracking/event_definition.rb | 16 ----
.../tracking/event_definition_validator.rb | 60 +++++++++++++
scripts/internal_events/monitor.rb | 9 +-
scripts/verify-tff-mapping | 6 +-
spec/lib/gitlab/internal_events_spec.rb | 34 +++++---
.../gitlab/tracking/event_definition_spec.rb | 27 ------
.../event_definition_validator_spec.rb | 85 +++++++++++++++++++
...definition_validator_validate_all_spec.rb} | 2 +-
spec/requests/api/usage_data_spec.rb | 3 +-
.../internal_event_tracking_examples.rb | 7 +-
tests.yml | 2 +-
15 files changed, 205 insertions(+), 93 deletions(-)
create mode 100644 lib/gitlab/tracking/event_definition_validator.rb
create mode 100644 spec/lib/gitlab/tracking/event_definition_validator_spec.rb
rename spec/lib/gitlab/tracking/{event_definition_validate_all_spec.rb => event_definition_validator_validate_all_spec.rb} (77%)
diff --git a/config/events/schema.json b/config/events/schema.json
index 8b0f89fbd1b44..eb1c37d332c70 100644
--- a/config/events/schema.json
+++ b/config/events/schema.json
@@ -86,7 +86,7 @@
"additionalProperties": false
}
},
- "additionalProperties": false
+ "additionalProperties": true
},
"iglu_schema_url": {
"type": [
diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb
index a5ec50cc9b694..421cdba499e54 100644
--- a/lib/api/helpers.rb
+++ b/lib/api/helpers.rb
@@ -719,7 +719,7 @@ def increment_unique_values(event_name, values)
Gitlab::AppLogger.warn("Redis tracking event failed for event: #{event_name}, message: #{error.message}")
end
- def track_event(event_name, user:, send_snowplow_event: true, namespace_id: nil, project_id: nil, additional_properties: Gitlab::InternalEvents::DEFAULT_ADDITIONAL_PROPERTIES)
+ def track_event(event_name, user:, send_snowplow_event: true, namespace_id: nil, project_id: nil, additional_properties: {})
return unless user.present?
namespace = Namespace.find(namespace_id) if namespace_id
diff --git a/lib/api/usage_data.rb b/lib/api/usage_data.rb
index c157545304c03..f5c3df17df587 100644
--- a/lib/api/usage_data.rb
+++ b/lib/api/usage_data.rb
@@ -95,29 +95,26 @@ class UsageData < ::API::Base
documentation: { example: 1234 }
optional :project_id, type: Integer, desc: 'Project ID',
documentation: { example: 1234 }
+ optional :additional_properties, type: Hash, desc: 'Additional properties to be tracked',
+ documentation: { example: { label: 'login_button', value: 1 } }
end
post 'track_event', urgency: :low do
event_name = params[:event]
namespace_id = params[:namespace_id]
project_id = params[:project_id]
- additional_properties = params
- .fetch(:additional_properties, Gitlab::InternalEvents::DEFAULT_ADDITIONAL_PROPERTIES)
- .symbolize_keys
+ additional_properties = params.fetch(:additional_properties, {}).symbolize_keys
unless Gitlab::Tracking::AiTracking.track_via_code_suggestions?(event_name, current_user)
Gitlab::Tracking::AiTracking.track_event(event_name, additional_properties.merge(user: current_user))
end
- internal_event_additional_props = additional_properties
- .slice(*Gitlab::InternalEvents::ALLOWED_ADDITIONAL_PROPERTIES.keys)
-
track_event(
event_name,
send_snowplow_event: false,
user: current_user,
namespace_id: namespace_id,
project_id: project_id,
- additional_properties: internal_event_additional_props
+ additional_properties: additional_properties
)
status :ok
diff --git a/lib/gitlab/internal_events.rb b/lib/gitlab/internal_events.rb
index 25616406fbe87..c18b82460d8e6 100644
--- a/lib/gitlab/internal_events.rb
+++ b/lib/gitlab/internal_events.rb
@@ -8,12 +8,11 @@ module InternalEvents
SNOWPLOW_EMITTER_BUFFER_SIZE = 100
DEFAULT_BUFFER_SIZE = 1
- ALLOWED_ADDITIONAL_PROPERTIES = {
+ BASE_ADDITIONAL_PROPERTIES = {
label: [String],
property: [String],
value: [Integer, Float]
}.freeze
- DEFAULT_ADDITIONAL_PROPERTIES = {}.freeze
KEY_EXPIRY_LENGTH = Gitlab::UsageDataCounters::HLLRedisCounter::KEY_EXPIRY_LENGTH
class << self
@@ -23,7 +22,10 @@ class << self
def track_event(
event_name, category: nil, send_snowplow_event: true,
- additional_properties: DEFAULT_ADDITIONAL_PROPERTIES, **kwargs)
+ additional_properties: {}, **kwargs)
+
+ extra = custom_additional_properties(additional_properties)
+ additional_properties = additional_properties.slice(*BASE_ADDITIONAL_PROPERTIES.keys)
unless Gitlab::Tracking::EventDefinition.internal_event_exists?(event_name)
raise UnknownEventError, "Unknown event: #{event_name}"
@@ -35,7 +37,7 @@ def track_event(
kwargs[:namespace] ||= project.namespace if project
update_redis_values(event_name, additional_properties, kwargs)
- trigger_snowplow_event(event_name, category, additional_properties, kwargs) if send_snowplow_event
+ trigger_snowplow_event(event_name, category, additional_properties, extra, kwargs) if send_snowplow_event
send_application_instrumentation_event(event_name, additional_properties, kwargs) if send_snowplow_event
if Feature.enabled?(:early_access_program, kwargs[:user], type: :wip)
@@ -73,17 +75,8 @@ def validate_property!(hash, key, *class_names)
end
def validate_additional_properties!(additional_properties)
- return if additional_properties.empty?
-
- disallowed_properties = additional_properties.keys - ALLOWED_ADDITIONAL_PROPERTIES.keys
- unless disallowed_properties.empty?
- info = "Additional properties should include only #{ALLOWED_ADDITIONAL_PROPERTIES.keys}. " \
- "Disallowed properties found: #{disallowed_properties}"
- raise InvalidPropertyError, info
- end
-
- additional_properties.each do |key, _value|
- allowed_classes = ALLOWED_ADDITIONAL_PROPERTIES[key]
+ BASE_ADDITIONAL_PROPERTIES.keys.intersection(additional_properties.keys).each do |key|
+ allowed_classes = BASE_ADDITIONAL_PROPERTIES[key]
validate_property!(additional_properties, key, *allowed_classes)
end
end
@@ -106,6 +99,10 @@ def update_redis_values(event_name, additional_properties, kwargs)
end
end
+ def custom_additional_properties(additional_properties)
+ additional_properties.except(*BASE_ADDITIONAL_PROPERTIES.keys)
+ end
+
def update_total_counter(event_selection_rule)
expiry = event_selection_rule.time_framed? ? KEY_EXPIRY_LENGTH : nil
@@ -132,7 +129,7 @@ def update_unique_counter(event_selection_rule, kwargs)
)
end
- def trigger_snowplow_event(event_name, category, additional_properties, kwargs)
+ def trigger_snowplow_event(event_name, category, additional_properties, extra, kwargs)
user = kwargs[:user]
project = kwargs[:project]
namespace = kwargs[:namespace]
@@ -143,7 +140,8 @@ def trigger_snowplow_event(event_name, category, additional_properties, kwargs)
user_id: user&.id,
namespace_id: namespace&.id,
plan_name: namespace&.actual_plan_name,
- feature_enabled_by_namespace_ids: feature_enabled_by_namespace_ids
+ feature_enabled_by_namespace_ids: feature_enabled_by_namespace_ids,
+ **extra
).to_context
service_ping_context = Tracking::ServicePingContext.new(
diff --git a/lib/gitlab/tracking/event_definition.rb b/lib/gitlab/tracking/event_definition.rb
index e8a6556e4a774..50beba6433c81 100644
--- a/lib/gitlab/tracking/event_definition.rb
+++ b/lib/gitlab/tracking/event_definition.rb
@@ -5,9 +5,6 @@ module Tracking
InvalidEventError = Class.new(RuntimeError)
class EventDefinition
- EVENT_SCHEMA_PATH = Rails.root.join('config', 'events', 'schema.json')
- SCHEMA = ::JSONSchemer.schema(EVENT_SCHEMA_PATH)
-
attr_reader :path
attr_reader :attributes
@@ -65,19 +62,6 @@ def yaml_path
path.delete_prefix(Rails.root.to_s)
end
- def validation_errors
- SCHEMA.validate(attributes.deep_stringify_keys).map do |error|
- <<~ERROR_MSG
- --------------- VALIDATION ERROR ---------------
- Definition file: #{path}
- Error type: #{error['type']}
- Data: #{error['data']}
- Path: #{error['data_pointer']}
- Details: #{error['details']}
- ERROR_MSG
- end
- end
-
def event_selection_rules
@event_selection_rules ||= find_event_selection_rules
end
diff --git a/lib/gitlab/tracking/event_definition_validator.rb b/lib/gitlab/tracking/event_definition_validator.rb
new file mode 100644
index 0000000000000..ce952987c501b
--- /dev/null
+++ b/lib/gitlab/tracking/event_definition_validator.rb
@@ -0,0 +1,60 @@
+# frozen_string_literal: true
+
+module Gitlab
+ module Tracking
+ class EventDefinitionValidator
+ EVENT_SCHEMA_PATH = Rails.root.join('config/events/schema.json')
+ SCHEMA = ::JSONSchemer.schema(EVENT_SCHEMA_PATH)
+
+ def initialize(definition)
+ @attributes = definition.attributes
+ @path = definition.path
+ end
+
+ def validation_errors
+ (validate_schema << validate_additional_properties).compact
+ end
+
+ private
+
+ attr_reader :attributes, :path
+
+ def validate_additional_properties
+ additional_props = attributes[:additional_properties]&.map { |k, _| k }
+
+ return unless additional_props.present?
+
+ extra_props = additional_props - main_additional_properties
+
+ return unless extra_props.count == additional_props.count
+
+ # if additional properties were not used
+
+ <<~ERROR_MSG
+ --------------- VALIDATION ERROR ---------------
+ Definition file: #{path}
+ Error type: consider using the built-in additional properties:
+ "#{main_additional_properties.join(', ')}"
+ before adding custom extra properties: #{extra_props.join(', ')}
+ ERROR_MSG
+ end
+
+ def main_additional_properties
+ Gitlab::InternalEvents::BASE_ADDITIONAL_PROPERTIES.keys - [:value]
+ end
+
+ def validate_schema
+ SCHEMA.validate(attributes.deep_stringify_keys).map do |error|
+ <<~ERROR_MSG
+ --------------- VALIDATION ERROR ---------------
+ Definition file: #{path}
+ Error type: #{error['type']}
+ Data: #{error['data']}
+ Path: #{error['data_pointer']}
+ Details: #{error['details']}
+ ERROR_MSG
+ end
+ end
+ end
+ end
+end
diff --git a/scripts/internal_events/monitor.rb b/scripts/internal_events/monitor.rb
index a1a455436e133..83ea3edf539e6 100644
--- a/scripts/internal_events/monitor.rb
+++ b/scripts/internal_events/monitor.rb
@@ -54,7 +54,8 @@ def extract_standard_context(event)
user_id: context["data"]["user_id"],
namespace_id: context["data"]["namespace_id"],
project_id: context["data"]["project_id"],
- plan: context["data"]["plan"]
+ plan: context["data"]["plan"],
+ extra: context["data"]["extra"]
}
end
{}
@@ -75,7 +76,8 @@ def generate_snowplow_table
'plan',
'Label',
'Property',
- 'Value'
+ 'Value',
+ 'Extra'
]
rows << :separator
@@ -93,7 +95,8 @@ def generate_snowplow_table
standard_context[:plan],
event['event']['se_label'],
event['event']['se_property'],
- event['event']['se_value']
+ event['event']['se_value'],
+ standard_context[:extra]
]
row.map! { |value| red(value) } if event['rawEvent']['parameters']['dtm'].to_i > @initial_max_timestamp
diff --git a/scripts/verify-tff-mapping b/scripts/verify-tff-mapping
index 772011ab30127..6e3b81a349a6a 100755
--- a/scripts/verify-tff-mapping
+++ b/scripts/verify-tff-mapping
@@ -100,17 +100,17 @@ tests = [
{
explanation: 'FOSS Snowplow definitions map to event validation spec',
changed_file: 'config/events/status_page_incident_unpublished.yml',
- expected: ['spec/lib/gitlab/tracking/event_definition_validate_all_spec.rb']
+ expected: ['spec/lib/gitlab/tracking/event_definition_validator_validate_all_spec.rb']
},
{
explanation: 'EE Snowplow definitions map to event validation spec',
changed_file: 'ee/config/events/licenses_list_viewed.yml',
- expected: ['spec/lib/gitlab/tracking/event_definition_validate_all_spec.rb']
+ expected: ['spec/lib/gitlab/tracking/event_definition_validator_validate_all_spec.rb']
},
{
explanation: 'Snowplow schema maps to event validation spec',
changed_file: 'config/events/schema.json',
- expected: ['spec/lib/gitlab/tracking/event_definition_validate_all_spec.rb']
+ expected: ['spec/lib/gitlab/tracking/event_definition_validator_validate_all_spec.rb']
},
{
diff --git a/spec/lib/gitlab/internal_events_spec.rb b/spec/lib/gitlab/internal_events_spec.rb
index c85dd8f638aba..1b8c855028ed3 100644
--- a/spec/lib/gitlab/internal_events_spec.rb
+++ b/spec/lib/gitlab/internal_events_spec.rb
@@ -60,7 +60,7 @@ def expect_redis_tracking
end
end
- def expect_snowplow_tracking(expected_namespace = nil, expected_additional_properties = {})
+ def expect_snowplow_tracking(expected_namespace = nil, expected_additional_properties = {}, extra: {})
service_ping_context = Gitlab::Tracking::ServicePingContext
.new(data_source: :redis_hll, event: event_name)
.to_context
@@ -81,7 +81,7 @@ def expect_snowplow_tracking(expected_namespace = nil, expected_additional_prope
c[:schema] == Gitlab::Tracking::StandardContext::GITLAB_STANDARD_SCHEMA_URL
end
- validate_standard_context(standard_context, expected_namespace)
+ validate_standard_context(standard_context, expected_namespace, extra)
# Verify Service Ping context
service_ping_context = contexts.find { |c| c[:schema] == Gitlab::Tracking::ServicePingContext::SCHEMA_URL }
@@ -90,12 +90,13 @@ def expect_snowplow_tracking(expected_namespace = nil, expected_additional_prope
end
end
- def validate_standard_context(standard_context, expected_namespace)
+ def validate_standard_context(standard_context, expected_namespace, extra)
namespace = expected_namespace || project&.namespace
expect(standard_context).not_to eq(nil)
expect(standard_context[:data][:user_id]).to eq(user&.id)
expect(standard_context[:data][:namespace_id]).to eq(namespace&.id)
expect(standard_context[:data][:project_id]).to eq(project&.id)
+ expect(standard_context[:data][:extra]).to eq(extra)
end
def validate_service_ping_context(service_ping_context)
@@ -192,6 +193,23 @@ def validate_service_ping_context(service_ping_context)
expect_snowplow_tracking(nil, additional_properties)
end
+ context 'with a custom property' do
+ let(:custom_properties) do
+ additional_properties.merge(custom: 'custom_property')
+ end
+
+ it 'is sent to Snowplow' do
+ described_class.track_event(
+ event_name,
+ additional_properties: custom_properties,
+ user: user,
+ project: project
+ )
+
+ expect_snowplow_tracking(nil, additional_properties, extra: { custom: 'custom_property' })
+ end
+ end
+
context 'when a filter is defined' do
let(:time_framed) { true }
let(:event_selection_rules) do
@@ -377,16 +395,6 @@ def validate_service_ping_context(service_ping_context)
end
end
end
-
- context "when disallowed additional properties are passed" do
- let(:error_class) { described_class::InvalidPropertyError }
- let(:additional_properties) { { new_property: 'value' } }
-
- it_behaves_like 'an event that logs an error' do
- let(:event_kwargs) { { user: user } }
- let(:logged_kwargs) { { user: user.id } }
- end
- end
end
it 'updates Redis, RedisHLL and Snowplow', :aggregate_failures do
diff --git a/spec/lib/gitlab/tracking/event_definition_spec.rb b/spec/lib/gitlab/tracking/event_definition_spec.rb
index 1fee6ac2451b1..7a7a913a39396 100644
--- a/spec/lib/gitlab/tracking/event_definition_spec.rb
+++ b/spec/lib/gitlab/tracking/event_definition_spec.rb
@@ -76,33 +76,6 @@ def write_metric(metric, path, content)
end
end
- describe '#validate' do
- using RSpec::Parameterized::TableSyntax
-
- where(:attribute, :value) do
- :description | 1
- :category | nil
- :action | nil
- :label_description | 1
- :property_description | 1
- :value_description | 1
- :extra_properties | 'smth'
- :product_group | nil
- :distributions | %(be eb)
- :tiers | %(pro)
- end
-
- with_them do
- before do
- attributes[attribute] = value
- end
-
- it 'has validation errors' do
- expect(described_class.new(path, attributes).validation_errors).not_to be_empty
- end
- end
- end
-
describe '.definitions' do
let(:metric1) { Dir.mktmpdir('metric1') }
let(:metric2) { Dir.mktmpdir('metric2') }
diff --git a/spec/lib/gitlab/tracking/event_definition_validator_spec.rb b/spec/lib/gitlab/tracking/event_definition_validator_spec.rb
new file mode 100644
index 0000000000000..12946ab1770e2
--- /dev/null
+++ b/spec/lib/gitlab/tracking/event_definition_validator_spec.rb
@@ -0,0 +1,85 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::Tracking::EventDefinitionValidator, feature_category: :service_ping do
+ let(:attributes) do
+ {
+ description: 'Created issues',
+ category: 'issues',
+ action: 'create',
+ label_description: 'API',
+ property_description: 'The string "issue_id"',
+ value_description: 'ID of the issue',
+ extra_properties: { confidential: false },
+ product_group: 'group::product analytics',
+ distributions: %w[ee ce],
+ tiers: %w[free premium ultimate],
+ introduced_by_url: "https://gitlab.com/example/-/merge_requests/123",
+ milestone: '1.6'
+ }
+ end
+
+ let(:path) { File.join('events', 'issues_create.yml') }
+ let(:definition) { Gitlab::Tracking::EventDefinition.new(path, attributes) }
+ # let(:yaml_content) { attributes.deep_stringify_keys.to_yaml }
+
+ describe '#validate' do
+ using RSpec::Parameterized::TableSyntax
+
+ where(:attribute, :value) do
+ :description | 1
+ :category | nil
+ :action | nil
+ :label_description | 1
+ :property_description | 1
+ :value_description | 1
+ :extra_properties | 'smth'
+ :product_group | nil
+ :distributions | %(be eb)
+ :tiers | %(pro)
+ end
+
+ with_them do
+ before do
+ attributes[attribute] = value
+ end
+
+ it 'has validation errors' do
+ expect(described_class.new(definition).validation_errors).not_to be_empty
+ end
+ end
+
+ describe 'internal event additional_properties' do
+ let(:attributes) do
+ {
+ description: 'Created issues',
+ category: 'issues',
+ action: 'create',
+ internal_events: true,
+ product_group: 'activation',
+ introduced_by_url: "https://gitlab.com/example/-/merge_requests/123",
+ distributions: %w[ce],
+ milestone: "1.0",
+ tiers: %w[free],
+ additional_properties: {
+ label: { description: "desc" },
+ property: { description: "desc" }
+ }
+ }
+ end
+
+ it 'valid when extra added in addition to the built in' do
+ attributes[:additional_properties][:extra] = { description: "desc" }
+
+ expect(described_class.new(definition).validation_errors).to be_empty
+ end
+
+ it 'invalid when extra added with no built in used' do
+ attributes[:additional_properties] = { extra: { description: "desc" } }
+
+ expect(described_class.new(definition).validation_errors).not_to be_empty
+ end
+ end
+ end
+end
diff --git a/spec/lib/gitlab/tracking/event_definition_validate_all_spec.rb b/spec/lib/gitlab/tracking/event_definition_validator_validate_all_spec.rb
similarity index 77%
rename from spec/lib/gitlab/tracking/event_definition_validate_all_spec.rb
rename to spec/lib/gitlab/tracking/event_definition_validator_validate_all_spec.rb
index cc2ccc511bb38..c39cd9f0af4f1 100644
--- a/spec/lib/gitlab/tracking/event_definition_validate_all_spec.rb
+++ b/spec/lib/gitlab/tracking/event_definition_validator_validate_all_spec.rb
@@ -5,7 +5,7 @@
RSpec.describe Gitlab::Tracking::EventDefinition, feature_category: :product_analytics_data_management do
it 'only has valid event definitions', :aggregate_failures do
described_class.definitions.each do |definition|
- validation_errors = definition.validation_errors
+ validation_errors = Gitlab::Tracking::EventDefinitionValidator.new(definition).validation_errors
expect(validation_errors).to be_empty, validation_errors.join
end
end
diff --git a/spec/requests/api/usage_data_spec.rb b/spec/requests/api/usage_data_spec.rb
index 9ddd79b2db0d1..8a5619195fd5e 100644
--- a/spec/requests/api/usage_data_spec.rb
+++ b/spec/requests/api/usage_data_spec.rb
@@ -208,7 +208,8 @@
let_it_be(:additional_properties) do
{
label: 'label3',
- property: 'admin'
+ property: 'admin',
+ lang: 'ruby'
}
end
diff --git a/spec/support/shared_examples/controllers/internal_event_tracking_examples.rb b/spec/support/shared_examples/controllers/internal_event_tracking_examples.rb
index 0c808d1ebde73..7fb134d4345dc 100644
--- a/spec/support/shared_examples/controllers/internal_event_tracking_examples.rb
+++ b/spec/support/shared_examples/controllers/internal_event_tracking_examples.rb
@@ -11,11 +11,13 @@
# - label
# - property
# - value
+# - additional_properties
# - event_attribute_overrides
RSpec.shared_examples 'internal event tracking' do
let(:all_metrics) do
- additional_properties = Gitlab::InternalEvents::ALLOWED_ADDITIONAL_PROPERTIES.to_h do |key, _val|
+ additional_properties = try(:additional_properties) || {}
+ base_additional_properties = Gitlab::InternalEvents::BASE_ADDITIONAL_PROPERTIES.to_h do |key, _val|
[key, try(key)]
end
@@ -26,7 +28,8 @@
# Only include unique metrics if the unique_identifier_name is present in the spec
next if event_selection_rule.unique_identifier_name && !try(event_selection_rule.unique_identifier_name)
- event_selection_rule.matches?(additional_properties)
+ properties = additional_properties.merge(base_additional_properties)
+ event_selection_rule.matches?(properties)
end
definition.key if matching_rules.flatten.any?
diff --git a/tests.yml b/tests.yml
index 24533a92f720e..0e76f9a8ac61e 100644
--- a/tests.yml
+++ b/tests.yml
@@ -53,7 +53,7 @@ mapping:
- source:
- '(?<prefix>ee/)?config/events/.+\.yml'
- 'config/events/schema\.json'
- test: 'spec/lib/gitlab/tracking/event_definition_validate_all_spec.rb'
+ test: 'spec/lib/gitlab/tracking/event_definition_validator_validate_all_spec.rb'
# DB structure should map to schema spec
- source: 'db/structure\.sql'
--
GitLab