Skip to content

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
Signed-off-by: Ethan S. Chen <ethanschen@163.com>
@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

3 participants