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

Prevent slow shutdown caused by waiting full metrics export interval #1827

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

akahn
Copy link

@akahn akahn commented Mar 19, 2025

We noticed that when using the new metrics functionality in the library that our Rails application now took longer to shut down. We want to export metrics every 60 seconds, but we don't want to wait an additional 60 seconds for the app to stop. This was caused by the shutdown logic in PeriodicMetricReader which worked by setting @continue to false, indicating to the loop in start to no longer continue sleeping and writing. However, this meant that on shutdown, it takes a full @export_interval in order to "notice" that @continue is false an the program should exit.

With this change, upon shutdown, the program performs a final export and then kills the writer thread. This is an approach suggested by @xuan-cao-swi in #1662 (comment).

Some questions:

  1. Does the kill need to acquire the lock first?
  2. Is it still necessary to set @continue to false, if @continue is no longer used to control shutdown behavior?
  3. Is @continue even needed any more?
  4. Would it be preferable, rather than the thread-killing approach, to use a ConditionVariable with wait(@export_mutex, @export_interval) in the writing loop, and signal() in shutdown? This would allow the writing loop "wake up" responsively on shutdown, while ordinarily waiting @export_interval before looping and writing again.

@akahn akahn mentioned this pull request Mar 20, 2025
@akahn
Copy link
Author

akahn commented Mar 24, 2025

Note: option #4 is being explored by @xuan-cao-swi in #1826

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.

1 participant