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

DEBUG-2334 Dynamic Instrumentation code tracker component #3942

Merged
merged 34 commits into from
Oct 2, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
bcf3991
DEBUG-2334 Dynamic Instrumentation code tracker component
p Sep 23, 2024
12b3d63
Remove Concurrent::Map usage to avoid dependency on concurrent-ruby
p Sep 24, 2024
657f8bf
Update spec/datadog/di/code_tracker_spec.rb
p-datadog Sep 25, 2024
ed08f40
Update lib/datadog/di/code_tracker.rb
p-datadog Sep 25, 2024
5e25e1f
Update lib/datadog/di/code_tracker.rb
p-datadog Sep 25, 2024
7e5066d
Update lib/datadog/di/code_tracker.rb
p-datadog Sep 25, 2024
125d6e4
Update spec/datadog/di/code_tracker_spec.rb
p-datadog Sep 25, 2024
2e1de86
fix registry lock type
p Sep 25, 2024
66be4eb
start docstring
p Sep 25, 2024
dd15c2e
put entire start method under trace point lock
p Sep 25, 2024
cd766bb
Merge branch 'master' into di-code-tracker
p-datadog Sep 25, 2024
17952c4
use patched rbs to get RubyVM::InstructionSequence method type defini…
p Sep 25, 2024
3b39340
mark tests as di tests because needed trace points do not exist on ru…
p Sep 25, 2024
ba20d12
add spec helper require
p Sep 25, 2024
5b1f118
Merge branch 'master' into di-code-tracker
p-datadog Sep 26, 2024
ec3426e
skip DI tests on ruby 2.5
p Sep 27, 2024
f120f8f
Merge branch 'master' into di-code-tracker
p-datadog Sep 27, 2024
ff0d80c
standard
p Sep 27, 2024
95c976b
Update lib/datadog/di/code_tracker.rb
p-datadog Sep 30, 2024
cef3c4f
standard
p Sep 30, 2024
ed60efa
delete constructor test
p Sep 30, 2024
54d04aa
rbs patch was accepted, use latest rbs release
p Sep 30, 2024
f1dd12d
note .trace enables trace point
p Oct 1, 2024
7a8b12d
break out load & require test cases
p Oct 1, 2024
6882a74
do not track evaled code
p Oct 1, 2024
c985b2b
Update spec/datadog/di/code_tracker_spec.rb
p-datadog Oct 1, 2024
b8ce73c
standard
p Oct 1, 2024
d75b446
separate eval test cases with and without explicit location
p Oct 1, 2024
4cae1cc
document eval path situation more
p Oct 1, 2024
c77a571
standard
p Oct 1, 2024
96c94eb
Merge branch 'master' into di-code-tracker
p-datadog Oct 1, 2024
e2ac427
init instance var to squelch ruby 2.6 warning
p Oct 1, 2024
0093e62
fix on ruby 3.0 and earlier
p Oct 1, 2024
72d5102
steep
p Oct 1, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
152 changes: 152 additions & 0 deletions lib/datadog/di/code_tracker.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,152 @@
# frozen_string_literal: true

module Datadog
module DI
# Tracks loaded Ruby code by source file and maintains a map from
# source file to the loaded code (instruction sequences).
# Also arranges for code in the loaded files to be instrumented by
# line probes that have already been received by the library.
#
# The loaded code is used to target line trace points when installing
# line probes which dramatically improves efficiency of line trace points.
#
# Note that, since most files will only be loaded one time (via the
# "require" mechanism), the code tracker needs to be global and not be
# recreated when the DI component is created.
#
# @api private
class CodeTracker
Comment on lines +5 to +18
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be worth documenting what parts of this class are concurrent and why.

e.g.If I understand it well (and I'm speculating/reverse engineering), the concurrency in registry is because iseqs_for_path may be concurrent with the tracepoint, but usually there will be no concurrency between multiple invocations for iseqs_for_path (presumably because remote configuration apply is sequential).

(I don't quite understand when there can be concurrency in start/stop; given that usually the dd-trace-rb components system takes care of enforcing that starting and stopping things is a sequential operation as well)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

start & stop should not be running at the same time. start/stop could (I suppose) be running while the trace point is invoked. Since starting and stopping should happen once and never in normal usage, I reused the mutexes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll admit that if you confirm that start/stop should not be concurrent then I don't quite understand what trace_point_lock is protecting 🤔

I saw you setup a meeting to discuss this, so let's continue the discussion there. In any case, I don't think the discussion should be a blocker to merging this PR, as we can always tweak this in a later PR.

def initialize
@registry = Hash.new
p-datadog marked this conversation as resolved.
Show resolved Hide resolved
@trace_point_lock = Mutex.new
@registry_lock = Hash.new
p-datadog marked this conversation as resolved.
Show resolved Hide resolved
end

p-datadog marked this conversation as resolved.
Show resolved Hide resolved
def start
p-datadog marked this conversation as resolved.
Show resolved Hide resolved
# If this code tracker is already running, we can do nothing or
# restart it (by disabling the trace point and recreating it).
# It is likely that some applications will attempt to activate
# DI more than once where the intention is to just activate DI;
# do not break such applications by clearing out the registry.
# For now, until there is a use case for recreating the trace point,
# do nothing if the code tracker has already started.
return if active?

compiled_trace_point = TracePoint.trace(:script_compiled) do |tp|
# Useful attributes of the trace point object here:
# .instruction_sequence
# .method_id
# .path (refers to the code location that called the require/eval/etc.,
# not where the loaded code is; use .path on the instruction sequence
# to obtain the location of the compiled code)
# .eval_script
#
# For now just map the path to the instruction sequence.
path = tp.instruction_sequence.path
registry_lock.synchronize do
registry[path] = tp.instruction_sequence
end
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does script_compiled get emitted for each individual method in a file?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it should be emitted once per file.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, in that case, how do we know the correct iseq to target, if 1 file has N iseqs? (Or I may be misunderstanding how this works?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is one iseq per file.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting... I think my mental model was slightly off for this one.

As you pointed out taking for instance

# test.rb

def a
  puts "a!"
end

def b
  puts "b!"
end

and doing

[1] pry(main)> iseq = RubyVM::InstructionSequence.compile(File.read("test.rb"))
[3] pry(main)> puts RubyVM::InstructionSequence.disasm(iseq)
== disasm: #<ISeq:<compiled>@<compiled>:1 (1,0)-(8,3)> (catch: FALSE)
0000 definemethod                           :a, a                     (   2)[Li]
0003 definemethod                           :b, b                     (   6)[Li]
0006 putobject                              :b
0008 leave

== disasm: #<ISeq:a@<compiled>:2 (2,0)-(4,3)> (catch: FALSE)
0000 putself                                                          (   3)[LiCa]
0001 putstring                              "a!"
0003 opt_send_without_block                 <calldata!mid:puts, argc:1, FCALL|ARGS_SIMPLE>
0005 leave                                                            (   4)[Re]

== disasm: #<ISeq:b@<compiled>:6 (6,0)-(8,3)> (catch: FALSE)
0000 putself                                                          (   7)[LiCa]
0001 putstring                              "b!"
0003 opt_send_without_block                 <calldata!mid:puts, argc:1, FCALL|ARGS_SIMPLE>
0005 leave                                                            (   8)[Re]
=> nil

e.g. will mean we get it all in one go.

Yet... it seems there's still separate objects for the different iseqs -- there's a RubyVM::InstructionSequence#each_child and the object ids of the objects it returns are different from the top-level iseq so they seem like separate objects arranged in a tree, not one object that's presenting different views of itself.

That said, for the usage we'll be making in DI, maybe this extra distinction doesn't matter very much? And I learned something new :)


trace_point_lock.synchronize do
# Since trace point creation itself is not under a lock, see if
# another thread created the trace point, in which case we can
# disable our trace point and do nothing.
if @compiled_trace_point
# Disable the local variable, leave instance variable as it is.
compiled_trace_point.disable
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it me or was the tracepoint was not enabled before we disable it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TracePoint.trace enables the trace point. .new does not enable. I agree it can be confusing at first glance.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aaaaah that's super subtle, missed it! May be worth adding a comment or using an explicit enable to make it easier on the readers? (but just a suggestion)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a note.

return
end

@compiled_trace_point = compiled_trace_point
end
end

# Returns whether this code tracker has been activated and is
# tracking.
def active?
trace_point_lock.synchronize do
!!@compiled_trace_point
end
end

# Returns an array of RubVM::InstructionSequence (i.e. the compiled code)
# for the provided path.
#
# The argument can be a full path to a Ruby source code file or a
# suffix (basename + one or more directories preceding the basename).
# The idea with suffix matches is that file paths are likely to
# be different between development and production environments and
# the source control system uses relative paths and doesn't have
# absolute paths at all.
#
# Suffix matches are not guaranteed to be correct, meaning there may
# be multiple files with the same basename and they may all match a
# given suffix. In such cases, this method will return all matching
# paths (and all of these paths will be attempted to be instrumented
# by upstream code).
#
# If the suffix matches one of the paths completely (which requires it
# to be an absolute path), only the exactly matching path is returned.
# Otherwise all known paths that end in the suffix are returned.
# If no paths match, an empty array is returned.
def iseqs_for_path(suffix)
registry_lock.synchronize do
exact = registry[suffix]
if exact
return [exact]
end
inexact = []
registry.each do |path, iseq|
p-datadog marked this conversation as resolved.
Show resolved Hide resolved
# Exact match is not possible here, meaning any matching path
# has to be longer than the suffix. Require full component matches,
# meaning either the first character of the suffix is a slash
# or the previous character in the path is a slash.
# For now only check for forward slashes for Unix-like OSes;
# backslash is a legitimate character of a file name in Unix
# therefore simply permitting forward or back slash is not
# sufficient, we need to perform an OS check to know which
# path separator to use.
if path.length > suffix.length && (
path[path.length - suffix.length - 1] == "/" ||
suffix[0] == "/"
p-datadog marked this conversation as resolved.
Show resolved Hide resolved
p-datadog marked this conversation as resolved.
Show resolved Hide resolved
) && path.end_with?(suffix)
inexact << iseq
end
p-datadog marked this conversation as resolved.
Show resolved Hide resolved
end
inexact
end
end

# Stops tracking code that is being loaded.
#
# This method should ordinarily never be called - if a file is loaded
# when code tracking is not active, this file will not be instrumentable
# by line probes.
#
# This method is intended for test suite use only, where multiple
# code tracker instances are created, to fully clean up the old instances.
def stop
# Permit multiple stop calls.
trace_point_lock.synchronize do
@compiled_trace_point&.disable
# Clear the instance variable so that the trace point may be
# reinstated in the future.
@compiled_trace_point = nil
end
registry_lock.synchronize do
registry.clear
end
end

private

# Mapping from paths of loaded files to RubyVM::InstructionSequence
# objects representing compiled code of those files.
attr_reader :registry

attr_reader :trace_point_lock
attr_reader :registry_lock
end
end
end
23 changes: 23 additions & 0 deletions sig/datadog/di/code_tracker.rbs
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
module Datadog
module DI
class CodeTracker
@registry: Hash[String,RubyVM::InstructionSequence]

@lock: Thread::Mutex

@compiled_trace_point: TracePoint?

def initialize: () -> void

def start: () -> void
def active?: () -> bool
def iseqs_for_path: (String suffix) -> (::Array[RubyVM::InstructionSequence])
def stop: () -> void

private
attr_reader registry: Hash[String,RubyVM::InstructionSequence]
attr_reader trace_point_lock: Thread::Mutex
attr_reader registry_lock: Thread::Mutex
end
end
end
119 changes: 119 additions & 0 deletions spec/datadog/di/code_tracker_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,119 @@
require "datadog/di/code_tracker"

RSpec.describe Datadog::DI::CodeTracker do
let(:tracker) do
described_class.new
end

describe ".new" do
it "creates an instance" do
expect(tracker).to be_a(described_class)
end
end
p-datadog marked this conversation as resolved.
Show resolved Hide resolved

describe "#start" do
after do
tracker.stop
end

it "tracks loaded files" do
# The expectations appear to be lazy-loaded, therefore
# we need to invoke the same expectation before starting
# code tracking as we'll be using later in the test.
expect(tracker.send(:registry)).to be_empty
tracker.start
# Should still be empty here.
expect(tracker.send(:registry)).to be_empty
load File.join(File.dirname(__FILE__), "code_tracker_test_class_1.rb")
expect(tracker.send(:registry).each.to_a.length).to eq(1)
p-datadog marked this conversation as resolved.
Show resolved Hide resolved

path = tracker.send(:registry).each.to_a.first.first
p-datadog marked this conversation as resolved.
Show resolved Hide resolved
# The path in the registry should be absolute.
expect(path[0]).to eq "/"
p-datadog marked this conversation as resolved.
Show resolved Hide resolved
p-datadog marked this conversation as resolved.
Show resolved Hide resolved
# The full path is dependent on the environment/system
# running the tests, but we can assert on the basename
# which will be the same.
expect(File.basename(path)).to eq("code_tracker_test_class_1.rb")
# And, we should in fact have a full path.
expect(path).to start_with("/")
end
end

describe "#active?" do
context "when started" do
before do
tracker.start
end

after do
tracker.stop
end

it "is true" do
expect(tracker.active?).to be true
end
end

context "when stopped" do
before do
tracker.start
tracker.stop
end

it "is false" do
expect(tracker.active?).to be false
end
end
end

describe "#iseqs_for_path" do
around do |example|
tracker.start

load File.join(File.dirname(__FILE__), "code_tracker_test_class_1.rb")
load File.join(File.dirname(__FILE__), "code_tracker_test_class_2.rb")
load File.join(File.dirname(__FILE__), "code_tracker_test_class_3.rb")
load File.join(File.dirname(__FILE__), "code_tracker_test_classes", "code_tracker_test_class_1.rb")
expect(tracker.send(:registry).each.to_a.length).to eq(4)
y9v marked this conversation as resolved.
Show resolved Hide resolved

# To be able to assert on the registry, replace values (iseqs)
# with the keys.
(registry = tracker.send(:registry)).each do |k, v|
registry[k] = k
end

example.run

tracker.stop
end

context "exact match for full path" do
let(:path) do
File.join(File.dirname(__FILE__), "code_tracker_test_class_1.rb")
end

it "returns the exact match only" do
expect(tracker.iseqs_for_path(path)).to eq([path])
end
end

context "basename match" do
let(:expected) do
[
File.join(File.dirname(__FILE__), "code_tracker_test_class_1.rb"),
File.join(File.dirname(__FILE__), "code_tracker_test_classes", "code_tracker_test_class_1.rb"),
]
end

it "returns the exact match only" do
expect(tracker.iseqs_for_path("code_tracker_test_class_1.rb")).to eq(expected)
end
end

context "match not on path component boundary" do
it "returns no matches" do
expect(tracker.iseqs_for_path("1.rb")).to eq([])
end
end
end
end
2 changes: 2 additions & 0 deletions spec/datadog/di/code_tracker_test_class_1.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
class CodeTrackerTestClass1
end
2 changes: 2 additions & 0 deletions spec/datadog/di/code_tracker_test_class_2.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
class CodeTrackerTestClass2
end
2 changes: 2 additions & 0 deletions spec/datadog/di/code_tracker_test_class_3.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
class CodeTrackerTestClass3
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
# Different name to not conflict with the upper-level class definition.
# Note that file basenames need to be identical for some of the test cases.
class SubdirCodeTrackerTestClass1
end
Loading