Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
7812b78
Try replacing NewRelic browser instrumentation with custom error handler
aduth Aug 7, 2023
6f46a45
Experiment with hand-minified inline snippet
aduth Aug 7, 2023
575e2b4
Must go smaller
aduth Aug 7, 2023
2b4d20e
Smaller!
aduth Aug 7, 2023
914adef
Filter events to same-host script errors
aduth Aug 29, 2023
01ad0e1
Use javascript_tag for prelude script
aduth Aug 29, 2023
6470b0b
Log Webpack script errors in development environment
aduth Aug 29, 2023
223a63d
Route frontend error events to NewRelic
aduth Aug 29, 2023
164cb8a
Revert "Log Webpack script errors in development environment"
aduth Aug 29, 2023
106fd69
Remove demo error
aduth Aug 29, 2023
7ff899a
Update index.spec.ts
aduth Aug 30, 2023
da0edb2
Add custom proc support to FrontendLogger, simplify FrontendLogContro…
zachmargolis Aug 30, 2023
625de7e
Bring back parens around assignment inside a conditional
zachmargolis Aug 30, 2023
e59f074
Use #public_send on method name so that AnalyticsEventEnhancer can do…
zachmargolis Aug 30, 2023
ff757a4
dedicated class?
aduth Aug 30, 2023
f2f1da4
WIP: experimenting with less special-casing of Analytics class
zachmargolis Aug 30, 2023
c45092f
Revert "WIP: experimenting with less special-casing of Analytics class"
zachmargolis Aug 30, 2023
80d18c6
Slight simplification, support #call-able objects
zachmargolis Aug 30, 2023
51f3e01
Rename and document things for clarity
zachmargolis Aug 30, 2023
33f4f68
Lint is_a
aduth Aug 31, 2023
36cb9d6
Fix issue with IdV::AnalyticsEventsEnhancer override
aduth Aug 31, 2023
c9d605c
Log errors as expected
aduth Sep 14, 2023
13d961e
Temporary: Test error
aduth Sep 14, 2023
1f7ee55
Alphabetize analytics events
aduth Sep 14, 2023
cffb342
Update spec expectations
aduth Sep 14, 2023
4431b94
Revert "Temporary: Test error"
aduth Sep 14, 2023
4bdf111
Try namespacing logged error
aduth Sep 14, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion app/controllers/frontend_log_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ 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,
Expand All @@ -30,7 +31,7 @@ class FrontendLogController < ApplicationController
'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
Expand Down
6 changes: 3 additions & 3 deletions app/helpers/script_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,11 @@ def javascript_packs_tag_once(*names, prepend: false)
alias_method :enqueue_component_scripts, :javascript_packs_tag_once

