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

[PROF-7440] Add fallback name/invoke location for unnamed threads started in native code #2993

Merged
merged 2 commits into from
Jul 25, 2023

Conversation

ivoanjo
Copy link
Member

@ivoanjo ivoanjo commented Jul 24, 2023

What does this PR do?:

This PR adds a fallback invoke location (which gets used as a name) for threads that a) Are unnamed, and b) Were started from native code.

The added invoke location is the fixed string (Unnamed thread from native gem) (Unnamed thread from native code).

Motivation:

We previously (in #2950) started using invoke location for threads that do not have a name BUT when a thread is started from native code it would have an empty name.

Having an empty name is not particularly helpful, so I decided to instead have a simple placeholder that at least provides an hint on what the thread may be.

Additional Notes:

We don't yet do any resolving of symbols from native code, but in the future I guess we may try to see if the native code has a name at the C level, and use that as a fallback, but that's much more than a one-liner change :)

How to test the change?:

Change includes code coverage.

…rted in native code

**What does this PR do?**:

This PR adds a fallback invoke location (which gets used as a name) for
threads that a) Are unnamed, and b) Were started from native code.

The added invoke location is the fixed string
`(Unnamed thread from native gem)`.

**Motivation**:

We previously (in #2950) started using invoke location for threads
that do not have a name BUT when a thread is started from native
code it would have an empty name.

Having an empty name is not particularly helpful, so I decided to
instead have a simple placeholder that at least provides an hint on
what the thread may be.

**Additional Notes**:

We don't yet do any resolving of symbols from native code, but in
the future I guess we may try to see if the native code has a name
at the C level, and use that as a fallback, but that's much more
than a one-liner change :)

**How to test the change?**:

Change includes code coverage.
@ivoanjo ivoanjo requested a review from a team July 24, 2023 13:41
@github-actions github-actions bot added the profiling Involves Datadog profiling label Jul 24, 2023
@codecov-commenter
Copy link

Codecov Report

Merging #2993 (be60c81) into master (d6fd258) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2993      +/-   ##
==========================================
- Coverage   98.09%   98.09%   -0.01%     
==========================================
  Files        1305     1305              
  Lines       72731    72736       +5     
  Branches     3366     3366              
==========================================
+ Hits        71345    71348       +3     
- Misses       1386     1388       +2     
Impacted Files Coverage Δ
...atadog/profiling/collectors/thread_context_spec.rb 98.30% <100.00%> (+0.02%) ⬆️

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

// If the first function of a thread is native code, there won't be an invoke location, so we use this fallback.
// NOTE: In the future, I wonder if we could take the pointer to the native function, and try to see if there's a native
// symbol attached to it.
snprintf(thread_context->thread_invoke_location, THREAD_INVOKE_LOCATION_LIMIT_CHARS, "%s", "(Unnamed thread from native gem)");
Copy link
Member

Choose a reason for hiding this comment

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

Is it always a thread from a "gem"? Could this thread have originated from the current application's native code?

Copy link
Member Author

Choose a reason for hiding this comment

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

You know what, your question gave me quite a bit of pause.

This being Ruby, the answer is of course "everything is possible", but then I started thinking it would be kinda awkward it would be, since you wouldn't get the automatic compilation behavior on install like you get for gems. On the other hand, I can imagine if you need to interact with some weird hardware (like for a point of sale), you may end up doing that anyway.

In the end, as we already had another placeholder "In native code", I changed this one to be (Unnamed thread from native code) to match it in c52c732 .

@ivoanjo ivoanjo merged commit c4e1f97 into master Jul 25, 2023
@ivoanjo ivoanjo deleted the ivoanjo/name-fallback-native-threads branch July 25, 2023 08:15
@github-actions github-actions bot added this to the 1.13.0 milestone Jul 25, 2023
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.

3 participants