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

Name Datadog::Core::Remote::Worker thread #3207

Merged
merged 3 commits into from
Oct 24, 2023

Conversation

ivoanjo
Copy link
Member

@ivoanjo ivoanjo commented Oct 18, 2023

What does this PR do?

This PR sets the name of the thread started by the Datadog::Core::Remote::Worker class (to be the name of the class).

This follows the same pattern we use for other dd-trace-rb internal threads.

Motivation:

The profiler shows thread names in multiple places, and nameless threads make it harder to see the data in several cases, so as much as possible we want to avoid them.

One particular case is that for the timeline feature, we collapse all dd-trace-rb threads by default, but that collapsing feature relies on thread names, and so the remote worker thread was not being properly collapsed (which is how I spotted it).

Additional Notes:

I've chosen to still include the Ruby 2.2/2.1 support because I plan to backport this to the 1.x branch.

How to test the change?

This change includes test coverage. You can also enable the profiler and check that when looking at thread names (right sidebar in single and aggregated profiles; left side for timeline) you will see the thread for this class properly named.

For Datadog employees:

  • If this PR touches code that signs or publishes builds or packages, or handles
    credentials of any kind, I've requested a review from @DataDog/security-design-and-guidance.
  • This PR doesn't touch any of that.

@ivoanjo ivoanjo requested a review from a team as a code owner October 18, 2023 13:35
@github-actions github-actions bot added the core Involves Datadog core libraries label Oct 18, 2023
@ivoanjo ivoanjo changed the title Ivoanjo/name core remote worker thread Name Datadog::Core::Remote::Worker thread Oct 18, 2023
**What does this PR do?**

This PR sets the name of the thread started by the
`Datadog::Core::Remote::Worker` class (to be the name of the class).

This follows the same pattern we use for other dd-trace-rb internal
threads.

**Motivation:**

The profiler shows thread names in multiple places, and nameless
threads make it harder to see the data in several cases, so as much
as possible we want to avoid them.

One particular case is that for the timeline feature, we collapse
all dd-trace-rb threads by default, but that collapsing feature
relies on thread names, and so the remote worker thread was not
being properly collapsed (which is how I spotted it).

**Additional Notes:**

I've chosen to still include the Ruby 2.2/2.1 support because I plan
to backport this to the 1.x branch.

**How to test the change?**

This change includes test coverage. You can also enable the profiler
and check that when looking at thread names (right sidebar in
single and aggregated profiles; left side for timeline) you will
see the thread for this class properly named.
@ivoanjo ivoanjo force-pushed the ivoanjo/name-core-remote-worker-thread branch from 81d6996 to 64b1769 Compare October 24, 2023 13:12
@ivoanjo
Copy link
Member Author

ivoanjo commented Oct 24, 2023

Update: I had tried to merge master into this branch but that triggered a bunch of gemfile updates that were missing.

Instead, I pulled the gemfile updates into a separate PR (#3213) and rebased this branch on top of master once that other PR got merged.

@ivoanjo ivoanjo merged commit 3f5c6d9 into master Oct 24, 2023
217 of 218 checks passed
@ivoanjo ivoanjo deleted the ivoanjo/name-core-remote-worker-thread branch October 24, 2023 16:46
ivoanjo added a commit that referenced this pull request Oct 25, 2023
…te-worker-thread

Backport #3207 to 1.x-stable: Name `Datadog::Core::Remote::Worker` thread
@marcotc marcotc added this to the 1.16.0 milestone Oct 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Involves Datadog core libraries
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants