Skip to content
Merged
17 changes: 3 additions & 14 deletions app/controllers/frontend_log_controller.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,4 @@
class FrontendLogController < ApplicationController
class FrontendError < StandardError; end

respond_to :json

skip_before_action :verify_authenticity_token
Expand All @@ -10,11 +8,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' => 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,
Expand All @@ -34,15 +31,11 @@ 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
}.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
Expand All @@ -69,10 +62,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
Expand Down
7 changes: 7 additions & 0 deletions app/services/frontend_error_logger.rb
Original file line number Diff line number Diff line change
@@ -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
30 changes: 24 additions & 6 deletions app/services/frontend_logger.rb
Original file line number Diff line number Diff line change
@@ -1,27 +1,45 @@
class FrontendLogger
attr_reader :analytics, :event_map

# @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<String,Object>] attributes payload with string keys
def track_event(name, attributes)
if (analytics_method = event_map[name])
analytics.send(analytics_method.name, **hash_from_method_kwargs(attributes, analytics_method))
analytics_method = event_map[name]

if analytics_method.is_a?(Symbol)
analytics.send(
analytics_method,
**hash_from_kwargs(attributes, AnalyticsEvents.instance_method(analytics_method)),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

9e28cb8 fixes an issue with some specific events in IdV not working. I think this is also what transform_values was helping with previously, since trying to get the method signature from the analytics instance is going to have issues with how IdV::AnalyticsEventsEnhancer overrides the method and loses the signature.

| [3] pry(#<FrontendLogger>)> analytics.method(analytics_method)
=> #<Method: Analytics(Idv::AnalyticsEventsEnhancer)#idv_personal_key_acknowledgment_toggled(**kwargs) /identity-idp/app/services/idv/analytics_events_enhancer.rb:39>

define_method(method_name) do |**kwargs|

We could also consider moving this back into transform_values, and maybe there's even something where we can consolidate the handling so that the EVENT_MAP hash values are all the same type of callable thing? The issue comes with binding though... maybe we can have a small logic branch to normalize an unbound method to bind it with analytics?

analytics_method = analytics_method.bind(analytics) if analytics_method.is_a?(UnboundMethod)
analytics_method.respond_to?(:call)
  analytics_method.call(**hash_from_kwargs(attributes, analytics_method))
else
  analytics.track_event("Frontend: #{name}", attributes)
end

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re: bound vs unbound... I feel like we should have only one or the other? Seems like a lot of handling to manage both

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zachmargolis How do you feel about the current implementation in this branch?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel good! 🚢

)
elsif analytics_method.respond_to?(:call)
analytics_method.call(**hash_from_kwargs(attributes, analytics_method))
else
analytics.track_event("Frontend: #{name}", attributes)
end
end

private

def hash_from_method_kwargs(hash, method)
method_kwargs(method).index_with { |key| hash[key.to_s] }
# @param [Hash<String,Object>] hash
# @param [Proc,Method] callable
# @return [Hash<Symbol,Object>]
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<Symbol>] 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
Expand Down
4 changes: 2 additions & 2 deletions spec/controllers/frontend_log_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand All @@ -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',
Expand Down
17 changes: 16 additions & 1 deletion spec/services/frontend_logger_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,10 @@ def example_method_handler(ok:, **rest)

let(:event_map) do
{
'method' => ExampleAnalyticsEvents.instance_method(:example_method_handler),
'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
let(:logger) { described_class.new(analytics: analytics, event_map: event_map) }
Expand Down Expand Up @@ -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' => 1
)
end
end
end
end