Skip to content

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

@kaylareopelle
Copy link
Contributor

Hi @akahn, is this PR still needed now that #1826 has been merged?

@akahn
Copy link
Author

akahn commented Apr 14, 2025

Fixed by #1826

@akahn akahn closed this Apr 14, 2025
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.

2 participants