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.

Sign up for GitHub

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

Jump to bottom

add crash-report capabilities #574

Merged
merged 11 commits into from
Dec 17, 2020
Merged

add crash-report capabilities #574

merged 11 commits into from
Dec 17, 2020

Conversation

umbynos
Copy link
Contributor

@umbynos umbynos commented Nov 30, 2020 •
edited
Loading

Please check if the PR fulfills these requirements

  • The PR has no duplicates (please search among the Pull Requests
    before creating one)
  • What kind of change does this PR introduce?

feature

  • What is the current behavior?

Sometimes the agent crashes without apparent reason

  • What is the new behavior?

A folder containing crash-reports is generated in that same folder containing the executable (e.g. in Linux it's in ~/ArduinoCreateAgent-<version>/logs) when the agent crashes. It should allow users to better report issues, and developers to better understand and fix bugs.
The file created inside the folder is named crashreport_yyyyMMddHHmmss.log and should contain something like:

stacktrace from panic: 
goroutine 1 [running, locked to thread]:
runtime/debug.Stack(0x0, 0x0, 0x0)
	/home/linuxbrew/.linuxbrew/Cellar/go/1.15.3/libexec/src/runtime/debug/stack.go:24 +0xa5
main.panicToFile()
	/home/umberto/Nextcloud/8tb/Lavoro/arduino-create-agent/main.go:165 +0x22d
panic(0xa09400, 0xb0ebb0)
	/home/linuxbrew/.linuxbrew/Cellar/go/1.15.3/libexec/src/runtime/panic.go:975 +0x4c6
main.main()
	/home/umberto/Nextcloud/8tb/Lavoro/arduino-create-agent/main.go:118 +0x98
  • Does this PR introduce a breaking change?

no

  • Other information:
  • add crash-report generation
  • allow users to enable/disable the functionality
  • allow users to remove all the generated logs

@umbynos umbynos changed the title add first implementation of a function generating a crash-report add crash-report capabilities Nov 30, 2020
@umbynos umbynos self-assigned this Nov 30, 2020
umbynos
umbynos commented Nov 30, 2020
View reviewed changes
main.go Outdated
fileName := "crashreport_" + time.Now().Format("20060102150405") + ".txt"
currDir, err := osext.ExecutableFolder()
if err != nil {
panic(err)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know if it's correct to panic inside a defer function. Maybe it's better to log the error. Returning the error is not possible.

umbynos
umbynos commented Nov 30, 2020
View reviewed changes
main.go Outdated Show resolved Hide resolved
@umbynos
Copy link
Contributor Author

umbynos commented Nov 30, 2020

Problems still to be solved:

  1. if the panic happens in the loop() function the panic does not get stopped because loop() is run in a different goroutine.

The process continues up the stack until all functions in the current goroutine have returned, at which point the program crashes
https://blog.golang.org/defer-panic-and-recover

  1. Handling of the old crash-reports still missing (which is the best way to do it?)

@umbynos
Copy link
Contributor Author

umbynos commented Dec 1, 2020

I've changed the approach: now everything happening on stderr is redirected to the crash-report saved locally. I've followed this issue There's still work to do regarding stdout and stderr. Now every output is redirected to the file (maybe because stdout and stderr are treated the same way(?))
Another possible problem concerns crash-report handling (how not to spam too many files)

@umbynos
Copy link
Contributor Author

umbynos commented Dec 1, 2020

Example of crash-report generated on windows.
crashreport_20201201165111.txt

@zmoog
Copy link
Contributor

zmoog commented Dec 2, 2020 •
edited by umbynos
Loading

Useful suggestions/enhancements:

  • Add a command in the systray (or a flag in the confi.ini) to clean/enable the logs
  • replace .txt -> .log extension for the log file

@umbynos umbynos force-pushed the umbynos/add_crashreport branch from 78dd953 to ac0f713 Compare December 4, 2020 09:01
@umbynos
Copy link
Contributor Author

umbynos commented Dec 10, 2020

Now the user should be able to optionally enable crash-report generation changing crashreport = true in config.ini
By default the functionally is disabled (crashreport = false).

@umbynos
Copy link
Contributor Author

umbynos commented Dec 14, 2020

image
Added option in the trayicon's menu to remove the crashreports
image
The newly added item is disables if the folder logs is not present.
Also logging have been added to the code added

@umbynos umbynos force-pushed the umbynos/add_crashreport branch from 1b49a47 to 774959c Compare December 14, 2020 16:47
@umbynos umbynos added the type: enhancement Proposed improvement label Dec 15, 2020
zmoog
zmoog approved these changes Dec 17, 2020
View reviewed changes
Copy link
Contributor

@zmoog zmoog left a comment

Choose a reason for hiding this comment

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

LGTM ❤️

@umbynos
CI polishing, add win64, update macOS min version (#578)
e6dc18d 
* uniform `test.yml` to `release.yml`

* added again #561 for testing purposes

* add win64 build in and refactor taskfile

* change min supported macOS version to 10.11 (also go 1.14)

* hard-code gon config in a step for uniformity and ease

* refactor and optimize Taskfile (✨) change also workflows accordingly

* add another win CI matrix to build win 32-64

* add win 32/64 to all the jobs in the release workflow & optimizations 🧙🏻
@umbynos umbynos merged commit 9c49735 into devel Dec 17, 2020
@umbynos umbynos deleted the umbynos/add_crashreport branch December 17, 2020 17:19
umbynos added a commit that referenced this pull request Dec 18, 2020
@umbynos
Add crash-report capabilities (#574)
d9936ce 
* change approach: redirect stderr to file instead using defer

* update gin dependency because of gin-gonic/gin#1571

* set default logger to stdout. (stderr is currently redirected to file)

* save crash-report to newly created `logs/` folder

* add enable/disable crash-report generation (default off)

* add entry in trayicon's menu to remove crash-reports (if there are any)

* CI polishing, add win64, update macOS min version  (#578)
@per1234 per1234 added the topic: code Related to content of the project itself label Mar 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zmoog zmoog zmoog approved these changes

Assignees

@umbynos umbynos

Labels
topic: code Related to content of the project itself type: enhancement Proposed improvement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@umbynos @zmoog @per1234