From 59b09b432b29828247124c3e3ba1a994fe2f0fd3 Mon Sep 17 00:00:00 2001 From: Zach Margolis Date: Wed, 30 Aug 2023 10:53:29 -0700 Subject: [PATCH 01/10] Add custom proc support to FrontendLogger, simplify FrontendLogController error reporting --- app/controllers/frontend_log_controller.rb | 17 +++++------------ app/services/frontend_logger.rb | 11 +++++++++-- .../controllers/frontend_log_controller_spec.rb | 2 +- spec/services/frontend_logger_spec.rb | 15 +++++++++++++++ 4 files changed, 30 insertions(+), 15 deletions(-) diff --git a/app/controllers/frontend_log_controller.rb b/app/controllers/frontend_log_controller.rb index ef4ea773cd7..052d065c07c 100644 --- a/app/controllers/frontend_log_controller.rb +++ b/app/controllers/frontend_log_controller.rb @@ -10,11 +10,10 @@ class FrontendError < StandardError; end # In rare circumstances, these writes can clobber other, more important writes. before_action :skip_session_commit - FRONTEND_ERROR_EVENT = 'Frontend Error'.freeze - # Please try to keep this list alphabetical as well! # rubocop:disable Layout/LineLength EVENT_MAP = { + 'Frontend Error' => proc { |_analytics, payload| NewRelic::Agent.notice_error(FrontendError.new, custom_params: payload) }, 'IdV: consent checkbox toggled' => :idv_consent_checkbox_toggled, 'IdV: download personal key' => :idv_personal_key_downloaded, 'IdV: location submitted' => :idv_in_person_location_submitted, @@ -34,15 +33,13 @@ class FrontendError < StandardError; end 'Sign In: IdV requirements accordion clicked' => :sign_in_idv_requirements_accordion_clicked, 'User prompted before navigation' => :user_prompted_before_navigation, 'User prompted before navigation and still on page' => :user_prompted_before_navigation_and_still_on_page, - }.transform_values { |method| AnalyticsEvents.instance_method(method) }.freeze + }.transform_values do |method| + method.is_a?(Proc) ? method : AnalyticsEvents.instance_method(method) + end.freeze # rubocop:enable Layout/LineLength def create - if error_event? - NewRelic::Agent.notice_error(FrontendError.new, custom_params: log_params[:payload].to_h) - else - frontend_logger.track_event(log_params[:event], log_params[:payload].to_h) - end + frontend_logger.track_event(log_params[:event], log_params[:payload].to_h) render json: { success: true }, status: :ok end @@ -69,10 +66,6 @@ def valid_event? log_params[:event].present? end - def error_event? - log_params[:event] == FRONTEND_ERROR_EVENT - end - def valid_payload? params[:payload].nil? || !log_params[:payload].nil? end diff --git a/app/services/frontend_logger.rb b/app/services/frontend_logger.rb index 6155253e9a1..7b331009ccb 100644 --- a/app/services/frontend_logger.rb +++ b/app/services/frontend_logger.rb @@ -1,14 +1,21 @@ class FrontendLogger attr_reader :analytics, :event_map + # @param [Analytics] + # @param [Hash{String=>UnboundMethod,Proc}] def initialize(analytics:, event_map:) @analytics = analytics @event_map = event_map end + # @param [String] name + # @param [Hash] attributes def track_event(name, attributes) - if (analytics_method = event_map[name]) - analytics.send(analytics_method.name, **hash_from_method_kwargs(attributes, analytics_method)) + case analytics_method = event_map[name] + when Proc + analytics_method.call(analytics, attributes) + when UnboundMethod + analytics_method.bind_call(analytics, **hash_from_method_kwargs(attributes, analytics_method)) else analytics.track_event("Frontend: #{name}", attributes) end diff --git a/spec/controllers/frontend_log_controller_spec.rb b/spec/controllers/frontend_log_controller_spec.rb index d8e4b48a68c..5595e288020 100644 --- a/spec/controllers/frontend_log_controller_spec.rb +++ b/spec/controllers/frontend_log_controller_spec.rb @@ -174,7 +174,7 @@ context 'for an error event' do let(:params) do { - 'event' => described_class::FRONTEND_ERROR_EVENT, + 'event' => 'Frontend Error', 'payload' => { 'name' => 'name', 'message' => 'message', diff --git a/spec/services/frontend_logger_spec.rb b/spec/services/frontend_logger_spec.rb index 01bffa7ce85..c8012d3bebc 100644 --- a/spec/services/frontend_logger_spec.rb +++ b/spec/services/frontend_logger_spec.rb @@ -17,6 +17,9 @@ def example_method_handler(ok:, **rest) let(:event_map) do { 'method' => ExampleAnalyticsEvents.instance_method(:example_method_handler), + 'proc' => proc do |analytics, payload| + analytics.track_event('some customized event', payload.merge('custom' => true)) + end, } end let(:logger) { described_class.new(analytics: analytics, event_map: event_map) } @@ -46,5 +49,17 @@ def example_method_handler(ok:, **rest) expect(analytics).to have_logged_event('example', ok: true, rest: {}) end end + + context 'with proc handler' do + let(:name) { 'proc' } + + it 'calls the method and passes analytics and attributes' do + call + + expect(analytics).to have_logged_event( + 'some customized event', 'ok' => true, 'other' => true, 'custom' => true + ) + end + end end end From e16ab676ea1626df20cc3287547d92331860e963 Mon Sep 17 00:00:00 2001 From: Zach Margolis Date: Wed, 30 Aug 2023 10:57:32 -0700 Subject: [PATCH 02/10] Bring back parens around assignment inside a conditional --- app/services/frontend_logger.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/services/frontend_logger.rb b/app/services/frontend_logger.rb index 7b331009ccb..a367957db0a 100644 --- a/app/services/frontend_logger.rb +++ b/app/services/frontend_logger.rb @@ -11,7 +11,7 @@ def initialize(analytics:, event_map:) # @param [String] name # @param [Hash] attributes def track_event(name, attributes) - case analytics_method = event_map[name] + case (analytics_method = event_map[name]) when Proc analytics_method.call(analytics, attributes) when UnboundMethod From 8b317402dba4ccce122f19f4502829ff8c73cf89 Mon Sep 17 00:00:00 2001 From: Zach Margolis Date: Wed, 30 Aug 2023 11:52:57 -0700 Subject: [PATCH 03/10] Use #public_send on method name so that AnalyticsEventEnhancer can do its thing - bind_call calls a specific method implementation, so it doesn't go through the Enhancer implementation --- app/services/frontend_logger.rb | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/app/services/frontend_logger.rb b/app/services/frontend_logger.rb index a367957db0a..4135446c083 100644 --- a/app/services/frontend_logger.rb +++ b/app/services/frontend_logger.rb @@ -15,7 +15,10 @@ def track_event(name, attributes) when Proc analytics_method.call(analytics, attributes) when UnboundMethod - analytics_method.bind_call(analytics, **hash_from_method_kwargs(attributes, analytics_method)) + analytics.public_send( + analytics_method.name, + **hash_from_method_kwargs(attributes, analytics_method), + ) else analytics.track_event("Frontend: #{name}", attributes) end From 80d8d0c5a53ad4013b92e884851f62116f4d81f3 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Wed, 30 Aug 2023 16:01:17 -0400 Subject: [PATCH 04/10] dedicated class? --- app/controllers/frontend_log_controller.rb | 8 ++------ app/services/frontend_error_logger.rb | 7 +++++++ app/services/frontend_logger.rb | 15 ++++++++------- spec/controllers/frontend_log_controller_spec.rb | 2 +- 4 files changed, 18 insertions(+), 14 deletions(-) create mode 100644 app/services/frontend_error_logger.rb diff --git a/app/controllers/frontend_log_controller.rb b/app/controllers/frontend_log_controller.rb index 052d065c07c..dc570ccefb9 100644 --- a/app/controllers/frontend_log_controller.rb +++ b/app/controllers/frontend_log_controller.rb @@ -1,6 +1,4 @@ class FrontendLogController < ApplicationController - class FrontendError < StandardError; end - respond_to :json skip_before_action :verify_authenticity_token @@ -13,7 +11,7 @@ class FrontendError < StandardError; end # Please try to keep this list alphabetical as well! # rubocop:disable Layout/LineLength EVENT_MAP = { - 'Frontend Error' => proc { |_analytics, payload| NewRelic::Agent.notice_error(FrontendError.new, custom_params: payload) }, + 'Frontend Error' => FrontendErrorLogger.method(:track_error), 'IdV: consent checkbox toggled' => :idv_consent_checkbox_toggled, 'IdV: download personal key' => :idv_personal_key_downloaded, 'IdV: location submitted' => :idv_in_person_location_submitted, @@ -33,9 +31,7 @@ class FrontendError < StandardError; end 'Sign In: IdV requirements accordion clicked' => :sign_in_idv_requirements_accordion_clicked, 'User prompted before navigation' => :user_prompted_before_navigation, 'User prompted before navigation and still on page' => :user_prompted_before_navigation_and_still_on_page, - }.transform_values do |method| - method.is_a?(Proc) ? method : AnalyticsEvents.instance_method(method) - end.freeze + }.freeze # rubocop:enable Layout/LineLength def create diff --git a/app/services/frontend_error_logger.rb b/app/services/frontend_error_logger.rb new file mode 100644 index 00000000000..1d15be0b986 --- /dev/null +++ b/app/services/frontend_error_logger.rb @@ -0,0 +1,7 @@ +class FrontendErrorLogger + class FrontendError < StandardError; end + + def self.track_error(name:, message:, stack:) + NewRelic::Agent.notice_error(FrontendError.new, custom_params: { name:, message:, stack: }) + end +end diff --git a/app/services/frontend_logger.rb b/app/services/frontend_logger.rb index 4135446c083..f1d85f0fe70 100644 --- a/app/services/frontend_logger.rb +++ b/app/services/frontend_logger.rb @@ -11,14 +11,15 @@ def initialize(analytics:, event_map:) # @param [String] name # @param [Hash] attributes def track_event(name, attributes) - case (analytics_method = event_map[name]) - when Proc - analytics_method.call(analytics, attributes) - when UnboundMethod - analytics.public_send( - analytics_method.name, - **hash_from_method_kwargs(attributes, analytics_method), + analytics_method = event_map[name] + case analytics_method + when Symbol + analytics.send( + analytics_method, + **hash_from_method_kwargs(attributes, analytics.method(analytics_method)), ) + when Method + analytics_method.call(**hash_from_method_kwargs(attributes, analytics_method)) else analytics.track_event("Frontend: #{name}", attributes) end diff --git a/spec/controllers/frontend_log_controller_spec.rb b/spec/controllers/frontend_log_controller_spec.rb index 5595e288020..7b052939644 100644 --- a/spec/controllers/frontend_log_controller_spec.rb +++ b/spec/controllers/frontend_log_controller_spec.rb @@ -186,7 +186,7 @@ it 'notices the error to NewRelic instead of analytics logger' do expect(fake_analytics).not_to receive(:track_event) expect(NewRelic::Agent).to receive(:notice_error).with( - described_class::FrontendError.new, + FrontendErrorLogger::FrontendError.new, custom_params: { name: 'name', message: 'message', From ae086b4a4792e5ee1b704db43599cebbdcddf845 Mon Sep 17 00:00:00 2001 From: Zach Margolis Date: Wed, 30 Aug 2023 13:17:35 -0700 Subject: [PATCH 05/10] WIP: experimenting with less special-casing of Analytics class --- app/controllers/frontend_log_controller.rb | 42 +++++++++++----------- app/services/frontend_error_logger.rb | 2 +- app/services/frontend_logger.rb | 29 +++++++++------ 3 files changed, 40 insertions(+), 33 deletions(-) diff --git a/app/controllers/frontend_log_controller.rb b/app/controllers/frontend_log_controller.rb index dc570ccefb9..ba980c44f11 100644 --- a/app/controllers/frontend_log_controller.rb +++ b/app/controllers/frontend_log_controller.rb @@ -11,26 +11,26 @@ class FrontendLogController < ApplicationController # Please try to keep this list alphabetical as well! # rubocop:disable Layout/LineLength EVENT_MAP = { - 'Frontend Error' => FrontendErrorLogger.method(:track_error), - 'IdV: consent checkbox toggled' => :idv_consent_checkbox_toggled, - 'IdV: download personal key' => :idv_personal_key_downloaded, - 'IdV: location submitted' => :idv_in_person_location_submitted, - 'IdV: location visited' => :idv_in_person_location_visited, - 'IdV: Mobile device and camera check' => :idv_mobile_device_and_camera_check, - 'IdV: Native camera forced after failed attempts' => :idv_native_camera_forced, - 'IdV: personal key acknowledgment toggled' => :idv_personal_key_acknowledgment_toggled, - 'IdV: prepare submitted' => :idv_in_person_prepare_submitted, - 'IdV: prepare visited' => :idv_in_person_prepare_visited, - 'IdV: switch_back submitted' => :idv_in_person_switch_back_submitted, - 'IdV: switch_back visited' => :idv_in_person_switch_back_visited, - 'IdV: user clicked sp link on ready to verify page' => :idv_in_person_ready_to_verify_sp_link_clicked, - 'IdV: user clicked what to bring link on ready to verify page' => :idv_in_person_ready_to_verify_what_to_bring_link_clicked, - 'IdV: verify in person troubleshooting option clicked' => :idv_verify_in_person_troubleshooting_option_clicked, - 'Multi-Factor Authentication: download backup code' => :multi_factor_auth_backup_code_download, - 'Show Password button clicked' => :show_password_button_clicked, - 'Sign In: IdV requirements accordion clicked' => :sign_in_idv_requirements_accordion_clicked, - 'User prompted before navigation' => :user_prompted_before_navigation, - 'User prompted before navigation and still on page' => :user_prompted_before_navigation_and_still_on_page, + 'Frontend Error' => [FrontendErrorLogger, :track_error], + 'IdV: consent checkbox toggled' => [Analytics, :idv_consent_checkbox_toggled], + 'IdV: download personal key' => [Analytics, :idv_personal_key_downloaded], + 'IdV: location submitted' => [Analytics, :idv_in_person_location_submitted], + 'IdV: location visited' => [Analytics, :idv_in_person_location_visited], + 'IdV: Mobile device and camera check' => [Analytics, :idv_mobile_device_and_camera_check], + 'IdV: Native camera forced after failed attempts' => [Analytics, :idv_native_camera_forced], + 'IdV: personal key acknowledgment toggled' => [Analytics, :idv_personal_key_acknowledgment_toggled], + 'IdV: prepare submitted' => [Analytics, :idv_in_person_prepare_submitted], + 'IdV: prepare visited' => [Analytics, :idv_in_person_prepare_visited], + 'IdV: switch_back submitted' => [Analytics, :idv_in_person_switch_back_submitted], + 'IdV: switch_back visited' => [Analytics, :idv_in_person_switch_back_visited], + 'IdV: user clicked sp link on ready to verify page' => [Analytics, :idv_in_person_ready_to_verify_sp_link_clicked], + 'IdV: user clicked what to bring link on ready to verify page' => [Analytics, :idv_in_person_ready_to_verify_what_to_bring_link_clicked], + 'IdV: verify in person troubleshooting option clicked' => [Analytics, :idv_verify_in_person_troubleshooting_option_clicked], + 'Multi-Factor Authentication: download backup code' => [Analytics, :multi_factor_auth_backup_code_download], + 'Show Password button clicked' => [Analytics, :show_password_button_clicked], + 'Sign In: IdV requirements accordion clicked' => [Analytics, :sign_in_idv_requirements_accordion_clicked], + 'User prompted before navigation' => [Analytics, :user_prompted_before_navigation], + 'User prompted before navigation and still on page' => [Analytics, :user_prompted_before_navigation_and_still_on_page], }.freeze # rubocop:enable Layout/LineLength @@ -43,7 +43,7 @@ def create private def frontend_logger - FrontendLogger.new(analytics: analytics, event_map: EVENT_MAP) + FrontendLogger.new(analytics: analytics, error_logger: FrontendErrorLogger.new, event_map: EVENT_MAP) end def log_params diff --git a/app/services/frontend_error_logger.rb b/app/services/frontend_error_logger.rb index 1d15be0b986..4110ad49926 100644 --- a/app/services/frontend_error_logger.rb +++ b/app/services/frontend_error_logger.rb @@ -1,7 +1,7 @@ class FrontendErrorLogger class FrontendError < StandardError; end - def self.track_error(name:, message:, stack:) + def track_error(name:, message:, stack:) NewRelic::Agent.notice_error(FrontendError.new, custom_params: { name:, message:, stack: }) end end diff --git a/app/services/frontend_logger.rb b/app/services/frontend_logger.rb index f1d85f0fe70..84a39015503 100644 --- a/app/services/frontend_logger.rb +++ b/app/services/frontend_logger.rb @@ -1,25 +1,32 @@ class FrontendLogger - attr_reader :analytics, :event_map + attr_reader :analytics, :error_logger, :event_map - # @param [Analytics] - # @param [Hash{String=>UnboundMethod,Proc}] - def initialize(analytics:, event_map:) + # @param [Analytics] analytics + # @param [FrontendErrorLogger] error_logger + # @param [Hash{String=>Symbol,Method}] event_map + def initialize(analytics:, error_logger:, event_map:) @analytics = analytics + @error_logger = error_logger @event_map = event_map end # @param [String] name # @param [Hash] attributes def track_event(name, attributes) - analytics_method = event_map[name] - case analytics_method - when Symbol - analytics.send( + target_class, analytics_method = event_map[name] + + # case/when doesn't work because === on classes is an instance check, not identity + target = if target_class == Analytics + analytics + elsif target_class == FrontendErrorLogger + error_logger + end + + if analytics_method + target.send( analytics_method, - **hash_from_method_kwargs(attributes, analytics.method(analytics_method)), + **hash_from_method_kwargs(attributes, target.method(analytics_method)), ) - when Method - analytics_method.call(**hash_from_method_kwargs(attributes, analytics_method)) else analytics.track_event("Frontend: #{name}", attributes) end From 3d82f7da4c3d258ac63ac2705615c68b10df50b7 Mon Sep 17 00:00:00 2001 From: Zach Margolis Date: Wed, 30 Aug 2023 15:17:04 -0700 Subject: [PATCH 06/10] Revert "WIP: experimenting with less special-casing of Analytics class" This reverts commit ae086b4a4792e5ee1b704db43599cebbdcddf845. --- app/controllers/frontend_log_controller.rb | 42 +++++++++++----------- app/services/frontend_error_logger.rb | 2 +- app/services/frontend_logger.rb | 29 ++++++--------- 3 files changed, 33 insertions(+), 40 deletions(-) diff --git a/app/controllers/frontend_log_controller.rb b/app/controllers/frontend_log_controller.rb index ba980c44f11..dc570ccefb9 100644 --- a/app/controllers/frontend_log_controller.rb +++ b/app/controllers/frontend_log_controller.rb @@ -11,26 +11,26 @@ class FrontendLogController < ApplicationController # Please try to keep this list alphabetical as well! # rubocop:disable Layout/LineLength EVENT_MAP = { - 'Frontend Error' => [FrontendErrorLogger, :track_error], - 'IdV: consent checkbox toggled' => [Analytics, :idv_consent_checkbox_toggled], - 'IdV: download personal key' => [Analytics, :idv_personal_key_downloaded], - 'IdV: location submitted' => [Analytics, :idv_in_person_location_submitted], - 'IdV: location visited' => [Analytics, :idv_in_person_location_visited], - 'IdV: Mobile device and camera check' => [Analytics, :idv_mobile_device_and_camera_check], - 'IdV: Native camera forced after failed attempts' => [Analytics, :idv_native_camera_forced], - 'IdV: personal key acknowledgment toggled' => [Analytics, :idv_personal_key_acknowledgment_toggled], - 'IdV: prepare submitted' => [Analytics, :idv_in_person_prepare_submitted], - 'IdV: prepare visited' => [Analytics, :idv_in_person_prepare_visited], - 'IdV: switch_back submitted' => [Analytics, :idv_in_person_switch_back_submitted], - 'IdV: switch_back visited' => [Analytics, :idv_in_person_switch_back_visited], - 'IdV: user clicked sp link on ready to verify page' => [Analytics, :idv_in_person_ready_to_verify_sp_link_clicked], - 'IdV: user clicked what to bring link on ready to verify page' => [Analytics, :idv_in_person_ready_to_verify_what_to_bring_link_clicked], - 'IdV: verify in person troubleshooting option clicked' => [Analytics, :idv_verify_in_person_troubleshooting_option_clicked], - 'Multi-Factor Authentication: download backup code' => [Analytics, :multi_factor_auth_backup_code_download], - 'Show Password button clicked' => [Analytics, :show_password_button_clicked], - 'Sign In: IdV requirements accordion clicked' => [Analytics, :sign_in_idv_requirements_accordion_clicked], - 'User prompted before navigation' => [Analytics, :user_prompted_before_navigation], - 'User prompted before navigation and still on page' => [Analytics, :user_prompted_before_navigation_and_still_on_page], + 'Frontend Error' => FrontendErrorLogger.method(:track_error), + 'IdV: consent checkbox toggled' => :idv_consent_checkbox_toggled, + 'IdV: download personal key' => :idv_personal_key_downloaded, + 'IdV: location submitted' => :idv_in_person_location_submitted, + 'IdV: location visited' => :idv_in_person_location_visited, + 'IdV: Mobile device and camera check' => :idv_mobile_device_and_camera_check, + 'IdV: Native camera forced after failed attempts' => :idv_native_camera_forced, + 'IdV: personal key acknowledgment toggled' => :idv_personal_key_acknowledgment_toggled, + 'IdV: prepare submitted' => :idv_in_person_prepare_submitted, + 'IdV: prepare visited' => :idv_in_person_prepare_visited, + 'IdV: switch_back submitted' => :idv_in_person_switch_back_submitted, + 'IdV: switch_back visited' => :idv_in_person_switch_back_visited, + 'IdV: user clicked sp link on ready to verify page' => :idv_in_person_ready_to_verify_sp_link_clicked, + 'IdV: user clicked what to bring link on ready to verify page' => :idv_in_person_ready_to_verify_what_to_bring_link_clicked, + 'IdV: verify in person troubleshooting option clicked' => :idv_verify_in_person_troubleshooting_option_clicked, + 'Multi-Factor Authentication: download backup code' => :multi_factor_auth_backup_code_download, + 'Show Password button clicked' => :show_password_button_clicked, + 'Sign In: IdV requirements accordion clicked' => :sign_in_idv_requirements_accordion_clicked, + 'User prompted before navigation' => :user_prompted_before_navigation, + 'User prompted before navigation and still on page' => :user_prompted_before_navigation_and_still_on_page, }.freeze # rubocop:enable Layout/LineLength @@ -43,7 +43,7 @@ def create private def frontend_logger - FrontendLogger.new(analytics: analytics, error_logger: FrontendErrorLogger.new, event_map: EVENT_MAP) + FrontendLogger.new(analytics: analytics, event_map: EVENT_MAP) end def log_params diff --git a/app/services/frontend_error_logger.rb b/app/services/frontend_error_logger.rb index 4110ad49926..1d15be0b986 100644 --- a/app/services/frontend_error_logger.rb +++ b/app/services/frontend_error_logger.rb @@ -1,7 +1,7 @@ class FrontendErrorLogger class FrontendError < StandardError; end - def track_error(name:, message:, stack:) + def self.track_error(name:, message:, stack:) NewRelic::Agent.notice_error(FrontendError.new, custom_params: { name:, message:, stack: }) end end diff --git a/app/services/frontend_logger.rb b/app/services/frontend_logger.rb index 84a39015503..f1d85f0fe70 100644 --- a/app/services/frontend_logger.rb +++ b/app/services/frontend_logger.rb @@ -1,32 +1,25 @@ class FrontendLogger - attr_reader :analytics, :error_logger, :event_map + attr_reader :analytics, :event_map - # @param [Analytics] analytics - # @param [FrontendErrorLogger] error_logger - # @param [Hash{String=>Symbol,Method}] event_map - def initialize(analytics:, error_logger:, event_map:) + # @param [Analytics] + # @param [Hash{String=>UnboundMethod,Proc}] + def initialize(analytics:, event_map:) @analytics = analytics - @error_logger = error_logger @event_map = event_map end # @param [String] name # @param [Hash] attributes def track_event(name, attributes) - target_class, analytics_method = event_map[name] - - # case/when doesn't work because === on classes is an instance check, not identity - target = if target_class == Analytics - analytics - elsif target_class == FrontendErrorLogger - error_logger - end - - if analytics_method - target.send( + analytics_method = event_map[name] + case analytics_method + when Symbol + analytics.send( analytics_method, - **hash_from_method_kwargs(attributes, target.method(analytics_method)), + **hash_from_method_kwargs(attributes, analytics.method(analytics_method)), ) + when Method + analytics_method.call(**hash_from_method_kwargs(attributes, analytics_method)) else analytics.track_event("Frontend: #{name}", attributes) end From ff8a00f830f91bb2bc6012dc61de5258d4b2684b Mon Sep 17 00:00:00 2001 From: Zach Margolis Date: Wed, 30 Aug 2023 15:26:39 -0700 Subject: [PATCH 07/10] Slight simplification, support #call-able objects --- app/services/frontend_logger.rb | 12 +++++++----- spec/services/frontend_logger_spec.rb | 8 ++++---- 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/app/services/frontend_logger.rb b/app/services/frontend_logger.rb index f1d85f0fe70..592876039d1 100644 --- a/app/services/frontend_logger.rb +++ b/app/services/frontend_logger.rb @@ -1,24 +1,26 @@ class FrontendLogger attr_reader :analytics, :event_map - # @param [Analytics] - # @param [Hash{String=>UnboundMethod,Proc}] + # @param [Analytics] analytics + # @param [Hash{String=>Symbol,#call}] event_map map of string event names to method names + # on Analytics, or a custom implementation that's callable (like a Proc or Method) def initialize(analytics:, event_map:) @analytics = analytics @event_map = event_map end + # Logs an event and converts the payload to the correct keyword args # @param [String] name # @param [Hash] attributes def track_event(name, attributes) analytics_method = event_map[name] - case analytics_method - when Symbol + + if analytics_method.kind_of?(Symbol) analytics.send( analytics_method, **hash_from_method_kwargs(attributes, analytics.method(analytics_method)), ) - when Method + elsif analytics_method.respond_to?(:call) analytics_method.call(**hash_from_method_kwargs(attributes, analytics_method)) else analytics.track_event("Frontend: #{name}", attributes) diff --git a/spec/services/frontend_logger_spec.rb b/spec/services/frontend_logger_spec.rb index c8012d3bebc..577e7c5aa03 100644 --- a/spec/services/frontend_logger_spec.rb +++ b/spec/services/frontend_logger_spec.rb @@ -16,9 +16,9 @@ def example_method_handler(ok:, **rest) let(:event_map) do { - 'method' => ExampleAnalyticsEvents.instance_method(:example_method_handler), - 'proc' => proc do |analytics, payload| - analytics.track_event('some customized event', payload.merge('custom' => true)) + 'method' => analytics.method(:example_method_handler), + 'proc' => lambda do |ok:, other:| + analytics.track_event('some customized event', 'ok' => ok, 'other' => other, 'custom' => 1) end, } end @@ -57,7 +57,7 @@ def example_method_handler(ok:, **rest) call expect(analytics).to have_logged_event( - 'some customized event', 'ok' => true, 'other' => true, 'custom' => true + 'some customized event', 'ok' => true, 'other' => true, 'custom' => 1 ) end end From a567375a4c7f1b21bab94d31d559f9145168186f Mon Sep 17 00:00:00 2001 From: Zach Margolis Date: Wed, 30 Aug 2023 15:34:41 -0700 Subject: [PATCH 08/10] Rename and document things for clarity --- app/services/frontend_logger.rb | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/app/services/frontend_logger.rb b/app/services/frontend_logger.rb index 592876039d1..ec6cc610103 100644 --- a/app/services/frontend_logger.rb +++ b/app/services/frontend_logger.rb @@ -11,17 +11,17 @@ def initialize(analytics:, event_map:) # Logs an event and converts the payload to the correct keyword args # @param [String] name - # @param [Hash] attributes + # @param [Hash] attributes payload with string keys def track_event(name, attributes) analytics_method = event_map[name] if analytics_method.kind_of?(Symbol) analytics.send( analytics_method, - **hash_from_method_kwargs(attributes, analytics.method(analytics_method)), + **hash_from_kwargs(attributes, analytics.method(analytics_method)), ) elsif analytics_method.respond_to?(:call) - analytics_method.call(**hash_from_method_kwargs(attributes, analytics_method)) + analytics_method.call(**hash_from_kwargs(attributes, analytics_method)) else analytics.track_event("Frontend: #{name}", attributes) end @@ -29,12 +29,17 @@ def track_event(name, attributes) private - def hash_from_method_kwargs(hash, method) - method_kwargs(method).index_with { |key| hash[key.to_s] } + # @param [Hash] hash + # @param [Proc,Method] callable + # @return [Hash] + def hash_from_kwargs(hash, callable) + kwargs(callable).index_with { |key| hash[key.to_s] } end - def method_kwargs(method) - method. + # @param [Proc,Method] callable + # @return [Array] the names of the kwargs for the callable (both optional and required) + def kwargs(callable) + callable. parameters. map { |type, name| name if [:key, :keyreq].include?(type) }. compact From 3cdad4edcbde8a65e17ebdd9fe6cd618b9a097e3 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Thu, 31 Aug 2023 08:27:53 -0400 Subject: [PATCH 09/10] Lint is_a --- app/services/frontend_logger.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/services/frontend_logger.rb b/app/services/frontend_logger.rb index ec6cc610103..88dabd9ff5b 100644 --- a/app/services/frontend_logger.rb +++ b/app/services/frontend_logger.rb @@ -15,7 +15,7 @@ def initialize(analytics:, event_map:) def track_event(name, attributes) analytics_method = event_map[name] - if analytics_method.kind_of?(Symbol) + if analytics_method.is_a?(Symbol) analytics.send( analytics_method, **hash_from_kwargs(attributes, analytics.method(analytics_method)), From 9e28cb858f018dd10f695f7a918174776d079303 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Thu, 31 Aug 2023 08:28:54 -0400 Subject: [PATCH 10/10] Fix issue with IdV::AnalyticsEventsEnhancer override The method signature is lost if read directly from analytics instance, since it's overridden by AnalyticsEventsEnhancer. We need the original method reference --- app/services/frontend_logger.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/services/frontend_logger.rb b/app/services/frontend_logger.rb index 88dabd9ff5b..1e5034d003a 100644 --- a/app/services/frontend_logger.rb +++ b/app/services/frontend_logger.rb @@ -18,7 +18,7 @@ def track_event(name, attributes) if analytics_method.is_a?(Symbol) analytics.send( analytics_method, - **hash_from_kwargs(attributes, analytics.method(analytics_method)), + **hash_from_kwargs(attributes, AnalyticsEvents.instance_method(analytics_method)), ) elsif analytics_method.respond_to?(:call) analytics_method.call(**hash_from_kwargs(attributes, analytics_method))