-
Notifications
You must be signed in to change notification settings - Fork 2
Fix the telemetry exception handling for daemon mode #60
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 the telemetry exception handling for daemon mode #60
Conversation
In daemon mode, the request is made in subprocess effectively causing a huge set of stacktrace in terminal as provided in iterative/studio#11282. This handles the exception properly along with timeout for the telemetry call. Closes iterative/studio#11282
src/iterative_telemetry/__init__.py
Outdated
'{self.url}', | ||
params={{'token':'{self.token}'}}, | ||
json={payload}, | ||
timeout=2 |
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.
I think it should be safe to make it 10+ seconds?
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.
I used the default from the timeout we already had for thread based or normal method of sending the request. I am not sure if we want to have timeout upto 10 seconds delay to close a program when program already failed for the user.
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.
if we want to have timeout upto 10 seconds delay to close a program when program already failed for the user.
are we waiting for the process to exit / complete on the user program exit?
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.
Looks like we have close_fds=True,
so, we won't have the blocking exit. Will update the timeout to 10 seconds.
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.
Well, close_fds=True
does not close stdin
, stdout
and stderr
streams. So it might end up blocking. But that's a separate issue.
In daemon mode, the request is made in subprocess effectively causing a
huge set of stacktrace in terminal as provided in
https://github.com/iterative/studio/issues/11282.
This handles the exception properly along with timeout for the telemetry
call.
Closes https://github.com/iterative/studio/issues/11282