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

Fix profiler sometimes not adding the "In native code" placeholder #2594

Merged
merged 1 commit into from
Feb 3, 2023

Conversation

ivoanjo
Copy link
Member

@ivoanjo ivoanjo commented 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.

**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.
@ivoanjo ivoanjo requested a review from a team February 2, 2023 11:52
@github-actions github-actions bot added the profiling Involves Datadog profiling label Feb 2, 2023
@ivoanjo ivoanjo merged commit ab0e812 into master Feb 3, 2023
@ivoanjo ivoanjo deleted the ivoanjo/fix-flaky-stack-in-native-code-detection branch February 3, 2023 16:20
@github-actions github-actions bot added this to the 1.10.0 milestone Feb 3, 2023
@TonyCTHsu TonyCTHsu mentioned this pull request Mar 6, 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.

2 participants