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

Make sentry_sdk (and distro) optional dependencies #4182

Merged
merged 4 commits into from
Jun 21, 2021

Conversation

ferdnyc
Copy link
Contributor

@ferdnyc ferdnyc commented Jun 10, 2021

Optional sentry_sdk

This PR moves all code that depends on the sentry_sdk module into a new classes.sentry, and locally attempts to import sentry_sdk as sdk if available. If not, sdk is set to None.

The sentry_sdk methods set_tag, set_user, and set_context are mirrored into classes.sentry. They are safe to call from other code, and will silently do nothing if sentry.sdk is None. sentry.init_tracing() and sentry.disable_tracing() offer the same protections.

Any other sentry_sdk methods can be used as needed, but those calls should take the form:

from classes import sentry
if sentry.sdk:
    sentry.sdk.some_method(...)

The Sentry init is moved just slightly later in the launch.py initialization, because there's really no point in initializing it just to exit after showing the command-line --help or similar. (Also, importing multiple modules before adjusting sys.path() has been problematic in the past.)

Optional distro

Additionally, the distro module is made optional in classes.sentry (as it's not a standard Python module), and is also (optionally) used in classes.metrics (in place of the long-since-deprecated platform.linux_distribution()).

Secure Google Analytics metrics

Additionally-additionally, the URLs to deliver Google Analytics metrics are made https://, since sentry_sdk is using HTTPS so it seems that train has sailed.
Fixes: #3953

cc: @jonoomph

Fixes: #4194

@ferdnyc
Copy link
Contributor Author

ferdnyc commented Jun 10, 2021

Actually... looking at this code, use of Sentry doesn't appear to be properly gated on the send-metrics preference. Initializing it in launch.py will unconditionally enable transmission of data, with sentry.disable_tracing() only called if the checkbox is unchecked when the first tutorial screen is shown. Which means we're back in the situation where data upload is enabled prior to the user being shown the data collection opt-in, no?

@ferdnyc
Copy link
Contributor Author

ferdnyc commented Jun 10, 2021

(The primary impetus behind relocating sentry_sdk and making it optional is that the Linux distros are never gonna go for hard-requiring it. But we might get away with bundling it, as long as it's still optional.)

@jonoomph
Copy link
Member

I wonder if distro.linux_distribution can be invoked from Windows? If you take a look at Sentry traces so far from Windows, it appears that this is not working correctly, although I haven't tracked it down yet. Otherwise, this looks great! Nice work! Merge away.

@ferdnyc
Copy link
Contributor Author

ferdnyc commented Jun 21, 2021

I wonder if distro.linux_distribution can be invoked from Windows? If you take a look at Sentry traces so far from Windows, it appears that this is not working correctly, although I haven't tracked it down yet.

Mm, I can change that to

if distro and platform.system() == "Linux":
    sdk.set_tag("distro", " ".join(distro.linux_distribution()))

@ferdnyc ferdnyc merged commit e1e6d3b into OpenShot:develop Jun 21, 2021
@ferdnyc ferdnyc deleted the optional-sentry branch June 21, 2021 03:00
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.

Installing code from github is missing sentry_sdk module in openshot-qt HTTPS for anonymized metrics
2 participants