Minor: Import upstream safety check into the profiler custom rb_profile_frames
#2583
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ile_frames` **What does this PR do?**: This PR imports the fix in ruby/ruby#7116 into the custom version of `rb_profile_frames` that we ship inside ddtrace. **Motivation**: As I mention in the inline comments, I don't expect this extra safety check to ever be triggered, but just in case I decided to add it anyway, since the cost of getting my expectation wrong is a VM crash. **Additional Notes**: N/A **How to test the change?**: Adding test coverage for this is... difficult. Our current test of this logic is indirect (e.g. via our own code for sampling), and reproducing the state to trigger this is hard. So for now I only added the check without extra testing.
TonyCTHsu
approved these changes
Jan 25, 2023
ivoanjo
added a commit
that referenced
this pull request
Feb 2, 2023
**What does this PR do?**: In #2583 we added a very small check to our custom `ddtrace_rb_profile_frames` to match a similar fix added to upstream Ruby. But I've just spotted this caused one of the profiling tests to become flaky: ``` $ bundle exec rspec spec/datadog/profiling --seed 14514 Failures: 1) Datadog::Profiling::Collectors::Stack when sampling a thread with empty locations gathers a one-element stack with a "In native code" placeholder Failure/Error: expect(gathered_stack).to contain_exactly({ base_label: '', path: 'In native code', lineno: 0 }) expected collection contained: [{:base_label=>"", :lineno=>0, :path=>"In native code"}] actual collection contained: [] the missing elements were: [{:base_label=>"", :lineno=>0, :path=>"In native code"}] # ./spec/datadog/profiling/collectors/stack_spec.rb:373:in `block (3 levels) in <top (required)>' # ./spec/spec_helper.rb:220:in `block (2 levels) in <top (required)>' # ./spec/spec_helper.rb:112:in `block (2 levels) in <top (required)>' # /home/ivo.anjo/.rvm/gems/ruby-2.7.7/gems/webmock-3.13.0/lib/webmock/rspec.rb:37:in `block (2 levels) in <top (required)>' 2) Datadog::Profiling::Collectors::Stack when sampling a thread with empty locations when marking sample as being in garbage collection gathers a two-element stack with a placeholder for "In native code" and another for garbage collection Failure/Error: expect(gathered_stack).to contain_exactly( { base_label: '', path: 'Garbage Collection', lineno: 0 }, { base_label: '', path: 'In native code', lineno: 0 } ) expected collection contained: [{:base_label=>"", :lineno=>0, :path=>"Garbage Collection"}, {:base_label=>"", :lineno=>0, :path=>"In native code"}] actual collection contained: [{:base_label=>"", :lineno=>0, :path=>"Garbage Collection"}] the missing elements were: [{:base_label=>"", :lineno=>0, :path=>"In native code"}] # ./spec/datadog/profiling/collectors/stack_spec.rb:380:in `block (4 levels) in <top (required)>' # ./spec/spec_helper.rb:220:in `block (2 levels) in <top (required)>' # ./spec/spec_helper.rb:112:in `block (2 levels) in <top (required)>' # /home/ivo.anjo/.rvm/gems/ruby-2.7.7/gems/webmock-3.13.0/lib/webmock/rspec.rb:37:in `block (2 levels) in <top (required)>' Finished in 13.42 seconds (files took 0.44192 seconds to load) 563 examples, 2 failures, 15 pending Failed examples: rspec ./spec/datadog/profiling/collectors/stack_spec.rb:372 # Datadog::Profiling::Collectors::Stack when sampling a thread with empty locations gathers a one-element stack with a "In native code" placeholder rspec ./spec/datadog/profiling/collectors/stack_spec.rb:379 # Datadog::Profiling::Collectors::Stack when sampling a thread with empty locations when marking sample as being in garbage collection gathers a two-element stack with a placeholder for "In native code" and another for garbage collection ``` This issue was caused by the `cfp == NULL` check being added before the `end_cfp == NULL` check we have to detect when we should add the "In native code" placeholder. In some cases, `cfp` is set and the placeholder is added, but in others, it's not. To fix this, I've swapped the order of the checks, so that the check to introduce the placeholder gets higher priority. **Motivation**: Fix bug (that also causes flaky tests). **Additional Notes**: (N/A) **How to test the change?**: Validate that running profiling test suite works with for instance seed 14154.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do?:
This PR imports the fix in ruby/ruby#7116 into the custom version of
rb_profile_framesthat we ship inside ddtrace.Motivation:
As I mention in the inline comments, I don't expect this extra safety check to ever be triggered, but just in case I decided to add it anyway, since the cost of getting my expectation wrong is a VM crash.
Additional Notes:
N/A
How to test the change?:
Adding test coverage for this is... difficult. Our current test of this logic is indirect (e.g. via our own code for sampling), and reproducing the state to trigger this is hard.
So for now I only added the check without extra testing.