-
Notifications
You must be signed in to change notification settings - Fork 381
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
update internals so that the code is a little bit more idiomatic #4
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yay! Useful docs! Better comments! Clearer code!
# ensures that the internal is readable. | ||
return span.trace(&Proc.new) if block_given? | ||
span.trace | ||
span |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a typo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually it's weird but the current span.trace()
implementation doesn't return the span
properly. I want to merge both to remove this weird approach that seems a typo. By the way, if I remove the span
, it will not be returned to the caller and so I cannot use the instance.
Your comment is totally right and I want to fix that with a proper merge of both methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly this is what happens without the latest span
:
Run options: --seed 45376
# Running:
.......F.E..
Finished in 0.116818s, 102.7236 runs/s, 428.0151 assertions/s.
1) Failure:
TracerTest#test_trace_no_block [/Users/emanuele/workspaces/dd/dd-trace-rb/test/tracer_test.rb:23]:
Expected false to be truthy.
2) Error:
TracerTest#test_trace_no_block_not_finished:
NoMethodError: undefined method `end_time' for nil:NilClass
/Users/emanuele/workspaces/dd/dd-trace-rb/test/tracer_test.rb:73:in `test_trace_no_block_not_finished'
12 runs, 50 assertions, 1 failures, 1 errors, 0 skips
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And you need to call span.trace
before returning span
? If so, can you add a comment explaining that this isn't a typo, until you fix it (unless you plan on fixing it pretty immediately)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately yes. That's why I want to merge both methods so that we can easily understand what this method does. I'll add a comment until it's fixed in the next PR!
I have a couple of general ruby style comments. I don't know how general these are but I've found them to be useful in my work and at companies I've been at. When accessing an element in a hash, I prefer def f(foo, bar, opts={})
do_some_stuff(foo)
also_something_else(opt[:start_date])
end
def somewhere_else(start)
foo = 5
bar = 'pizza'
f(foo, bar, date_start: start)
end I've seen bugs where a typo or refactor causes the wrong hash item to be accessed. This is annoying if it causes a crash somewhere down the call chain and you waste some time figuring out why some variable is nil when it shouldn't be. It's worse if the code proceeds and does the wrong thing, maybe because something treats a nil as "use the default value". Using Anyway, totally up to y'all! |
Totally agree with both suggestions. About the Updating the PR with your suggestions! |
return if parent.nil? | ||
@parent = parent |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You intend that set_parent(nil)
is silently ignore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch! think I did a mistake during the refactoring. I will add a test for that because if the parent is set to nil
, we should ensure that the parent_id
and the parent
attribute are set to nil
too!
Looks great! |
Last fix and I'm going to merge that! Thank you! 😄 |
@dbenamydd done! Going to merge that! |
**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. ```
What it does