Skip to content

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Feb 11, 2025
Merged

Fix the telemetry exception handling for daemon mode #60

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d63da68
Fix the telemetry exception handling for daemon mode
amritghimire Feb 7, 2025
daeb301
PR comments
amritghimire Feb 10, 2025
330d9da
Timeout to 10
amritghimire Feb 11, 2025
8da4239
Suppress exception instead
amritghimire Feb 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 12 additions & 4 deletions src/iterative_telemetry/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,10 +190,18 @@ def send(
impl(payload)

def _send_daemon(self, payload):
cmd = (
f"import requests;requests.post('{self.url}',"
f"params={{'token':'{self.token}'}},json={payload})"
)
cmd = f"""
import requests, logging
try:
requests.post(
'{self.url}',
params={{'token':'{self.token}'}},
json={payload},
timeout=2
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Collaborator

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.

)
except (requests.exceptions.RequestException, Exception) as e:
logging.debug(f'Telemetry request failed: {{str(e)}}')
"""

if os.name == "nt":

Expand Down
Loading