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 write_to_textfile leaves back temp files on errors #1066

Merged
merged 1 commit into from
Oct 25, 2024

Conversation

ethanschen
Copy link
Contributor

@ethanschen ethanschen commented Oct 16, 2024

Fix #1044

@ethanschen ethanschen changed the title Fix write_to_textfile leaves back temp files on errors (#1044) Fix write_to_textfile leaves back temp files on errors Oct 16, 2024
@calestyo
Copy link

Looks in good to me.

The only two things I'd comment are:

  • Catching Exception won’t e.g. catch KeyboardInterrupt ... but probably this doesn’t matter, since I guess there’s no clean-up in that case anyway (e.g. when metrics would be only half-written or so).
  • At least in principle os.remove(tmppath) could raise an exception, too. ... One could catch that with another try block and, if caught, set the __cause__ of that exception to the original. But probably overkill.

@ethanschen
Copy link
Contributor Author

Looks in good to me.

The only two things I'd comment are:

  • Catching Exception won’t e.g. catch KeyboardInterrupt ... but probably this doesn’t matter, since I guess there’s no clean-up in that case anyway (e.g. when metrics would be only half-written or so).
  • At least in principle os.remove(tmppath) could raise an exception, too. ... One could catch that with another try block and, if caught, set the __cause__ of that exception to the original. But probably overkill.

Thanks.

Regarding your first point, I believe the KeyboardInterrupt exception would only occur when triggered manually by the user.I could add handling for this exception, but I'm not sure if it’s necessary for prometheus_client. Perhaps @csmarchbanks could provide some insights.

As for the second point, if the os.remove method throws an exception, the error stack trace is quite clear, as shown below:

Traceback (most recent call last):
  File "/WorkSpace/venv/lib/python3.8/site-packages/prometheus_client/exposition.py", line 378, in write_to_textfile
    os.rename(tmppath, path)
FileNotFoundError: [Errno 2] No such file or directory: '.80904.140704537089984' -> ''

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "x.py", line 6, in <module>
    write_to_textfile('', registry)
  File "/WorkSpace/venv/lib/python3.8/site-packages/prometheus_client/exposition.py", line 381, in write_to_textfile
    os.remove(1/0)
ZeroDivisionError: division by zero

Therefore, I think avoiding nested exception handling code might make it more elegant.

Copy link
Member

@csmarchbanks csmarchbanks left a comment

Choose a reason for hiding this comment

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

👍 I agree that handling keyboard interrupts is not needed and keeping this as simple as possible is nice. Thanks!

@csmarchbanks csmarchbanks merged commit c89624f into prometheus:master Oct 25, 2024
11 checks passed
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.

write_to_textfile leaves back temp files on errors
3 participants