[PROF-7307] Include 'ruby vm type' in profiler allocation samples #3074
Conversation
**What does this PR do?**: This PR builds extends the existing support for the `ThreadContext` collector to sample allocations to also record the "ruby vm type" of sampled objects. This "ruby vm type" is what Ruby internally represents in its `enum ruby_value_type`, see https://github.com/ruby/ruby/blob/84a12d657848dfb54f8cc556d344f017a793e95a/include/ruby/internal/value_type.h#L112 **Motivation**: I'm working on tagging stacks of sampled objects with both the object class, as well as the ruby vm type. I'm not yet sure how we may expose the ruby vm type in the UX, but having it is useful for development and debugging, and I can imagine it being useful in advanced use cases (e.g. a customer downloading a pprof to look at this data). As libdatadog interns labels (DataDog/libdatadog#205), the cost of tracking this data is quite low. **Additional Notes**: Allocation profiling is still not something that can be enabled; e.g. right now we can only exercise it directly by calling the `ThreadContext` collector. **How to test the change?**: Change includes test coverage.
Codecov Report
@@ Coverage Diff @@
## master #3074 +/- ##
=======================================
Coverage 98.13% 98.13%
=======================================
Files 1328 1328
Lines 75128 75139 +11
Branches 3421 3421
=======================================
+ Hits 73725 73736 +11
Misses 1403 1403
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| #ifdef RUBY_T_MOVED // Introduced in Ruby 2.7 | ||
| case(RUBY_T_MOVED ): return "T_MOVED"; | ||
| #endif | ||
| default: return "T_UNKNOWN_OR_MISSING_RUBY_VALUE_TYPE_ENTRY"; |
There was a problem hiding this comment.
Is this a default created for this PR? I couldn't find it the ruby's source code.
If so, I'd say keep it different from the others, and I find it quite hard to parse such a long screaming case string.
I suggest something like "Unknown Ruby Value", or something that will unmistakably read as "Ha, this is not a valid Ruby VM type".
There was a problem hiding this comment.
Changed in d73a72f to BUG: Unknown value for ruby_value_type. As you suspected, the only objective here was to have an obvious wrong value that we could grep in the codebase and tie back to this function, so we can make it look even more as an obvious wrong value.
The previous `#ifdef RUBY_T_MOVED` was actually failing silently; and because it's hard to test this helper, it was easy to miss since it just meant a not-often-used element of the enum was omitted on all Rubies. By instead flipping the check to be not defined, we get a better behavior: the default is to include the enum member, and if we get the logic to disable incorrect, we get a compile error, not a silent bug.
|
/merge |
|
🚂 MergeQueue This merge request is not mergeable yet, because of pending checks/missing approvals. It will be added to the queue as soon as checks pass and/or get approvals. You can remove it from the waiting list with you can cancel this operation by commenting your pull request with |
|
/remove |
dd-devflow
bot
added
mergequeue-status: removed
and removed
mergequeue-status: waiting
labels
|
This merge request was unqueued If you need support, contact us on slack #ci-interfaces! |
|
I was playing with the merge queue feature, please ignore! :) |
What does this PR do?:
This PR builds extends the existing support for the
ThreadContextcollector to sample allocations to also record the "ruby vm type" of sampled objects.This "ruby vm type" is what Ruby internally represents in its
enum ruby_value_type, seehttps://github.com/ruby/ruby/blob/84a12d657848dfb54f8cc556d344f017a793e95a/include/ruby/internal/value_type.h#L112
Motivation:
I'm working on tagging stacks of sampled objects with both the object class, as well as the ruby vm type.
I'm not yet sure how we may expose the ruby vm type in the UX, but having it is useful for development and debugging, and I can imagine it being useful in advanced use cases (e.g. a customer downloading a pprof to look at this data).
As libdatadog interns labels (DataDog/libdatadog#205), the cost of tracking this data is quite low.
Additional Notes:
Allocation profiling is still not something that can be enabled; e.g. right now we can only exercise it directly by calling the
ThreadContextcollector.How to test the change?:
Change includes test coverage.