add Rubocop for style guidelines#3
Merged
Merged
Conversation
clutchski
reviewed
Oct 4, 2016
| powerpack (0.1.1) | ||
| rainbow (2.1.0) | ||
| rake (10.5.0) | ||
| rubocop (0.43.0) |
Contributor
There was a problem hiding this comment.
these are just dev deps right?
Contributor
Author
There was a problem hiding this comment.
Yes. Actually the tracer requires third-party libraries only for development: https://github.com/DataDog/dd-trace-rb/pull/3/files#diff-b06e29a984e208fededfd0c6e60ca9ecR35
When the GEM is created, these dependencies are not installed.
p-datadog
pushed a commit
that referenced
this pull request
Nov 24, 2024
In the previous PR I mistakenly placed the 'gem' instruction into a spring test. It was intended to be in the cucumber test.
TonyCTHsu
pushed a commit
that referenced
this pull request
Nov 24, 2024
In the previous PR I mistakenly placed the 'gem' instruction into a spring test. It was intended to be in the cucumber test.
ivoanjo
added a commit
that referenced
this pull request
Dec 2, 2024
**What does this PR do?** This PR raises the minimum Ruby version required for heap profiling from the previous value of >= 2.7 to >= 3.1 due to a new VM bug discovered (see below for details). It's mostly a revert of #3366, where we had first tried to workaround a Ruby 2.7/3.0 bug, but it turns out we missed a spot, and we could trigger VM crashes because of that. **Motivation:** Ruby versions prior to 3.1 had a special optimization called `rb_gc_force_recycle` which would allow objects to directly be garbage collected (e.g. without needing to wait for the GC). It turns out that `rb_gc_force_recycle` did not play well with the changes in Ruby 2.7 to how object ids worked. We uncovered this earlier on during the development of the heap profiler, and put in a workaround for the bug that we thought was enough... Unfortunately, it turns out that the workaround is not enough. The following reproducer, when run on Ruby 2.7 or 3.0 shows how the Ruby VM can segfault inside `id2ref` due to the issue above: ```ruby puts RUBY_DESCRIPTION require "datadog" require "objspace" require "pry" NUM_OBJECTS = 10_000_000 recycled_ids = Array.new(NUM_OBJECTS) { 123 } many_objects = Array.new(NUM_OBJECTS) { Object.new } (0...NUM_OBJECTS).each do |i| recycled_ids[i] = many_objects[i].object_id end puts "Seeded objects!" gets (0...NUM_OBJECTS).each do |i| Datadog::Profiling::StackRecorder::Testing._native_gc_force_recycle(many_objects[i]) many_objects[i] = nil end puts GC.stat puts "Recycled objects!" gets many_objects = nil 10.times { GC.start } Array.new(10_000) { Object.new } 10.times { GC.start } puts GC.stat puts "GC'd objects! (Ruby should have released pages?)" gets recycled_ids.each { |i| begin (nil == ObjectSpace._id2ref(i)) rescue nil end } puts "Done!" ``` Crash details: ``` Program received signal SIGSEGV, Segmentation fault. is_swept_object (ptr=93825033355200, objspace=<optimised out>) at gc.c:3868 3868 return page->flags.before_sweep ? FALSE : TRUE; (gdb) bt #0 is_swept_object (ptr=93825033355200, objspace=<optimised out>) at gc.c:3868 #1 is_garbage_object (objspace=0x55555555d220, objspace=0x55555555d220, ptr=93825033355200) at gc.c:3887 #2 is_live_object (ptr=93825033355200, objspace=0x55555555d220) at gc.c:3909 #3 is_live_object (ptr=93825033355200, objspace=0x55555555d220) at gc.c:3898 #4 id2ref (objid=8264881) at gc.c:3999 #5 os_id2ref (os=<optimised out>, objid=<optimised out>) at gc.c:4019 ``` This crash happens because of two things: 1. Ruby does not clean the object id entry for a recycled object from its internal hash map 2. If the memory page where the object lived is returned back to the OS, trying to `id2ref` on that id will cause Ruby to try to read invalid memory and crash. **Additional Notes:** I've chosen to disable heap profiling on 2.7 and 3.0 because I can't think of a good workaround for the bug above, especially not one that does not increase the overhead of heap profiling. **How to test the change?** This PR updates the test coverage to expect Ruby 3.1+ as the minimum for the feature. You can also quickly validate it doesn't get enabled on the older Rubies using: ``` $ DD_PROFILING_ENABLED=true DD_PROFILING_EXPERIMENTAL_HEAP_ENABLED=true bundle exec ddprofrb exec ruby -e "puts RUBY_DESCRIPTION" W, [2024-12-02T10:42:28.771611 #112585] WARN -- datadog: [datadog] Current Ruby version (3.0.5) cannot support heap profiling due to VM bugs/limitations. Please upgrade to Ruby >= 3.1 in order to use this feature. Heap profiling has been disabled. ```
ivoanjo
added a commit
that referenced
this pull request
May 19, 2025
**What does this PR do?** This PR removes the `test-asan` step in CI as it's currently flaky and I wasn't able to debug the issue or skip it. This run https://github.com/DataDog/dd-trace-rb/actions/runs/15112212122/job/42474072954?pr=4664 shows this step failing: ``` Randomized with seed 3003 ......................................................1.8.1 ==2610==Running thread 2303 was not suspended. False leaks are possible. ==2610==Running thread 2304 was not suspended. False leaks are possible. ==2610==Processing thread 2606. ==2610==Stack at 0x7fff7be8d000-0x7fff7ce8d000 (SP = 0x7fff7ce81630). ==2610==TLS at 0x7f0d63c88b80-0x7f0d63c89cc0. ==2610==DTLS 5 at 0x512000597940-0x512000597a78. ==2610==DTLS 6 at 0x5020000d0e30-0x5020000d0e38. ==2615==Running thread 2303 was not suspended. False leaks are possible. ==2615==Running thread 2304 was not suspended. False leaks are possible. ==2615==Processing thread 2611. ==2615==Stack at 0x7fff7be8d000-0x7fff7ce8d000 (SP = 0x7fff7ce80cb0). ==2615==TLS at 0x7f0d63c88b80-0x7f0d63c89cc0. ==2615==DTLS 5 at 0x512000597940-0x512000597a78. ==2615==DTLS 6 at 0x5020000d0e30-0x5020000d0e38. ================================================================= ==2298==ERROR: AddressSanitizer: use-after-poison on address 0x7f0d48ea00e8 at pc 0x555ee334c0a6 bp 0x7fff7ce80680 sp 0x7fff7ce7fe40 READ of size 83 at 0x7f0d48ea00e8 thread T0 #0 0x555ee334c0a5 in __asan_memcpy (/home/runner/.rubies/ruby-3.4-asan/bin/ruby+0xc50a5) (BuildId: 0436b55d43125d84e3c30c1e201ade3c92f23140) #1 0x7f0d634b3855 in memcpy /usr/include/x86_64-linux-gnu/bits/string_fortified.h:29:10 #2 0x7f0d634b3855 in ruby_nonempty_memcpy /home/runner/work/ruby-dev-builder/ruby-dev-builder/./include/ruby/internal/memory.h:671:16 #3 0x7f0d634b3855 in ruby__sfvwrite /home/runner/work/ruby-dev-builder/ruby-dev-builder/sprintf.c:1083:9 #4 0x7f0d634b2dd1 in BSD__sprint /home/runner/work/ruby-dev-builder/ruby-dev-builder/./vsnprintf.c:318:8 #5 0x7f0d634b286e in BSD_vfprintf /home/runner/work/ruby-dev-builder/ruby-dev-builder/./vsnprintf.c:1215:3 #6 0x7f0d634afced in ruby_vsprintf0 /home/runner/work/ruby-dev-builder/ruby-dev-builder/sprintf.c:1164:5 #7 0x7f0d634b03ee in rb_str_vcatf /home/runner/work/ruby-dev-builder/ruby-dev-builder/sprintf.c:1234:5 #8 0x7f0d634af198 in rb_str_catf /home/runner/work/ruby-dev-builder/ruby-dev-builder/sprintf.c:1245:11 #9 0x7f0d636519e2 in location_format /home/runner/work/ruby-dev-builder/ruby-dev-builder/vm_backtrace.c:456:9 #10 0x7f0d636519e2 in location_to_str /home/runner/work/ruby-dev-builder/ruby-dev-builder/vm_backtrace.c:487:12 #11 0x7f0d6364ccc7 in location_to_str_dmyarg /home/runner/work/ruby-dev-builder/ruby-dev-builder/vm_backtrace.c:783:12 #12 0x7f0d6364ccc7 in backtrace_collect /home/runner/work/ruby-dev-builder/ruby-dev-builder/vm_backtrace.c:774:28 #13 0x7f0d6364ccc7 in backtrace_to_str_ary /home/runner/work/ruby-dev-builder/ruby-dev-builder/vm_backtrace.c:792:9 #14 0x7f0d6364ccc7 in ec_backtrace_to_ary /home/runner/work/ruby-dev-builder/ruby-dev-builder/vm_backtrace.c:1277:13 #15 0x7f0d6362b19e in vm_call_cfunc_with_frame_ /home/runner/work/ruby-dev-builder/ruby-dev-builder/./vm_insnhelper.c:3794:11 #16 0x7f0d635c0f63 in vm_sendish /home/runner/work/ruby-dev-builder/ruby-dev-builder/./vm_callinfo.h #17 0x7f0d635cc606 in vm_exec_core /home/runner/work/ruby-dev-builder/ruby-dev-builder/insns.def:898:11 ...etc... Address 0x7f0d48ea00e8 is a wild pointer inside of access range of size 0x000000000053. SUMMARY: AddressSanitizer: use-after-poison (/home/runner/.rubies/ruby-3.4-asan/bin/ruby+0xc50a5) (BuildId: 0436b55d43125d84e3c30c1e201ade3c92f23140) in __asan_memcpy Shadow bytes around the buggy address: 0x7f0d48e9fe00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x7f0d48e9fe80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x7f0d48e9ff00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x7f0d48e9ff80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x7f0d48ea0000: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 =>0x7f0d48ea0080: 00 00 00 00 00 00 00 00 00 00 f7 f7 f7[f7]f7 f7 0x7f0d48ea0100: f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 0x7f0d48ea0180: f7 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x7f0d48ea0200: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x7f0d48ea0280: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x7f0d48ea0300: 00 00 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 Shadow byte legend (one shadow byte represents 8 application bytes): Addressable: 00 Partially addressable: 01 02 03 04 05 06 07 Heap left redzone: fa Freed heap region: fd Stack left redzone: f1 Stack mid redzone: f2 Stack right redzone: f3 Stack after return: f5 Stack use after scope: f8 Global redzone: f9 Global init order: f6 Poisoned by user: f7 Container overflow: fc Array cookie: ac Intra object redzone: bb ASan internal: fe Left alloca redzone: ca Right alloca redzone: cb ==2298==ABORTING /home/runner/work/dd-trace-rb/dd-trace-rb/spec/spec_helper.rb:280: [BUG] ASAN error ruby 3.4.4 (2025-05-14 revision a38531fd3f) +PRISM [x86_64-linux] -- Control frame information ----------------------------------------------- c:0079 p:---- s:0392 e:000391 CFUNC :caller c:0078 p:0003 s:0388 e:000387 METHOD /home/runner/work/dd-trace-rb/dd-trace-rb/spec/spec_helper.rb:280 [FINISH] c:0077 p:---- s:0381 e:000380 CFUNC :new c:0076 p:0008 s:0376 e:000375 BLOCK /home/runner/work/dd-trace-rb/dd-trace-rb/spec/datadog/profiling/collectors/thread_context_spec.rb:33 c:0075 p:0005 s:0372 e:000371 BLOCK /home/runner/work/dd-trace-rb/dd-trace-rb/vendor/bundle/ruby/3.4.0/gems/rspec-core-3.13.3/lib/rspec/core/memoized_helpers.rb:34 c:0074 p:0009 s:0369 e:000365 BLOCK /home/runner/work/dd-trace-rb/dd-trace-rb/vendor/bundle/ruby/3.4.0/gems/rspec-core-3.13.3/lib/rspec/core/memoized_helpers.rb:17 [FINISH] c:0073 p:---- s:0363 e:000362 CFUNC :fetch c:0072 p:0009 s:0358 e:000357 BLOCK /home/runner/work/dd-trace-rb/dd-trace-rb/vendor/bundle/ruby/3.4.0/gems/rspec-core-3.13.3/lib/rspec/core/memoized_helpers.rb:17 c:0071 p:0006 s:0355 e:000354 METHOD /home/runner/work/dd-trace-rb/dd-trace-rb/vendor/bundle/ruby/3.4.0/gems/rspec-support-3.13.3/lib/rspec/support/reentrant_mutex. c:0070 p:0006 s:0351 e:000350 BLOCK /home/runner/work/dd-trace-rb/dd-trace-rb/vendor/bundle/ruby/3.4.0/gems/rspec-core-3.13.3/lib/rspec/core/memoized_helpers.rb:17 [FINISH] c:0069 p:---- s:0348 e:000347 CFUNC :fetch c:0068 p:0008 s:0343 e:000342 METHOD /home/runner/work/dd-trace-rb/dd-trace-rb/vendor/bundle/ruby/3.4.0/gems/rspec-core-3.13.3/lib/rspec/core/memoized_helpers.rb:17 c:0067 p:0008 s:0338 e:000337 BLOCK /home/runner/work/dd-trace-rb/dd-trace-rb/vendor/bundle/ruby/3.4.0/gems/rspec-core-3.13.3/lib/rspec/core/memoized_helpers.rb:34 c:0066 p:0024 s:0334 e:000331 BLOCK /home/runner/work/dd-trace-rb/dd-trace-rb/spec/datadog/profiling/collectors/thread_context_spec.rb:10 [FINISH] c:0065 p:---- s:0329 e:000328 CFUNC :instance_exec c:0064 p:0013 s:0324 e:000323 METHOD /home/runner/work/dd-trace-rb/dd-trace-rb/vendor/bundle/ruby/3.4.0/gems/rspec-core-3.13.3/lib/rspec/core/example.rb:457 ...etc... ``` On a second run, with no changes, this same spec passed: https://github.com/DataDog/dd-trace-rb/actions/runs/15112212122?pr=4664 **Motivation:** Avoid having flaky CI. **Additional Notes:** This is not the first time we had CI flakiness due to this check, and for a while it was running fine with the upstream "3.4-asan" builds. I plan on trying again once Ruby 3.5 is out. **How to test the change?** Validate the "test-memory-leaks" workflow no longer runs `test-asan`.
p-datadog
pushed a commit
that referenced
this pull request
Feb 27, 2026
Reword alternative #3 to acknowledge that manual instrumentation requires more setup work, rather than presenting it as an equal option. Users should understand this is more effort to maintain. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
p-datadog
pushed a commit
that referenced
this pull request
May 4, 2026
- Compact super_classes to skip anonymous superclasses (review #2) - Use method end_line for class end_line in convert_node_to_scope and calculate_class_line_range; stops underreporting for classes whose last method spans multiple lines (review #4) - Exclude bare 'Datadog' module by name (review #5) - Cache mod_name in extract rescue, removing redundant safe_mod_name call (review #7, nit 104) - Skip autoloaded constants in const_get loops to avoid triggering load as a side effect of extraction (review #1) - Add code comment on included_modules ancestor-chain choice (review #3) - Replace extract_module_symbols/extract_class_symbols callers with extract_scope_symbols and remove the duplicates (review #9) - Drop unused method_type parameter from extract_method_parameters (nit 661) New specs: - resolve_scope_type direct (CLASS, MODULE, NameError fallback) - extract_all top-level rescue → returns [], increments telemetry - bare 'Datadog' module rejected by user_code_module? - anonymous superclass omitted from super_classes
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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 it does
It adds Rubocop as a default tool for style guidelines. Some rules are relaxed because even if they "should" be enforced, at this stage they slow us our review / development cycle. We may enforce other rules later.
Note
chefrepositories to find a kind of "common".rubocop.yml