-
Notifications
You must be signed in to change notification settings - Fork 375
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
Changes from 1 commit
bcf3991
12b3d63
657f8bf
ed08f40
5e25e1f
7e5066d
125d6e4
2e1de86
66be4eb
dd15c2e
cd766bb
17952c4
3b39340
ba20d12
5b1f118
ec3426e
f120f8f
ff0d80c
95c976b
cef3c4f
ed60efa
54d04aa
f1dd12d
7a8b12d
6882a74
c985b2b
b8ce73c
d75b446
4cae1cc
c77a571
96c94eb
e2ac427
0093e62
72d5102
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,144 @@ | ||
# frozen_string_literal: true | ||
|
||
require "concurrent/map" | ||
|
||
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 | ||
def initialize | ||
@registry = Concurrent::Map.new | ||
@lock = Mutex.new | ||
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[path] = tp.instruction_sequence | ||
end | ||
|
||
@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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? | ||
@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) | ||
exact = registry[suffix] | ||
if exact | ||
return [exact] | ||
end | ||
inexact = [] | ||
registry.each do |path, iseq| | ||
# 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
|
||
) && path.end_with?(suffix) | ||
inexact << iseq | ||
end | ||
end | ||
inexact | ||
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. | ||
@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.clear | ||
end | ||
|
||
private | ||
|
||
# Mapping from paths of loaded files to RubyVM::InstructionSequence | ||
# objects representing compiled code of those files. | ||
attr_reader :registry | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
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] | ||
end | ||
end | ||
end |
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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
class CodeTrackerTestClass1 | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
class CodeTrackerTestClass2 | ||
end |
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 |
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.
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 becauseiseqs_for_path
may be concurrent with the tracepoint, but usually there will be no concurrency between multiple invocations foriseqs_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)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.
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.
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.
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.