-
Notifications
You must be signed in to change notification settings - Fork 373
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
Send analytics events with callstacks on panics and signals #1409
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Wumpf
approved these changes
Feb 27, 2023
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me!
I decided to add all the build-info as event properties too, so we can easily match a crash with the right version. |
@Wumpf you may wanna take a second look at my post-review additions 😬 |
Wumpf
reviewed
Feb 27, 2023
emilk
added a commit
that referenced
this pull request
Mar 2, 2023
* Send analytics events with callstacks on panics and signals * Trim everything leading up to `run_native_app` * analytics is an acceptable label for CI * Merge fix * Include git branch and rust build date/time in default analytics * Send analytics last * Better function naming * Include BuildInfo in crash report analytics * Let's prefix the event names with what they are: crashes * Unify how build-info is added to Event:s
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
📊 analytics
telemetry analytics
💣 crash
crash, deadlock/freeze, do-no-start
enhancement
New feature or request
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #1404
Pains have been taken to not reveal any sensitive part of the file path in the stacktrace.
The callstack only includes functions that is in the rerun source-code or in crates we call, so is not sensitive either. This will remain true as long as we only install these panic handlers in rerun binaries.
Even when we support user-plugins (with callbacks), users will have to write their own entry-point binaries to rerun, and should there be able to decide wgether or not to install our panic handlers, thus giving users the choice to not send callstacks that could include user code.
With this PR, we install crash handlers in
rerun::run
, which is called by thererun
binary and bypython -m rerun
.We take care to trim the callstack from any calls before
re_viewer::native::run_native_app
to exclude any user-code for users who (forever reason) callrerun::run
themselves.When we have a crash running
python -m rerun
the bottom of the callstack looks like this:I don't know if anything sensitive can be garnered from these
__PyEval
things, but I've decided to trim it anyway by culling everything followingrun_native_app
.Here is an example of an anonymized callstack on a Python crash, copied straight from PostHog:
Checklist
CHANGELOG.md
(if this is a big enough change to warrant it)