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

Auto-flush on process exit #141

Merged
merged 6 commits into from
Dec 18, 2024
Merged

Conversation

Mr0grog
Copy link
Collaborator

@Mr0grog Mr0grog commented Dec 14, 2024

This is a bit of an ugly first pass. The code needs some cleanup and there are some open questions, but it works and has docs and tests. Fixes #136.

This attaches to the process’s beforeExit event (supported in Node.js, Deno, and Bun) when auto-flushing is enabled in order to automatically flush when a program exits. Before, you needed to remember to call metrics.flush() manually at the end of your program, which is easy to forget (or not even know about in the first place!).

It also adds a close() method (not sure about the name) that flushes and buffered metrics and disables auto-flushing. It does not prevent adding new metrics to the buffer, but maybe it should? (Would they error? Get dropped silently? Call the onError handler?)

To-do/open questions:

  • Clean up implementation. At the very least, get some of this code out of the BufferedMetricsLogger constructor.
  • Dedupe all the mock reporter code in tests.
  • Is close() a good name? Other ideas: (Update: renamed to stop)
    • stop()
    • shutdown()
    • destroy()
    • disable()
    • pause()
  • Should close() also prevent buffering new metrics? (i.e. make addPoint() a no-op, or maybe throw an exception or call the global error handler) (Update: decided no for now.)

This is an ugly first-pass at auto-flushing before the process exits. This previously required manually flushing, which is easy to forget if everything just happens automatically otherwise. Fixes #136.

There's a lot that should be cleaned up here (the `BufferedMetricsLogger` just has way too much logic now, for example). But this gets it working and adds tests. This also adds a `close()` method (maybe it should be renamed? stop? shutdown?) that disables autoflushing and flushes any buffered metrics.
@Mr0grog Mr0grog marked this pull request as draft December 14, 2024 02:58
@Mr0grog Mr0grog marked this pull request as ready for review December 17, 2024 21:15
@Mr0grog
Copy link
Collaborator Author

Mr0grog commented Dec 17, 2024

Cleaned this up a bit and it should be good to go.

Besides burying too much code in the constructor, I think my initial implementation was a bit too fancy by trying to retain a subtle previous behavior where you could adjust auto-flushing on the fly by changing loggerInstance.flushIntervalSeconds, which would be picked up on the next flush. That property was already private though, and I don’t think that usage was ever really intended. Doing this also made it easier to rename close() to stop() (since it now has a twin start() method, which makes things feel a bit more well aligned to me).

Since renaming the method to stop(), I think that also answers the final question about disabling new metrics — “stop” doesn’t really imply a total shutdown, so it doesn’t disable anything other than auto-flushing (which can now be restarted if you want).

@Mr0grog Mr0grog merged commit 31ababb into main Dec 18, 2024
10 checks passed
@Mr0grog Mr0grog deleted the 136-autoflush-should-flush-when-you-need-it branch December 18, 2024 23:40
Mr0grog added a commit that referenced this pull request Dec 18, 2024
Release v0.12.1 with some maintenance updates and the new `process.beforeExit` functionality from #141.
@Mr0grog Mr0grog mentioned this pull request Dec 18, 2024
Mr0grog added a commit that referenced this pull request Dec 19, 2024
Release v0.12.1 with some maintenance updates and the new `process.beforeExit` functionality from #141.
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.

Auto flush when process is exiting
1 participant