diff --git a/config/feature_flags/gitlab_com_derisk/internal_events_automatic_property_mapping.yml b/config/feature_flags/gitlab_com_derisk/internal_events_automatic_property_mapping.yml new file mode 100644 index 0000000000000000000000000000000000000000..14b777e57c26e361ee0a6d4ca01617b4f2b36d2c --- /dev/null +++ b/config/feature_flags/gitlab_com_derisk/internal_events_automatic_property_mapping.yml @@ -0,0 +1,8 @@ +--- +name: internal_events_automatic_property_mapping +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/202563 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/565697 +milestone: '18.4' +type: gitlab_com_derisk +group: group::product analytics +default_enabled: false diff --git a/lib/gitlab/internal_events.rb b/lib/gitlab/internal_events.rb index fb32b3d1b5d7b0ae23953e431075c177191acd3d..237fc1e0d6393b40e275af6e132680f74ab67bbe 100644 --- a/lib/gitlab/internal_events.rb +++ b/lib/gitlab/internal_events.rb @@ -26,7 +26,7 @@ def track_event(event_name, category: nil, additional_properties: {}, **kwargs) track_analytics_event(event_name, send_snowplow_event, category: category, additional_properties: event_router.public_additional_properties, **kwargs) - event_router.event_definition.extra_trackers.each do |tracking_class, properties| + event_definition(event_name).extra_trackers.each do |tracking_class, properties| next unless tracking_class tracking_class.track_event(event_name, **event_router.extra_tracking_data(properties)) @@ -49,6 +49,10 @@ def track_event(event_name, category: nil, additional_properties: {}, **kwargs) private def track_analytics_event(event_name, send_snowplow_event, category: nil, additional_properties: {}, **kwargs) + if Feature.enabled?(:internal_events_automatic_property_mapping, kwargs[:user]) + map_additional_properties_to_snowplow(event_name, additional_properties) + end + extra = custom_additional_properties(additional_properties) base_additional_properties = additional_properties.slice(*base_additional_properties_keys) @@ -65,8 +69,7 @@ def track_analytics_event(event_name, send_snowplow_event, category: nil, additi end def update_redis_values(event_name, additional_properties, kwargs) - event_definition = Gitlab::Tracking::EventDefinition.find(event_name) - + event_definition = event_definition(event_name) return unless event_definition event_definition.event_selection_rules.each do |event_selection_rule| @@ -214,6 +217,52 @@ def extract_additional_properties!(event_name, additional_properties, kwargs) additional_properties[key] = kwargs.delete(key) if kwargs.key?(key) end end + + def event_definition(event_name) + Gitlab::Tracking::EventDefinition.find(event_name) + end + + def map_additional_properties_to_snowplow(event_name, additional_properties) + definition_props = event_definition(event_name).additional_properties + snowplow_props = [:label, :property] + + # if definition already includes both snowplow props + return if (snowplow_props - definition_props.keys).empty? + + # if definition has only one property and it's one of the snowplow props + return if definition_props.size == 1 && (definition_props.keys & snowplow_props).any? + + # if definition has only one key from snowplow_props, add missing snowplow property + if (definition_props.keys & snowplow_props).size == 1 + add_missing_snowplow_property(definition_props, additional_properties) + # if definition has no props from snowplow_props, add both label and property + elsif (definition_props.keys & snowplow_props).empty? + add_both_snowplow_properties(definition_props, additional_properties) + end + end + + def add_missing_snowplow_property(definition_props, additional_properties) + snowplow_props = [:label, :property] + existing_snowplow_key = (definition_props.keys & snowplow_props).first + non_snowplow_definition_keys = definition_props.keys - snowplow_props + + return unless non_snowplow_definition_keys.any? + + first_non_snowplow = non_snowplow_definition_keys.first + missing_snowplow_key = existing_snowplow_key == :label ? :property : :label + + return unless additional_properties.key?(first_non_snowplow) + + additional_properties[missing_snowplow_key] = additional_properties[first_non_snowplow] + end + + def add_both_snowplow_properties(definition_props, additional_properties) + definition_keys = definition_props.keys + available_keys = definition_keys.select { |key| additional_properties.key?(key) } + + additional_properties[:label] = additional_properties[available_keys[0]] if available_keys.size >= 1 + additional_properties[:property] = additional_properties[available_keys[1]] if available_keys.size >= 2 + end end end end diff --git a/lib/gitlab/tracking/event_definition.rb b/lib/gitlab/tracking/event_definition.rb index ff5607815e2c82b31e324228f3d723f79844f444..8c7c1929ec2382d9abdca8349f83118e90d80647 100644 --- a/lib/gitlab/tracking/event_definition.rb +++ b/lib/gitlab/tracking/event_definition.rb @@ -57,7 +57,7 @@ def initialize(path, opts = {}) end def additional_properties - @attributes.fetch(:additional_properties, {}) + @attributes[:properties] || @attributes[:additional_properties] || {} end def internal_events? diff --git a/spec/lib/gitlab/internal_events_spec.rb b/spec/lib/gitlab/internal_events_spec.rb index 29c82b6170a1e108c6ee7527c3e81c62dc94ad66..055bc609b24620e6b7442373022de80b6ef30f45 100644 --- a/spec/lib/gitlab/internal_events_spec.rb +++ b/spec/lib/gitlab/internal_events_spec.rb @@ -1108,4 +1108,129 @@ def self.track_event(event_name, **kwargs); end end end end + + describe '#map_additional_properties_to_snowplow' do + let(:event_name) { 'mapping_test_event' } + + subject(:map_properties) do + described_class.send(:map_additional_properties_to_snowplow, event_name, additional_properties) + end + + before do + allow(event_definition).to receive(:additional_properties).and_return(definition_properties) + end + + context 'when definition already includes both snowplow props' do + let(:definition_properties) { { label: {}, property: {}, other: {} } } + let(:additional_properties) { { label: 'test', property: 'value', other: 'data' } } + + it 'does not modify the additional_properties' do + expect { map_properties }.not_to change { additional_properties } + end + end + + context 'when definition has only one property and it is a snowplow prop' do + context 'with only label' do + let(:definition_properties) { { label: {} } } + let(:additional_properties) { { label: 'user_action' } } + + it 'does not modify the additional_properties' do + expect { map_properties }.not_to change { additional_properties } + end + end + + context 'with only property' do + let(:definition_properties) { { property: {} } } + let(:additional_properties) { { property: 'click_button' } } + + it 'does not modify the additional_properties' do + expect { map_properties }.not_to change { additional_properties } + end + end + end + + context 'when payload has only one Snowplow property' do + context 'if missing is :property' do + let(:definition_properties) { { label: {}, category: {}, action: {} } } + let(:additional_properties) { { label: 'click', category: 'button', action: 'submit' } } + + it 'adds first non-snowplow key as property' do + map_properties + expect(additional_properties).to eq({ label: 'click', category: 'button', action: 'submit', +property: 'button' }) + end + end + + context 'if missing is :label' do + let(:definition_properties) { { property: {}, category: {}, action: {} } } + let(:additional_properties) { { category: 'button', action: 'submit', property: 'click' } } + + it 'adds first non-snowplow key as label' do + map_properties + expect(additional_properties).to eq({ category: 'button', action: 'submit', property: 'click', +label: 'button' }) + end + end + + context 'when non-snowplow key is not available in additional_properties' do + let(:definition_properties) { { label: {}, category: {}, action: {} } } + let(:additional_properties) { { label: 'click' } } + + it 'does not add missing property when key not available' do + map_properties + expect(additional_properties).to eq({ label: 'click' }) + end + end + end + + context 'when payload has no Snowplow properties' do + let(:definition_properties) { { module: {}, action: {}, context: {} } } + + context 'with all properties available' do + let(:additional_properties) { { module: 'issues', action: 'create', context: 'page' } } + + it 'maps first two keys to label and property' do + map_properties + expect(additional_properties).to eq({ module: 'issues', action: 'create', context: 'page', label: 'issues', +property: 'create' }) + end + end + + context 'with partial properties available' do + let(:additional_properties) { { module: 'issues', context: 'page' } } + + it 'maps only available keys based on definition order' do + map_properties + expect(additional_properties).to eq({ module: 'issues', context: 'page', label: 'issues', property: 'page' }) + end + end + + context 'with only one property available' do + let(:additional_properties) { { action: 'create' } } + + it 'maps only first key to label' do + map_properties + expect(additional_properties).to eq({ action: 'create', label: 'create' }) + end + end + + context 'with no properties available' do + let(:additional_properties) { {} } + + it 'does not modify additional_properties' do + expect { map_properties }.not_to change { additional_properties } + end + end + + context 'when properties exist but not in definition order' do + let(:definition_properties) { { first: {}, second: {}, third: {} } } + let(:additional_properties) { { third: 'value3', first: 'value1' } } + + it 'maps based on definition order not payload order' do + map_properties + expect(additional_properties).to eq({ third: 'value3', first: 'value1', label: 'value1', property: 'value3' }) + end + end + end + end end diff --git a/spec/lib/gitlab/tracking/event_definition_spec.rb b/spec/lib/gitlab/tracking/event_definition_spec.rb index 7fcc6eec733e041b34ee45b22bd02a20be6f934d..82831600c9d708908f35cfd40f05b5fd3915dcbf 100644 --- a/spec/lib/gitlab/tracking/event_definition_spec.rb +++ b/spec/lib/gitlab/tracking/event_definition_spec.rb @@ -213,4 +213,47 @@ def write_metric(metric, path, content) end end end + + describe '#additional_properties' do + context 'when only additional_properties is present' do + let(:attributes) do + super().merge( + additional_properties: { foo: 'bar' } + ) + end + + it 'returns additional_properties' do + expect(definition.additional_properties).to eq({ foo: 'bar' }) + end + end + + context 'when only properties is present' do + let(:attributes) do + super().merge(properties: { baz: 'qux' }) + end + + it 'returns properties' do + expect(definition.additional_properties).to eq({ baz: 'qux' }) + end + end + + context 'when neither additional_properties nor properties are present' do + it 'returns an empty hash' do + expect(definition.additional_properties).to eq({}) + end + end + + context 'when both additional_properties and properties are present' do + let(:attributes) do + super().merge( + additional_properties: { foo: 'bar' }, + properties: { baz: 'qux' } + ) + end + + it 'returns properties' do + expect(definition.additional_properties).to eq({ baz: 'qux' }) + end + end + end end