def render_javascript_pack_once_tags(*names)
javascript_packs_tag_once(*names) if names.present?
if @scripts && (sources = AssetSources.get_sources(*@scripts)).present?
names = names.presence || @scripts
if names && (sources = AssetSources.get_sources(*names)).present?
safe_join(
[
javascript_assets_tag(*@scripts),
javascript_assets_tag(*names),
*sources.map do |source|
javascript_include_tag(
source,
Expand Down
32 changes: 20 additions & 12 deletions app/javascript/packages/analytics/index.spec.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { trackEvent, trackError } from '@18f/identity-analytics';
import { usePropertyValue, useSandbox } from '@18f/identity-test-helpers';
import { useSandbox } from '@18f/identity-test-helpers';
import type { SinonStub } from 'sinon';

describe('trackEvent', () => {
Expand Down Expand Up @@ -84,20 +84,28 @@ describe('trackEvent', () => {
});

describe('trackError', () => {
it('is a noop', () => {
trackError(new Error('Oops!'));
const sandbox = useSandbox();
const endpoint = '/log';

beforeEach(() => {
sandbox.stub(global.navigator, 'sendBeacon').returns(true);
document.body.innerHTML = `<script type="application/json" data-config>{"analyticsEndpoint":"${endpoint}"}</script>`;
});

context('with newrelic agent present', () => {
const sandbox = useSandbox();
const noticeError = sandbox.stub();
usePropertyValue(globalThis as any, 'newrelic', { noticeError });
it('tracks event', async () => {
trackError(new Error('Oops!'));

it('notices error in newrelic', () => {
const error = new Error('Oops!');
trackError(error);
expect(global.navigator.sendBeacon).to.have.been.calledOnce();

expect(noticeError).to.have.been.calledWith(error);
});
const [actualEndpoint, data] = (global.navigator.sendBeacon as SinonStub).firstCall.args;
expect(actualEndpoint).to.eql(endpoint);

const { event, payload } = JSON.parse(await data.text());
const { name, message, stack } = payload;

expect(event).to.equal('Frontend Error');
expect(name).to.equal('Error');
expect(message).to.equal('Oops!');
expect(stack).to.be.a('string');
});
});
12 changes: 3 additions & 9 deletions app/javascript/packages/analytics/index.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,6 @@
import type { noticeError } from 'newrelic';
import { getConfigValue } from '@18f/identity-config';

type NewRelicAgent = { noticeError: typeof noticeError };

interface NewRelicGlobals {
newrelic?: NewRelicAgent;
}
export { default as isTrackableErrorEvent } from './is-trackable-error-event';
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.

Follow-up task: Would like to move trackEvent and trackError to dedicated files track-event.ts and track-error.ts so this index.ts can actually be an index.


/**
* Logs an event.
Expand All @@ -30,6 +25,5 @@ export function trackEvent(event: string, payload?: object) {
*
* @param error Error object.
*/
export function trackError(error: Error) {
(globalThis as typeof globalThis & NewRelicGlobals).newrelic?.noticeError(error);
}
export const trackError = ({ name, message, stack }: Error) =>
trackEvent('Frontend Error', { name, message, stack });
37 changes: 37 additions & 0 deletions app/javascript/packages/analytics/is-trackable-error-event.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
import isTrackableErrorEvent from './is-trackable-error-event';

describe('isTrackableErrorEvent', () => {
context('with filename not present on event', () => {
const event = new ErrorEvent('error');

it('returns false', () => {
expect(isTrackableErrorEvent(event)).to.be.false();
});
});

context('with filename as an invalid url', () => {
const event = new ErrorEvent('error', { filename: '.' });

it('returns false', () => {
expect(isTrackableErrorEvent(event)).to.be.false();
});
});

context('with filename from a different host', () => {
const event = new ErrorEvent('error', { filename: 'http://different.example.com/foo.js' });

it('returns false', () => {
expect(isTrackableErrorEvent(event)).to.be.false();
});
});

context('with filename from the same host', () => {
const event = new ErrorEvent('error', {
filename: new URL('foo.js', window.location.origin).toString(),
});

it('returns true', () => {
expect(isTrackableErrorEvent(event)).to.be.true();
});
});
});
9 changes: 9 additions & 0 deletions app/javascript/packages/analytics/is-trackable-error-event.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
function isTrackableErrorEvent(event: ErrorEvent): boolean {
try {
return new URL(event.filename).host === window.location.host;
} catch {
return false;
}
}

export default isTrackableErrorEvent;
14 changes: 14 additions & 0 deletions app/javascript/packs/track-errors.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import { trackError, isTrackableErrorEvent } from '@18f/identity-analytics';

export interface WindowWithInitialErrors extends Window {
_e: ErrorEvent[];
}

declare let window: WindowWithInitialErrors;

const { _e: initialErrors } = window;

const handleErrorEvent = (event: ErrorEvent) =>
isTrackableErrorEvent(event) && trackError(event.error);
initialErrors.forEach(handleErrorEvent);
window.addEventListener('error', handleErrorEvent);
8 changes: 8 additions & 0 deletions app/services/analytics_events.rb
Original file line number Diff line number Diff line change
Expand Up @@ -573,6 +573,14 @@ def fraud_review_rejected(
)
end

# An uncaught error occurred in frontend JavaScript
# @param [String] name
# @param [String] message
# @param [String] stack
def frontend_error(name:, message:, stack: nil, **extra)
track_event('Frontend Error', name:, message:, stack:, **extra)
end

# @param [Boolean] success
# @param [Boolean] address_edited
# @param [Hash] pii_like_keypaths
Expand Down
11 changes: 11 additions & 0 deletions app/services/frontend_error_logger.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
class FrontendErrorLogger
class FrontendError < StandardError; end

def self.track_error(name:, message:, stack:)
NewRelic::Agent.notice_error(
FrontendError.new,
expected: true,
custom_params: { frontend_error: { 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)),
)
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
6 changes: 4 additions & 2 deletions app/views/layouts/base.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,9 @@
) %>
<meta content="#ffffff" name="theme-color" />

<% if BrowserSupport.supported?(request.user_agent) && IdentityConfig.store.newrelic_browser_key.present? && IdentityConfig.store.newrelic_browser_app_id.present? %>
<%= render 'shared/newrelic/browser_instrumentation' %>
<%# Prelude script for error tracking (see `track-errors`) %>
<%= javascript_tag(nonce: true) do %>
_e=[],addEventListener("error",(e)=>_e.push(e));
<% end %>

<%= yield(:head) if content_for?(:head) %>
Expand Down Expand Up @@ -109,6 +110,7 @@
false,
) %>
<%= javascript_packs_tag_once('application', prepend: true) %>
<%= javascript_packs_tag_once('track-errors') if BrowserSupport.supported?(request.user_agent) %>
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.

Question: Can we defer this?

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.

Question: Can we defer this?

I think we can and should make this <script async defer>, with the benefit being that the page can finish being considered "loaded" without being blocked by the tracker.

I still need to get some clarification about async script load order when combined with non-async scripts (related Slack discussion), and this'll require some revisions to ScriptHelper / AssetSources to make sure the extra attributes are carried across to the rendered script tag.

I'll plan to explore this as a follow-on task.

<%= render_javascript_pack_once_tags %>

<%= render 'shared/dap_analytics' if IdentityConfig.store.participate_in_dap && !session_with_trust? %>
Expand Down
Loading