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

chore(linux): Improve Sentry reports #9725

Merged
merged 2 commits into from
Oct 10, 2023
Merged

Conversation

ermshiperete
Copy link
Contributor

  • fix the calculation of the user hash so that the same user now always gets the same hash value. Previously we got a different value after restarting the app because the Python hash function calculates the hash based on the memory location.
  • add additional details to Sentry events so that the "All Events" tab on keyman.sentry.io will show helpful values in the "device" and "os" columns instead of emptry strings.

@keymanapp-test-bot skip

The `hash` method that we previously used calculates the hash based on
the memory location, i.e. with each program run we get a different value
for the same user. This change now uses the md5 algorithm so that the
same user always produces the same hash.
This change adds additional details to Sentry events so that the "All
Events" tab on keyman.sentry.io will show helpful values in the
"device" and "os" columns instead of emptry strings.
@keymanapp-test-bot
Copy link

keymanapp-test-bot bot commented Oct 9, 2023

User Test Results

Test specification and instructions

User tests are not required

Test Artifacts

Comment on lines +128 to +130
hash = hashlib.md5()
hash.update(getpass.getuser().encode())
set_user({'id': hash.hexdigest()})
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure about this -- md5 hashes of short strings are pretty easy to reverse with rainbow tables. It's low risk but it is a bit of an overshare.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So better use sha256?

Copy link
Member

@mcdurdin mcdurdin left a comment

Choose a reason for hiding this comment

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

LGTM. I am thinking I was worrying too much about the hash of username. I think it's no big deal -- the data is not really useful to anyone as it is just used to link related crash reports as you say.

@ermshiperete ermshiperete merged commit 405f2e8 into master Oct 10, 2023
3 checks passed
@ermshiperete ermshiperete deleted the chore/linux/sentrydetails branch October 10, 2023 13:06
@keyman-server
Copy link
Collaborator

Changes in this pull request will be available for download in Keyman version 17.0.187-alpha

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants