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

fix: Provide thread names whenever possible #316

Merged
merged 5 commits into from
Mar 10, 2025

Conversation

yuki24
Copy link
Contributor

@yuki24 yuki24 commented Mar 7, 2025

Requirements

  • I have added test coverage for new or changed functionality
  • I have followed the repository's pull request submission guidelines
  • I have validated my changes against all supported platform versions

Related issues

There are no related issues at the time of filing.

Describe the solution you've provided

At @speedshop we have a client that uses LaunchDarkly. One thing we noticed was that the Ruby SDK relies on the Thread very extensively, either directly or through the concurrent-ruby gem. As a result of this, a number of threads will be created after the first call to LD:

Xnapper-2025-03-07-16 06 14

This is all expected, and normally this is fine. In some specific circumstances where a deep dive into e.g. performance of each thread is required, not having a thread name could possibly lead to a longer investigation. For example, the gvl-tracing gem generates Perfetto profiling results for each thread, that looks like this:

Xnapper-2025-03-07-15 25 11

The red arrow points to Thread 11, which is not obvious as to who created it, what it's used for, etc. Compare that with the thread name of "puma srv tp 001 from puma 1" above, which makes it very clear that it's a worker thread within a Puma instance.

This PR updates the code that creates threads to also assign a name to them. After this PR the result of the Perfetto visualization could look like this:

Xnapper-2025-03-07-15 26 03

As you can see, now this is a lot easier to understand who owns this thread. Because of the nature of the thread where we don't know exactly when they are created or GC-ed, I believe it is also important to assign thread names whenever possible, so they will show up with a clear name in a profiling result like above.

Describe alternatives you've considered

The thread name itself is quite primitive enough that there is no other alternative here. Other good citizens like the debug gem, ActiveRecord, Puma all provide thread names, so I believe it is good practice to follow in general.

Additional context

The Concurrent::TimerTask class from concurrent-ruby, used in the EventProcessor class:

@flush_task = Concurrent::TimerTask.new(execution_interval: config.flush_interval) do

@contexts_flush_task = Concurrent::TimerTask.new(execution_interval: config.context_keys_flush_interval) do

@diagnostic_event_task = Concurrent::TimerTask.new(execution_interval: interval) do

also creates threads internally. Unfortunately it ignores the name option and there is no way for us to set a name at least today, so I left it unchanged for now.

@keelerm84
Copy link
Member

keelerm84 commented Mar 7, 2025

Thank you for raising this issue, and for your associated contribution!

To fully complete this, I need to merge your change in the other repository, cut that release, bump it here, and then I can review this one. I'll keep you updated as I make these changes.

@keelerm84 keelerm84 changed the title Provide thread names whenever possible fix: Provide thread names whenever possible Mar 10, 2025
@keelerm84 keelerm84 merged commit 6c30537 into launchdarkly:main Mar 10, 2025
7 checks passed
@keelerm84
Copy link
Member

@yuki24 thank you for this contribution. I made a few small tweaks regarding naming. I also bumped to the latest version of the event source (thank you again for that change as well).

keelerm84 pushed a commit that referenced this pull request Mar 10, 2025
🤖 I have created a release *beep* *boop*
---


##
[8.8.4](8.8.3...8.8.4)
(2025-03-10)


### Bug Fixes

* Provide thread names whenever possible
([#316](#316))
([6c30537](6c30537))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@keelerm84
Copy link
Member

This has been released in 8.8.4

@yuki24 yuki24 deleted the feature/specify-thread-names branch March 11, 2025 23:37
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.

3 participants