Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add identifier for explicit frontend error logging #11481

Merged
merged 3 commits into from
Nov 12, 2024

Conversation

aduth
Copy link
Member

@aduth aduth commented Nov 8, 2024

🛠 Summary of changes

Updates frontend error logging through trackError to require an identifier label.

This fixes an issue where explicit calls to trackError are currently not surfaced in error logging, due to changes in #8950 to filter out errors originating in scripts outside our own code. An unintended side-effect of this change is that we were ignoring our own errors.

Including an errorId serves a dual purpose of guaranteeing that the error originates in our own code, while also providing an easy way to trace the source of the error.

📜 Testing Plan

Verify that an error would be tracked to NewRelic for an error logged through trackError.

  1. Add a binding.pry immediately before the call to NewRelic::Agent.notice_error in FrontendErrorLogger (see diff below)
  2. Repeat Testing Plan from Add error handling for failed reCAPTCHA execute #11449
  3. Observe that the breakpoint is triggered, and that printing the output of error_id at the breakpoint shows 'recaptchaExecute'
diff --git a/app/services/frontend_error_logger.rb b/app/services/frontend_error_logger.rb
index 226dd49aff..4a466cf80c 100644
--- a/app/services/frontend_error_logger.rb
+++ b/app/services/frontend_error_logger.rb
@@ -6,6 +6,7 @@ class FrontendErrorLogger
   def self.track_error(name:, message:, stack:, filename: nil, error_id: nil)
     return unless FrontendErrorForm.new.submit(filename:, error_id:).success?
 
+    binding.pry
     NewRelic::Agent.notice_error(
       FrontendError.new,
       expected: true,

@lmgeorge
Copy link
Contributor

lmgeorge commented Nov 8, 2024

Overall, this looks good (even if I wish we didn't have to handroll error tracking like this, I feel like this should be easier).

I have a couple of questions:

  1. Do you expect to share error IDs across whole flows, or will they be unique per caller/try-catch block?
    I want to understand how much context we expect to be communicated by the error ID.
  2. Is it worthwhile to hoist the error IDs to higher in the containing file to make it easier to find if they are meant to be used at the file scope?
    (I'm thinking of the instance where webauthnSetup mimics webauthn-setup.ts filename.)

@aduth
Copy link
Member Author

aduth commented Nov 8, 2024

I think the answer to both your questions is that they ought to be unique per each call to trackEvent, not shared across user flows or within a file.

But practically speaking, they can be anything. The problem this aims to address is to make it easier to work backwards from errors logged to NewRelic to the originating code. Without either a file name or identifier, all we have to work from is some cryptic error, like an InvalidStateError with message 'The user attempted to register an authenticator that contains one of the credentials already registered with the relying party.', and hope that the person reading that knows how to trace it back to WebAuthn setup code.

@aduth aduth merged commit ed38d8a into main Nov 12, 2024
2 checks passed
@aduth aduth deleted the aduth-track-errors-filename branch November 12, 2024 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants