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

Include native thread id as part of thread id label for profiling #2345

Merged
merged 2 commits into from
Nov 8, 2022

Conversation

ivoanjo
Copy link
Member

@ivoanjo ivoanjo commented Nov 2, 2022

What does this PR do?:

This PR extends the thread id label that is attached to profiling samples to include the thread's "native thread id".

Up until now, the thread id that we attached was the result of calling Thread#object_id. That approach allows us to easily match what we see inside the Ruby process (because we can log the Thread#object_id where something happens) with the profile.

BUT if we wanted to match the profile with what the operating system's tooling showed, it was quite hard, because threads shown in the OS task manager tools show an OS-level thread identifier, which is not the same as the Thread#object_id.

To enable this matching, this PR changes the format of the thread id from "#{thread.object_id}" to "#{thread.native_object_id} (#{thread.object_id})" thus providing both identifers. (This is OK with the profiling backend).

Because Thread#native_object_id is a Ruby 3.1+ feature, on older Rubies we use a fallback identifier instead (pthread_t). This identifier isn't as useful as the native identifier; in the future we may want to explore a better fallback.

Motivation:

I found this helpful during development and when correlating data from profiler with other tools.

Additional Notes:

This includes full support for macOS, even though we don't officially support macOS.

This feature is only available on the new CPU Profiling 2.0 profiler which is still in alpha, see #2209.

How to test the change?:

This is easiest to test by profiling a Ruby app, and then checking that the native ids for threads match up with what shows up on /proc. (Remember, only for >= 3.1, for older Rubies it won't match.)

**What does this PR do?**:

This PR extends the `thread id` label that is attached to profiling
samples to include the thread's "native thread id".

Up until now, the `thread id` that we attached was the result of
calling `Thread#object_id`. That approach allows us to easily match
what we see inside the Ruby process (because we can log the
`Thread#object_id` where something happens) with the profile.

BUT if we wanted to match the profile with what the operating system's
tooling showed, it was quite hard, because threads shown in the
OS task manager tools show an OS-level thread identifier, which is
not the same as the `Thread#object_id`.

To enable this matching, this PR changes the format of the
`thread id` from `"#{thread.object_id}"` to
`"#{thread.native_object_id} (#{thread.object_id})"` thus providing
both identifers. (This is OK with the profiling backend).

Because `Thread#native_object_id` is a Ruby 3.1+ feature, on older
Rubies we use a fallback identifier instead (`pthread_t`). This
identifier isn't as useful as the native identifier; in the future
we may want to explore a better fallback.

**Motivation**:

I found this helpful during development and when correlating
data from profiler with other tools.

**Additional Notes**:

This includes full support for macOS, even though we don't
officially support macOS.

This feature is only available on the new CPU Profiling 2.0
profiler which is still in alpha, see #2209.

**How to test the change?**:

This is easiest to test by profiling a Ruby app, and then checking
that the native ids for threads match up with what shows up on
`/proc`. (Remember, only for >= 3.1, for older Rubies it won't match.)
@ivoanjo ivoanjo requested a review from a team November 2, 2022 11:41
@github-actions github-actions bot added the profiling Involves Datadog profiling label Nov 2, 2022
@@ -625,7 +625,7 @@ static struct per_thread_context *get_context_for(VALUE thread, struct cpu_and_w
}

static void initialize_context(VALUE thread, struct per_thread_context *thread_context) {
snprintf(thread_context->thread_id, THREAD_ID_LIMIT_CHARS, "%ld", thread_id_for(thread));
snprintf(thread_context->thread_id, THREAD_ID_LIMIT_CHARS, "%"PRIu64" (%lu)", native_thread_id_for(thread), (unsigned long) thread_id_for(thread));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PRIu64 TIL

@@ -122,6 +122,9 @@ def add_compiler_flag(flag)
# On older Rubies, there was no struct rb_native_thread. See private_vm_api_acccess.c for details.
$defs << '-DNO_RB_NATIVE_THREAD' if RUBY_VERSION < '3.2'

# On older Rubies, there was no tid member in the internal thread structure
$defs << '-DNO_THREAD_TID' if RUBY_VERSION < '3.1'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's so new!

Copy link
Member

@marcotc marcotc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔥

@ivoanjo ivoanjo merged commit f6f7a53 into master Nov 8, 2022
@ivoanjo ivoanjo deleted the ivoanjo/improve-thread-id branch November 8, 2022 10:11
@github-actions github-actions bot added this to the 1.6.0 milestone Nov 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
profiling Involves Datadog profiling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants