Skip to content

Log filename in frontend error logging#10027

Merged
aduth merged 4 commits intomainfrom
aduth-error-event-filename
Feb 8, 2024
Merged

Log filename in frontend error logging#10027
aduth merged 4 commits intomainfrom
aduth-error-event-filename

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented Feb 2, 2024

🛠 Summary of changes

Updates frontend error logging to include the ErrorEvent#filename property, if applicable.

Previously, this was typically assumed to be made available via the existing stack log detail, but this is not always included depending on the browser.

This detail will help validate a hypothesis that current errors being logged are originating from files not expected to be eligible for logging via isTrackableErrorEvent.

📜 Testing Plan

  1. echo "setTimeout(() => { throw new Error('example'); }, 100)" >> app/javascript/packages/analytics/index.ts
  2. NODE_ENV=production yarn build
  3. rails s
  4. Go to http://localhost:3000
  5. TBD validate NewRelic logging (Slack discussion)

@aduth
Copy link
Contributor Author

aduth commented Feb 5, 2024

The re-shuffling of exports from @18f/identity-analytics caused some issues for how we were stubbing it in other tests. The way we were doing that didn't feel very "proper" so this feels like it was inevitable. I might spend some time thinking about how best to approach this.

@aduth aduth marked this pull request as draft February 5, 2024 13:35
@aduth
Copy link
Contributor Author

aduth commented Feb 5, 2024

The re-shuffling of exports from @18f/identity-analytics caused some issues for how we were stubbing it in other tests. The way we were doing that didn't feel very "proper" so this feels like it was inevitable. I might spend some time thinking about how best to approach this.

I came up with an approach that works, though I'm not thrilled about the changes to FrontendLogController to support the default behavior of sendBeacon passed as a JSON string. It's slightly simpler on the client-side, and avoids the issue of having to "decode" a blob (which can't be done synchronously), but it feels improper to send JSON to the server as plaintext and re-parse it manually, when we have the option to send it as JSON in the first place (good sleuthing on this behavior in the initial implementation of sendBeacon, @matthinz).

The most compelling alternative is to find a way to override the default analytics behavior in the test environment, maybe by exposing some additional properties/configuration from the analytics package. Challenge is trying to find an elegant way to do this which isn't too distinct between environments to risk desync issues, and which keeps the package lean enough that test-env specific code doesn't unnecessarily inflate the end-user download size.

@aduth
Copy link
Contributor Author

aduth commented Feb 7, 2024

To avoid dealing with the JavaScript stubbing issue, b3bba00 switches back to the original index.ts exports to focus the changes here on the new filename logging. I'll follow-up separately with a pull request to find a way to re-split index.ts.

@aduth aduth marked this pull request as ready for review February 7, 2024 20:28
aduth added 3 commits February 8, 2024 08:38
changelog: Internal, Error Logging, Log filename in frontend error logging
Avoid dealing with dependency stubbing issue
@aduth aduth force-pushed the aduth-error-event-filename branch from b3bba00 to c187257 Compare February 8, 2024 13:38
@aduth aduth merged commit 465bffc into main Feb 8, 2024
@aduth aduth deleted the aduth-error-event-filename branch February 8, 2024 14:33
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