Merge pull request #171 from DataDog/anmarchenko/code_coverage_improv…

…ements [SDTEST-138] code coverage extension fixes and improvements
DataDog · Jun 10, 2024 · 6dd395c · 6dd395c
2 parents f58bb35 + b04133e
commit 6dd395c
Show file tree

Hide file tree

Showing 5 changed files with 212 additions and 252 deletions.
diff --git a/ext/datadog_cov/datadog_cov.c b/ext/datadog_cov/datadog_cov.c
@@ -1,64 +1,51 @@
 #include <ruby.h>
 #include <ruby/debug.h>
 
-// constants
-#define DD_COV_TARGET_FILES 1
-#define DD_COV_TARGET_LINES 2
+const int PROFILE_FRAMES_BUFFER_SIZE = 1;
 
-static int is_prefix(VALUE prefix, const char *str)
+char *ruby_strndup(const char *str, size_t size)
 {
-  if (prefix == Qnil)
-  {
-    return 0;
-  }
+  char *dup;
 
-  const char *c_prefix = RSTRING_PTR(prefix);
-  if (c_prefix == NULL)
-  {
-    return 0;
-  }
+  dup = xmalloc(size + 1);
+  memcpy(dup, str, size);
+  dup[size] = '\0';
 
-  long prefix_len = RSTRING_LEN(prefix);
-  if (strncmp(c_prefix, str, prefix_len) == 0)
-  {
-    return 1;
-  }
-  else
-  {
-    return 0;
-  }
+  return dup;
 }
 
 // Data structure
 struct dd_cov_data
 {
-  VALUE root;
-  VALUE ignored_path;
-  int mode;
+  char *root;
+  long root_len;
+
+  char *ignored_path;
+  long ignored_path_len;
+
   VALUE coverage;
+
+  uintptr_t last_filename_ptr;
 };
 
 static void dd_cov_mark(void *ptr)
 {
   struct dd_cov_data *dd_cov_data = ptr;
   rb_gc_mark_movable(dd_cov_data->coverage);
-  rb_gc_mark_movable(dd_cov_data->root);
-  rb_gc_mark_movable(dd_cov_data->ignored_path);
 }
 
 static void dd_cov_free(void *ptr)
 {
   struct dd_cov_data *dd_cov_data = ptr;
-
+  xfree(dd_cov_data->root);
+  xfree(dd_cov_data->ignored_path);
   xfree(dd_cov_data);
 }
 
 static void dd_cov_compact(void *ptr)
 {
   struct dd_cov_data *dd_cov_data = ptr;
   dd_cov_data->coverage = rb_gc_location(dd_cov_data->coverage);
-  dd_cov_data->root = rb_gc_location(dd_cov_data->root);
-  dd_cov_data->ignored_path = rb_gc_location(dd_cov_data->ignored_path);
 }
 
 const rb_data_type_t dd_cov_data_type = {
@@ -74,113 +61,110 @@ static VALUE dd_cov_allocate(VALUE klass)
 {
   struct dd_cov_data *dd_cov_data;
   VALUE obj = TypedData_Make_Struct(klass, struct dd_cov_data, &dd_cov_data_type, dd_cov_data);
+
   dd_cov_data->coverage = rb_hash_new();
-  dd_cov_data->root = Qnil;
-  dd_cov_data->ignored_path = Qnil;
-  dd_cov_data->mode = DD_COV_TARGET_FILES;
+  dd_cov_data->root = NULL;
+  dd_cov_data->root_len = 0;
+  dd_cov_data->ignored_path = NULL;
+  dd_cov_data->ignored_path_len = 0;
+  dd_cov_data->last_filename_ptr = 0;
+
   return obj;
 }
 
 // DDCov methods
 static VALUE dd_cov_initialize(int argc, VALUE *argv, VALUE self)
 {
   VALUE opt;
-  int mode;
 
   rb_scan_args(argc, argv, "10", &opt);
   VALUE rb_root = rb_hash_lookup(opt, ID2SYM(rb_intern("root")));
   if (!RTEST(rb_root))
   {
     rb_raise(rb_eArgError, "root is required");
   }
-
   VALUE rb_ignored_path = rb_hash_lookup(opt, ID2SYM(rb_intern("ignored_path")));
 
-  VALUE rb_mode = rb_hash_lookup(opt, ID2SYM(rb_intern("mode")));
-  if (!RTEST(rb_mode) || rb_mode == ID2SYM(rb_intern("files")))
-  {
-    mode = DD_COV_TARGET_FILES;
-  }
-  else if (rb_mode == ID2SYM(rb_intern("lines")))
-  {
-    mode = DD_COV_TARGET_LINES;
-  }
-  else
-  {
-    rb_raise(rb_eArgError, "mode is invalid");
-  }
-
   struct dd_cov_data *dd_cov_data;
   TypedData_Get_Struct(self, struct dd_cov_data, &dd_cov_data_type, dd_cov_data);
 
-  dd_cov_data->root = rb_root;
-  dd_cov_data->ignored_path = rb_ignored_path;
-  dd_cov_data->mode = mode;
+  dd_cov_data->root_len = RSTRING_LEN(rb_root);
+  dd_cov_data->root = ruby_strndup(RSTRING_PTR(rb_root), dd_cov_data->root_len);
+
+  if (RTEST(rb_ignored_path))
+  {
+    dd_cov_data->ignored_path_len = RSTRING_LEN(rb_ignored_path);
+    dd_cov_data->ignored_path = ruby_strndup(RSTRING_PTR(rb_ignored_path), dd_cov_data->ignored_path_len);
+  }
 
   return Qnil;
 }
 
-static void dd_cov_update_line_coverage(rb_event_flag_t event, VALUE data, VALUE self, ID id, VALUE klass)
+static void dd_cov_update_coverage(rb_event_flag_t event, VALUE data, VALUE self, ID id, VALUE klass)
 {
   struct dd_cov_data *dd_cov_data;
   TypedData_Get_Struct(data, struct dd_cov_data, &dd_cov_data_type, dd_cov_data);
 
-  const char *filename = rb_sourcefile();
-  if (filename == NULL)
+  const char *c_filename = rb_sourcefile();
+
+  // skip if we cover the same file again
+  uintptr_t current_filename_ptr = (uintptr_t)c_filename;
+  if (dd_cov_data->last_filename_ptr == current_filename_ptr)
   {
     return;
   }
+  dd_cov_data->last_filename_ptr = current_filename_ptr;
+
+  VALUE top_frame;
+  int captured_frames = rb_profile_frames(
+      0 /* stack starting depth */,
+      PROFILE_FRAMES_BUFFER_SIZE,
+      &top_frame,
+      NULL);
 
-  // if given filename is not located under the root, we skip it
-  if (is_prefix(dd_cov_data->root, filename) == 0)
+  if (captured_frames != PROFILE_FRAMES_BUFFER_SIZE)
   {
     return;
   }
 
-  // if ignored_path is provided and given filename is located under the ignored_path, we skip it too
-  // this is useful for ignoring bundled gems location
-  if (RTEST(dd_cov_data->ignored_path) && is_prefix(dd_cov_data->ignored_path, filename) == 1)
+  VALUE filename = rb_profile_frame_path(top_frame);
+  if (filename == Qnil)
   {
     return;
   }
 
-  VALUE rb_str_source_file = rb_str_new2(filename);
-
-  if (dd_cov_data->mode == DD_COV_TARGET_FILES)
+  char *filename_ptr = RSTRING_PTR(filename);
+  // if the current filename is not located under the root, we skip it
+  if (strncmp(dd_cov_data->root, filename_ptr, dd_cov_data->root_len) != 0)
   {
-    rb_hash_aset(dd_cov_data->coverage, rb_str_source_file, Qtrue);
     return;
   }
 
-  // this isn't optimized yet, this is a POC to show that lines coverage is possible
-  // ITR beta is going to use files coverage, we'll get back to this part when
-  // we need to implement lines coverage
-  if (dd_cov_data->mode == DD_COV_TARGET_LINES)
+  // if ignored_path is provided and the current filename is located under the ignored_path, we skip it too
+  // this is useful for ignoring bundled gems location
+  if (dd_cov_data->ignored_path_len != 0 && strncmp(dd_cov_data->ignored_path, filename_ptr, dd_cov_data->ignored_path_len) == 0)
   {
-    int line_number = rb_sourceline();
-    if (line_number <= 0)
-    {
-      return;
-    }
-
-    VALUE rb_lines = rb_hash_aref(dd_cov_data->coverage, rb_str_source_file);
-    if (rb_lines == Qnil)
-    {
-      rb_lines = rb_hash_new();
-      rb_hash_aset(dd_cov_data->coverage, rb_str_source_file, rb_lines);
-    }
-
-    rb_hash_aset(rb_lines, INT2FIX(line_number), Qtrue);
+    return;
   }
+
+  rb_hash_aset(dd_cov_data->coverage, filename, Qtrue);
 }
 
 static VALUE dd_cov_start(VALUE self)
 {
   // get current thread
   VALUE thval = rb_thread_current();
 
+  struct dd_cov_data *dd_cov_data;
+  TypedData_Get_Struct(self, struct dd_cov_data, &dd_cov_data_type, dd_cov_data);
+
+  if (dd_cov_data->root_len == 0)
+  {
+    rb_raise(rb_eRuntimeError, "root is required");
+  }
+
   // add event hook
-  rb_thread_add_event_hook(thval, dd_cov_update_line_coverage, RUBY_EVENT_LINE, self);
+  rb_thread_add_event_hook(thval, dd_cov_update_coverage, RUBY_EVENT_LINE, self);
 
   return self;
 }
@@ -190,16 +174,17 @@ static VALUE dd_cov_stop(VALUE self)
   // get current thread
   VALUE thval = rb_thread_current();
   // remove event hook for the current thread
-  rb_thread_remove_event_hook(thval, dd_cov_update_line_coverage);
+  rb_thread_remove_event_hook(thval, dd_cov_update_coverage);
 
   struct dd_cov_data *dd_cov_data;
   TypedData_Get_Struct(self, struct dd_cov_data, &dd_cov_data_type, dd_cov_data);
 
-  VALUE cov = dd_cov_data->coverage;
+  VALUE res = dd_cov_data->coverage;
 
   dd_cov_data->coverage = rb_hash_new();
+  dd_cov_data->last_filename_ptr = 0;
 
-  return cov;
+  return res;
 }
 
 void Init_datadog_cov(void)

