-
Notifications
You must be signed in to change notification settings - Fork 34
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
✨ Async analytics #260
✨ Async analytics #260
Conversation
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.
Tested on my machine and lgtm just remove those old lines you commented out and we are good to merge
response = future.result() | ||
|
||
if not response.status_code==200: | ||
print("Something went wrong while sending analytics!") |
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.
Out of scope for this pr but possibly might look into reading status codes for more specific error messages
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.
lgtm
Summary:
Make analytics run concurrently alongside cli commands
requests-futures
(an extension ofrequests
- what we use right now) for handling http requests asynchronouslyNew usage:
None, but the
--use-analytics=True
flag must be set for analytics to be recordedMotivation:
CLI analytics are currently a blocking function on the actual command, and can take a long time to run for those with bad internet. This change sends analytics requests in the background, so that there is no blocking.
References (optional):
Closes #250
Test Plan:
For the following tests
--use-analytics
should be set to falseFor the following
--use-analytics
should be set to trueTesting Screenshots:
No Analytics:
Successful Analytics:
Failed Analytics: