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

fix: disabling telemetry prevents writing config #3465

Merged
merged 6 commits into from
Oct 25, 2022

Conversation

julian-risch
Copy link
Member

@julian-risch julian-risch commented Oct 24, 2022

Related Issues

Proposed Changes:

This PR adds a check before writing the telemetry config file. If telemetry is disabled, this file is not written. As a consequence no event will be sent (not even a file event that telemetry is turned off).

Previously, if no config file existed and telemetry was disabled via an environment variable, it could happen that a new user id was generated and stored in a config file. As a consequence a final event was sent, which causes the error

ERROR:posthog:error uploading: HTTPSConnectionPool(host='tm.hs.deepset.ai', port=443): Max retries exceeded with url: /batch/ (Caused by NewConnectionError('<urllib3.connection.HTTPSConnection object at 0x7f07b4173c70>: Failed to establish a new connection: [Errno -3] Temporary failure in name resolution'))

if there is no internet connection.

How did you test it?

locally by running:

rm ~/.haystack/config.yaml
export HAYSTACK_TELEMETRY_ENABLED=False
python
from haystack.document_stores import InMemoryDocumentStore
document_store = InMemoryDocumentStore()

Notes for the reviewer

Checklist

@julian-risch julian-risch requested a review from ZanSara October 24, 2022 15:08
@julian-risch julian-risch requested a review from a team as a code owner October 24, 2022 15:08
@julian-risch julian-risch requested review from vblagoje and removed request for a team and vblagoje October 24, 2022 15:08
Copy link
Contributor

@ZanSara ZanSara left a comment

Choose a reason for hiding this comment

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

Looks good! Approving after the nitpick 😁

haystack/telemetry.py Outdated Show resolved Hide resolved
@julian-risch julian-risch merged commit 6a422d5 into main Oct 25, 2022
@julian-risch julian-risch deleted the disabling-telemetry branch October 25, 2022 10:11
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.

Telemetry can't be fully disabled
2 participants