client_python icon indicating copy to clipboard operation
client_python copied to clipboard

Fix `write_to_textfile` leaves back temp files on errors

Open ethanschen opened this issue 1 year ago • 2 comments

Fix #1044

ethanschen avatar Oct 16 '24 15:10 ethanschen

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.

calestyo avatar Oct 17 '24 23:10 calestyo

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.

ethanschen avatar Oct 19 '24 16:10 ethanschen