From b460e850bd66aafc210624d717377c0372fe69a1 Mon Sep 17 00:00:00 2001
From: Robin Schroer <git@sulami.xyz>
Date: Thu, 1 Feb 2024 10:31:07 +0900
Subject: [PATCH] Improve path groups handling

The only functional difference here is that empty paths will be logged
as "/", otherwise this is just a refactor.
---
 lib/degica_datadog.rb         |  1 +
 lib/degica_datadog/tracing.rb |  9 +++------
 lib/degica_datadog/util.rb    | 28 ++++++++++++++++++++++++++++
 spec/util_spec.rb             | 30 ++++++++++++++++++++++++++++++
 4 files changed, 62 insertions(+), 6 deletions(-)
 create mode 100644 lib/degica_datadog/util.rb
 create mode 100644 spec/util_spec.rb

diff --git a/lib/degica_datadog.rb b/lib/degica_datadog.rb
index 9a34051..28e45f4 100644
--- a/lib/degica_datadog.rb
+++ b/lib/degica_datadog.rb
@@ -3,6 +3,7 @@
 require_relative "degica_datadog/config"
 require_relative "degica_datadog/statsd"
 require_relative "degica_datadog/tracing"
+require_relative "degica_datadog/util"
 require_relative "degica_datadog/version"
 
 # Standardised interactions with Datadog.
diff --git a/lib/degica_datadog/tracing.rb b/lib/degica_datadog/tracing.rb
index f302c53..06d9fc4 100644
--- a/lib/degica_datadog/tracing.rb
+++ b/lib/degica_datadog/tracing.rb
@@ -4,10 +4,10 @@
 
 module DegicaDatadog
   # Tracing related functionality.
-  module Tracing # rubocop:disable Metrics/ModuleLength
+  module Tracing
     class << self
       # Initialize Datadog tracing. Call this in from config/application.rb.
-      def init(rake_tasks: []) # rubocop:disable Metrics/AbcSize,Metrics/MethodLength,Metrics/CyclomaticComplexity,Metrics/PerceivedComplexity
+      def init(rake_tasks: []) # rubocop:disable Metrics/AbcSize,Metrics/MethodLength
         return unless Config.enabled?
 
         require "ddtrace/auto_instrument"
@@ -69,10 +69,7 @@ def init(rake_tasks: []) # rubocop:disable Metrics/AbcSize,Metrics/MethodLength,
               # ID. The logic seems to be at least vaguely to replace any path
               # segment that contains a digit with a ?, so we're reproducing
               # that here.
-              path_group = span.get_tag("http.url")
-                .split("/")
-                .map { |segment| segment =~ /\d/ ? "?" : segment }
-                .join("/")
+              path_group = DegicaDatadog::Util.path_group(span.get_tag("http.url"))
               span.resource = "#{span.get_tag("http.method")} #{path_group}"
             end
           end
diff --git a/lib/degica_datadog/util.rb b/lib/degica_datadog/util.rb
new file mode 100644
index 0000000..da21a10
--- /dev/null
+++ b/lib/degica_datadog/util.rb
@@ -0,0 +1,28 @@
+# frozen_string_literal: true
+
+module DegicaDatadog
+  # Utility methods.
+  module Util
+    class << self
+      # Return a path group for a given path.
+      def path_group(path)
+        return unless path.is_a?(String)
+        return "/" if path.empty?
+
+        path
+          .split("/")
+          .map(&method(:process_path_segment))
+          .join("/")
+      end
+
+      private
+
+      def process_path_segment(segment)
+        return segment if segment =~ /^v\d+$/
+        return "?" if segment =~ /\d/
+
+        segment
+      end
+    end
+  end
+end
diff --git a/spec/util_spec.rb b/spec/util_spec.rb
new file mode 100644
index 0000000..e17cb6d
--- /dev/null
+++ b/spec/util_spec.rb
@@ -0,0 +1,30 @@
+# frozen_string_literal: true
+
+RSpec.describe DegicaDatadog::Util do
+  describe ".path_group" do
+    it "returns nil for non-strings" do
+      expect(described_class.path_group(nil)).to be_nil
+      expect(described_class.path_group(42)).to be_nil
+      expect(described_class.path_group([])).to be_nil
+    end
+
+    it "returns '/' for empty strings" do
+      expect(described_class.path_group("")).to eq("/")
+    end
+
+    it "returns the path for strings without numbers" do
+      expect(described_class.path_group("/foo/bar")).to eq("/foo/bar")
+    end
+
+    it "returns the path with segments containing digits replaced with ?" do
+      expect(described_class.path_group("/foo/42")).to eq("/foo/?")
+      expect(described_class.path_group("/foo/42/bar")).to eq("/foo/?/bar")
+      expect(described_class.path_group("/foo/v42uuid/bar")).to eq("/foo/?/bar")
+    end
+
+    it "keeps segments that look like API versioning" do
+      expect(described_class.path_group("/api/v1/foo")).to eq("/api/v1/foo")
+      expect(described_class.path_group("/api/v1/foo/42")).to eq("/api/v1/foo/?")
+    end
+  end
+end