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

Add sentry #5735

Merged
merged 1 commit into from
Nov 24, 2020
Merged

Add sentry #5735

merged 1 commit into from
Nov 24, 2020

Conversation

drew2a
Copy link
Contributor

@drew2a drew2a commented Nov 18, 2020

Resolves #5727

This PR adds Sentry support to Tribler Client.
The UX keeps unchanged.

The main concept: suppress (but store) all Sentry events until a user clicked on the "send report" button.

There are two new entities:

  1. SentryReporter(singleton) for managing work with Sentry
  2. SentryScrubber for scrubbing sensitive and redundant information.

How it works

Basically, Sentry-based reporting works the same way the old Reporter did:

  • catch the error in the Core (or in the GUI)
  • send it to the GUI over the Events endpoint (GUI errors are handled directly)
  • present the "report error" dialog to the user
  • send the report when the user clicks the "send report" button

Protecting user identities

A user will now be assigned a unique pseudonym (e.g.Jonh Doe) based on the hash of their public_key.

Note. The current implementation specifies user only for errors raised in the Core process, but it can be extended in the future.

Simplified python-friendly explanation

# `SentryReporter` will be inited at the start of a Tribler Client run
SentryReporter.init(sentry_url=sentry_url, scrubber=SentryScrubber())

# Then all Sentry events will be suppressed (`session.py`):
SentryReporter.allow_sending_globally(False)

# In a case of an error in  the `Core process`, the Sentry event will be obtained and sent to `GUI` (`session.py`):
sentry_event = SentryReporter.last_event
self.api_manager.get_endpoint('events').on_tribler_exception(text_long, sentry_event)

In the case of an error inside the GUI process, the event will be obtained directly in the GUI process.

After the user's click on the "sent report" button, the Sentry event will be sent to the server (feedbackdialog.py):

SentryReporter.send(post_data, sentry_event, sys_info_dict)

Removing sensitive and redundant information

SentryScrubber is the class responsible for scrubbing.
It is calling on every Sentry's attempt to sent an event.
By using regex it scans all the necessary event fields and replaces real information with placeholders.

What is sensitive information:

  • IPs
  • User Name
  • 40-symbols-long hashes

What is redundant information:

  • Information on installed python modules

Which fields will be affected by scrubbing:

  • Extra
  • Logentry
  • Breadcrumbds
  • Os.environ
  • Stacktrace
  • Sysinfo

Tests

This PR introduced the first tests for the tribler-common module.
All the corresponding Jenkin's jobs have to be updated:

  • PR Linux
  • PR macOS
  • PR Windows

Thanks to @ichorid for contributing to the description.

@ghost
Copy link

ghost commented Nov 18, 2020

DeepCode's analysis on #30b602 found:

  • ⚠️ 1 warning 👇

Top issues

Description Example fixes
hashlib.md5 is insecure. Consider changing it to a secure hashing algorithm (e.g. SHA256). Occurrences: 🔧 Example fixes

👉 View analysis in DeepCode’s Dashboard | Configure the bot

@drew2a drew2a force-pushed the add_sentry branch 6 times, most recently from 68e4d4e to 7b3f016 Compare November 19, 2020 16:42
@drew2a drew2a marked this pull request as ready for review November 19, 2020 16:46
@drew2a drew2a changed the title WIP Add sentry Add sentry Nov 19, 2020
@drew2a drew2a requested a review from ichorid November 19, 2020 16:47
Copy link
Contributor

@ichorid ichorid left a comment

Choose a reason for hiding this comment

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

Would you kindly run pre-commit hooks to format the stuff using black?

assert scrubber.scrub_entity_recursively({}) == {}
assert scrubber.scrub_entity_recursively([]) == []
assert scrubber.scrub_entity_recursively('') == ''
assert scrubber.scrub_entity_recursively(42) == 42
Copy link
Contributor

Choose a reason for hiding this comment

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

🐬 👍 🐟

Copy link
Contributor Author

@drew2a drew2a Nov 20, 2020

Choose a reason for hiding this comment

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

🐳🌸

sentry_url,
release=None,
# https://docs.sentry.io/platforms/python/configuration/integrations/
integrations=[
Copy link
Contributor

Choose a reason for hiding this comment

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

As I understand it from Sentry documentation, logging integration is one of the default integrations which are enabled by default, so probably it is unnecessary to specify it explicitly:
https://docs.sentry.io/platforms/python/configuration/integrations/default-integrations/
https://docs.sentry.io/platforms/python/guides/logging/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you a right, But now it is easy to configure levels of capturing information.
Otherwise, you need to spend some time reading documentation.

@drew2a drew2a changed the title Add sentry WIP Add sentry Nov 20, 2020
@xoriole
Copy link
Contributor

xoriole commented Nov 20, 2020

@drew2a Could you update sentry_url from environment variable to version.py via this script below? It will make sure that builder works as well.

with open(path.join('src', 'tribler-core', 'tribler_core', 'version.py'), 'w') as f:
f.write('version_id = "%s"%sbuild_date = "%s"%scommit_id = "%s"%s' %
(version_id, linesep, build_date, linesep, commit_id, linesep))

@drew2a
Copy link
Contributor Author

drew2a commented Nov 20, 2020

@xoriole yes, sure! 👌

@drew2a drew2a changed the title WIP Add sentry Add sentry Nov 23, 2020
ichorid
ichorid previously approved these changes Nov 24, 2020
Copy link
Contributor

@ichorid ichorid left a comment

Choose a reason for hiding this comment

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

Personally, I still perceive 9-line comments and tests for trivial one-liners a waste of both the writer's and the reader's mental energy, exactly the thing Python was designed to avoid.

But we need this PR in 7.6.

p.s.
Be warned that I will remove excessive comments if I ever touch the said code 😈

ichorid
ichorid previously approved these changes Nov 24, 2020
Copy link
Contributor

@ichorid ichorid left a comment

Choose a reason for hiding this comment

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

Thanks! 👍

@drew2a
Copy link
Contributor Author

drew2a commented Nov 24, 2020

@ichorid you are welcome :)

kozlovsky
kozlovsky previously approved these changes Nov 24, 2020
xoriole
xoriole previously approved these changes Nov 24, 2020
Copy link
Contributor

@xoriole xoriole left a comment

Choose a reason for hiding this comment

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

After the merge, please make sure to update the build jobs with the right sentry URL.
https://jenkins-ci.tribler.org/job/Build-Tribler_release/job/Build/

@drew2a drew2a force-pushed the add_sentry branch 2 times, most recently from 4198302 to 9cc33bb Compare November 24, 2020 15:53
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 5 Security Hotspots to review)
Code Smell A 6 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

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

Successfully merging this pull request may close these issues.

Add Sentry error reporting to the Tribler Client
4 participants