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

feat: persist application logs #242

Merged
merged 9 commits into from
Oct 9, 2024
Merged

Conversation

cristianoventura
Copy link
Collaborator

Description

This PR introduces changes needed to capture caught and uncaught exceptions to a log file. This will enable us to troubleshoot issues more efficiently.

Here's an example of a log file being generated by the Studio. This includes errors raised by the proxy, IpcMain call and unhandle exceptions.

[2024-10-07 12:06:39.823] [error] mitmproxy/certs.py:112: CryptographyDeprecationWarning: Properties that return a naïve datetime object have been deprecated. Please switch to not_valid_after_utc.

[2024-10-07 12:06:39.824] [error] /Users/username/www/k6-studio/resources/mac/arm64/k6-studio-proxy: Invalid proxy mode specification: regular@AAA (invalid port: AAA)

[2024-10-07 12:08:10.704] [error] Error: Error invoking remote method 'har:open': Error: ENOENT: no such file or directory, open '/Users/username/Documents/k6-studio/Recordings/2024-10-02_17-21-55.har'

[2024-10-07 12:17:58.722] [error] Unhandled rejection ReferenceError: currentBrowserProcess_NOT_DECLARED is not defined
    at IpcMainImpl.<anonymous> (/Users/username/www/k6-studio/.vite/build/main.js:2391:3)
    at IpcMainImpl.emit (node:events:518:28)
    at WebContents.<anonymous> (node:electron/js2c/browser_init:2:82429)
    at WebContents.emit (node:events:518:28)

In addition, a button was included in the UI to allow users to retrieve the log file more easily.

How to Test

  • Manually set the proxy to an invalid port and start the application
  • Manually change the recording path of a generator and try to open it
  • Throw exceptions in specific parts of the app and raise them from the UI

Checklist

  • I have performed a self-review of my code.
  • I have added tests for my changes.
  • I have run linter locally (npm run lint) and all checks pass.
  • I have run tests locally (npm test) and all tests pass.
  • I have commented on my code, particularly in hard-to-understand areas.

Screenshots (if appropriate):

image

Related PR(s)/Issue(s)

Resolves #235

@cristianoventura cristianoventura self-assigned this Oct 7, 2024
@cristianoventura cristianoventura marked this pull request as ready for review October 7, 2024 17:32
@cristianoventura cristianoventura requested a review from a team as a code owner October 7, 2024 17:32
@cristianoventura cristianoventura requested review from e-fisher and Llandy3d and removed request for a team October 7, 2024 17:32
- on Mac: `~/Library/Logs/k6 Studio/k6-studio.log`
- on Windows: `%USERPROFILE%\AppData\Roaming\k6 Studio\logs\k6-studio.log`

When opening an issue, please include a tail of your log file.
Copy link
Member

Choose a reason for hiding this comment

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

We should probably add a section in the bug template asking for this 🤔

src/logger.ts Outdated
log.initialize()

// log electron core events
// @see https://github.com/megahertz/electron-log/blob/master/docs/events.md
Copy link
Member

Choose a reason for hiding this comment

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

minor nit: I think the @see are redundant and probably the link alone would suffice 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@see is part of JSDoc. I believe it wouldn't be applicable for this context, so I'll remove it!

src/main.ts Outdated
@@ -505,6 +511,11 @@ ipcMain.handle('browser:open:external:link', (_, url: string) => {
shell.openExternal(url)
})

ipcMain.handle('application:open-log', () => {
Copy link
Member

Choose a reason for hiding this comment

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

minor suggestion: should we shorten it with app:open-log ?
I'm not sure about this

@cristianoventura cristianoventura force-pushed the feat/persist-application-logs branch from caa8ac7 to 52a3714 Compare October 7, 2024 19:55
Copy link
Collaborator

@e-fisher e-fisher left a comment

Choose a reason for hiding this comment

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

Throw exceptions in specific parts of the app and raise them from the UI

Should it catch UI exceptions automatically? I've tried adding throw new Error('test') to Home.tsx but it wasn't logged.

If I misunderstood, and it wasn't supposed to catch UI exceptions, could we wrap App in error boundary to implement this?

@cristianoventura
Copy link
Collaborator Author

Should it catch UI exceptions automatically? I've tried adding throw new Error('test') to Home.tsx but it wasn't logged.

If I misunderstood, and it wasn't supposed to catch UI exceptions, could we wrap App in error boundary to implement this?

That was missing since I forgot to initialize the error handler in the UI, good catch! Now it should be logging those to the disk as well! The error boundary is something we're missing and would be a nice addition but since we have everything logged to disk at this point, I'll leave the error boundary for a follow-up PR.

Copy link
Member

@Llandy3d Llandy3d left a comment

Choose a reason for hiding this comment

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

🙌

Copy link
Collaborator

@e-fisher e-fisher left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀

@cristianoventura cristianoventura merged commit a666c2a into main Oct 9, 2024
2 checks passed
@cristianoventura cristianoventura deleted the feat/persist-application-logs branch October 9, 2024 12:29
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.

Persist application logs
3 participants