diff --git a/lib/datadog/ci/itr/coverage/writer.rb b/lib/datadog/ci/itr/coverage/writer.rb
@@ -22,6 +22,8 @@ class Writer
           DEFAULT_BUFFER_MAX_SIZE = 10_000
           DEFAULT_SHUTDOWN_TIMEOUT = 60
 
+          DEFAULT_INTERVAL = 3
+
           def initialize(transport:, options: {})
             @transport = transport
 
@@ -32,7 +34,7 @@ def initialize(transport:, options: {})
             self.fork_policy = Core::Workers::Async::Thread::FORK_POLICY_RESTART
 
             # Workers::IntervalLoop settings
-            self.loop_base_interval = options[:interval] if options.key?(:interval)
+            self.loop_base_interval = options[:interval] || DEFAULT_INTERVAL
             self.loop_back_off_ratio = options[:back_off_ratio] if options.key?(:back_off_ratio)
             self.loop_back_off_max = options[:back_off_max] if options.key?(:back_off_max)
 

diff --git a/sig/datadog/ci/itr/coverage/ddcov.rbs b/sig/datadog/ci/itr/coverage/ddcov.rbs
@@ -4,7 +4,7 @@ module Datadog
       module Coverage
         class DDCov
 
-          def initialize: (root: String, ?mode: Symbol, ignored_path: String?) -> void
+          def initialize: (root: String, ignored_path: String?) -> void
 
           def start: () -> void
 

diff --git a/sig/datadog/ci/itr/coverage/writer.rbs b/sig/datadog/ci/itr/coverage/writer.rbs
@@ -22,6 +22,8 @@ module Datadog
 
           DEFAULT_SHUTDOWN_TIMEOUT: 60
 
+          DEFAULT_INTERVAL: 3
+
           def initialize: (transport: Datadog::CI::ITR::Coverage::Transport, ?options: ::Hash[untyped, untyped]) -> void
 
           def write: (Datadog::CI::ITR::Coverage::Event event) -> untyped