Check the DBM comment region in place, dropping the duplicate-comment substring

dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/instrumentation/dbm/SharedDBCommenter.java

@@ -7,6 +7,7 @@
 import datadog.trace.api.internal.VisibleForTesting;
 import datadog.trace.bootstrap.instrumentation.api.AgentSpan;
 import datadog.trace.bootstrap.instrumentation.api.Tags;
+import datadog.trace.util.SubSequence;
 import java.io.UnsupportedEncodingException;
 import java.net.URLEncoder;
 import java.nio.charset.StandardCharsets;
@@ -53,19 +54,29 @@ public class SharedDBCommenter {
   private static volatile boolean staticPrefixComputed = false;
   private static volatile String staticPrefix;
 
-  // Used by SQLCommenter and MongoCommentInjector to avoid duplicate comment injection.
-  // Note: the contains-chain could still be done "better" (a single scan), but the per-call
-  // "KEY + =" concatenation -- the allocating part -- is now hoisted to the *_EQ constants above.
+  // Used by SQLCommenter and MongoCommentInjector to avoid duplicate comment injection. Mongo
+  // passes the already-extracted comment body; SQLCommenter uses the range overload to check it
+  // in place. Both run the same nine "<key>=" needle checks.
   public static boolean containsTraceComment(String commentContent) {
-    return commentContent.contains(PARENT_SERVICE_EQ)
-        || commentContent.contains(DATABASE_SERVICE_EQ)
-        || commentContent.contains(DD_HOSTNAME_EQ)
-        || commentContent.contains(DD_DB_NAME_EQ)
-        || commentContent.contains(DD_PEER_SERVICE_EQ)
-        || commentContent.contains(DD_ENV_EQ)
-        || commentContent.contains(DD_VERSION_EQ)
-        || commentContent.contains(TRACEPARENT_EQ)
-        || commentContent.contains(DD_SERVICE_HASH_EQ);
+    return containsTraceComment(commentContent, 0, commentContent.length());
+  }
+
+  /**
+   * Range overload: true if {@code sql} contains a trace-comment needle fully within {@code [from,
+   * to)} -- checks the comment body in place, with no substring allocation of the region.
+   */
+  public static boolean containsTraceComment(String sql, int from, int to) {
+    // Zero-copy view of the comment body; reads like ordinary String.contains, no substring.
+    SubSequence comment = SubSequence.of(sql, from, to);
+    return comment.contains(PARENT_SERVICE_EQ)
+        || comment.contains(DATABASE_SERVICE_EQ)
+        || comment.contains(DD_HOSTNAME_EQ)
+        || comment.contains(DD_DB_NAME_EQ)
+        || comment.contains(DD_PEER_SERVICE_EQ)
+        || comment.contains(DD_ENV_EQ)
+        || comment.contains(DD_VERSION_EQ)
+        || comment.contains(TRACEPARENT_EQ)
+        || comment.contains(DD_SERVICE_HASH_EQ);
   }
 
   // Build database comment content without comment delimiters such as /* */

dd-java-agent/agent-bootstrap/src/test/java/datadog/trace/bootstrap/instrumentation/dbm/SharedDBCommenterContainsTraceCommentTest.java

@@ -0,0 +1,48 @@
+package datadog.trace.bootstrap.instrumentation.dbm;
+
+import static datadog.trace.bootstrap.instrumentation.dbm.SharedDBCommenter.containsTraceComment;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import org.junit.jupiter.api.Test;
+
+/**
+ * DB-level behavior of {@link SharedDBCommenter#containsTraceComment}: the nine "<key>=" needle
+ * set, the {@code String} delegate, and the range overload checking the comment body in place. (The
+ * {@code [from, to)} boundary semantics are unit-tested on {@code Strings.regionContains}.)
+ */
+class SharedDBCommenterContainsTraceCommentTest {
+
+  @Test
+  void delegate_wholeString() {
+    assertTrue(containsTraceComment("ddps='svc',dde='test'"));
+    assertFalse(containsTraceComment("just a plain comment"));
+    assertFalse(containsTraceComment(""));
+  }
+
+  @Test
+  void range_needleInsideCommentBody() {
+    String sql = "SELECT 1 /*ddps='svc',dde='test'*/";
+    int from = sql.indexOf("/*") + 2;
+    int to = sql.indexOf("*/");
+    assertTrue(containsTraceComment(sql, from, to));
+  }
+
+  @Test
+  void range_nonDdCommentBody() {
+    String sql = "SELECT 1 /* just a customer comment */";
+    int from = sql.indexOf("/*") + 2;
+    int to = sql.indexOf("*/");
+    assertFalse(containsTraceComment(sql, from, to));
+  }
+
+  @Test
+  void range_ddNeedleOutsideCommentRegionNotMatched() {
+    // The DD needle sits in the statement body, not the comment region we pass -- a whole-string
+    // contains would false-positive; the range check must scope to [from, to).
+    String sql = "ddps='x' /* clean */";
+    int from = sql.indexOf("/*") + 2;
+    int to = sql.indexOf("*/");
+    assertFalse(containsTraceComment(sql, from, to));
+  }
+}

dd-java-agent/instrumentation/jdbc/src/jmh/java/datadog/trace/instrumentation/jdbc/SQLCommenterDuplicateCommentBenchmark.java

@@ -0,0 +1,77 @@
+package datadog.trace.instrumentation.jdbc;
+
+import org.openjdk.jmh.annotations.Benchmark;
+import org.openjdk.jmh.annotations.Fork;
+import org.openjdk.jmh.annotations.Measurement;
+import org.openjdk.jmh.annotations.Scope;
+import org.openjdk.jmh.annotations.State;
+import org.openjdk.jmh.annotations.Threads;
+import org.openjdk.jmh.annotations.Warmup;
+
+/**
+ * Benchmark for the duplicate-comment guard in {@link SQLCommenter#inject} -- the {@code
+ * hasDDComment} path that avoids double-commenting an already-instrumented statement.
+ *
+ * <p><b>What we're measuring.</b> The guard used to materialize {@code sql.substring(commentStart,
+ * commentEnd)} (the comment body) just to scan it for trace-comment needles. (B) checks the comment
+ * region in place via {@code SharedDBCommenter.containsTraceComment(sql, from, to)} -- no
+ * substring.
+ *
+ * <p><b>Isolation.</b> The substring only happens when the SQL already carries a comment in the
+ * checked position; for a DD comment {@code inject} then returns early. Passing {@code dbType=null}
+ * skips the first-word scan (benchmarked separately for the {@code getFirstWord} change), so over
+ * already-DD-commented SQL the <i>only</i> allocation left in {@code inject} is the substring (B)
+ * removes. Run at {@code @Threads(8)} with {@code -prof gc}.
+ *
+ * <pre>
+ *   ./gradlew :dd-java-agent:instrumentation:jdbc:jmh   # add -prof gc
+ * </pre>
+ *
+ * <p><b>Results</b> (JDK 17, MacBook M-series, {@code @Threads(8)}, {@code @Fork(5)}, {@code -prof
+ * gc}):
+ *
+ * <pre>
+ *                        throughput            gc.alloc.rate.norm
+ *   before (substring)   23.5M ± 1.1M ops/s    140 B/op
+ *   after  (range/view)  26.2M ± 1.5M ops/s    ~0  B/op  (10^-5)
+ * </pre>
+ *
+ * The extractCommentContent substring (140 B/op) is gone -- the in-place range scan and the
+ * SubSequence view it flows through are both EA-elided. The allocation delta is exact and
+ * fork-stable; that's the win. At {@code @Fork(5)} the spread tightens and a small throughput
+ * uplift (~1.1x) resolves -- but this path is dominated by the nine indexOf scans (CPU the
+ * alloc-removal doesn't touch), so the headline win is the allocation, a small cut that compounds
+ * across comment-bearing injects, not a per-call throughput jump.
+ */
+@Fork(5)
+@Warmup(iterations = 2)
+@Measurement(iterations = 5)
+@Threads(8)
+public class SQLCommenterDuplicateCommentBenchmark {
+
+  // Already-DD-commented SQL (append style, comment at the end). First needle hits at different
+  // depths: ddps first (cheap), traceparent-only (scans 8 before the match).
+  static final String[] SQL = {
+    "SELECT * FROM foo /*ddps='svc',dde='test',dddbs='mydb',ddh='h',dddb='n',traceparent='00-00000000000000007fffffffffffffff-000000024cb016ea-00'*/",
+    "SELECT * FROM bar WHERE id = 42 /*traceparent='00-00000000000000007fffffffffffffff-000000024cb016ea-01'*/",
+  };
+
+  /** Per-thread cursor so threads don't contend on a shared index under {@code @Threads(8)}. */
+  @State(Scope.Thread)
+  public static class Cursor {
+    int index = 0;
+
+    String next() {
+      int i = index;
+      index = (i + 1) % SQL.length;
+      return SQL[i];
+    }
+  }
+
+  @Benchmark
+  public boolean alreadyCommented(Cursor cursor) {
+    // dbType=null skips the first-word scan; the DD comment makes inject return early after the
+    // duplicate-comment check -- the path (B) optimizes. Returns the input sql (no new String).
+    return SQLCommenter.inject(cursor.next(), "mydb", null, "h", "n", null, true) != null;
+  }
+}

dd-java-agent/instrumentation/jdbc/src/main/java/datadog/trace/instrumentation/jdbc/SQLCommenter.java

@@ -123,11 +123,6 @@ private static boolean hasDDComment(String sql, boolean appendComment) {
       return false;
     }
 
-    String commentContent = extractCommentContent(sql, appendComment);
-    return SharedDBCommenter.containsTraceComment(commentContent);
-  }
-
-  private static String extractCommentContent(String sql, boolean appendComment) {
     int startIdx;
     int endIdx;
     if (appendComment) {
@@ -138,9 +133,10 @@ private static String extractCommentContent(String sql, boolean appendComment) {
       endIdx = sql.indexOf(CLOSE_COMMENT);
     }
     if (startIdx != -1 && endIdx != -1 && endIdx > startIdx) {
-      return sql.substring(startIdx + OPEN_COMMENT_LEN, endIdx);
+      // Check the comment body in place -- no substring of the comment region.
+      return SharedDBCommenter.containsTraceComment(sql, startIdx + OPEN_COMMENT_LEN, endIdx);
     }
-    return "";
+    return false;
   }
 
   /**

internal-api/src/main/java/datadog/trace/util/Strings.java

@@ -340,4 +340,17 @@ public SubSequence next() {
       return subSeq;
     }
   }
+
+  /**
+   * True if {@code needle} occurs fully within {@code s[beginIndex, endIndex)} -- a range-limited,
+   * allocation-free alternative to {@code s.substring(beginIndex, endIndex).contains(needle)}.
+   *
+   * <p>{@code indexOf} returns the earliest occurrence at or after {@code beginIndex}; if that one
+   * overshoots {@code endIndex} there is no earlier full occurrence in range, so the bound check is
+   * exact.
+   */
+  public static boolean regionContains(String s, int beginIndex, int endIndex, String needle) {
+    int idx = s.indexOf(needle, beginIndex);
+    return idx >= 0 && idx + needle.length() <= endIndex;
+  }
 }

internal-api/src/main/java/datadog/trace/util/SubSequence.java

@@ -109,6 +109,14 @@ public final boolean equals(CharSequence that) {
     return true;
   }
 
+  /**
+   * True if this sub-sequence contains {@code needle} -- the zero-copy equivalent of {@code
+   * toString().contains(needle)}, with no substring materialized.
+   */
+  public final boolean contains(String needle) {
+    return Strings.regionContains(this.str, this.beginIndex, this.endIndex, needle);
+  }
+
   /** Case-insensitive content comparison; mirrors {@link String#equalsIgnoreCase(String)}. */
   public final boolean equalsIgnoreCase(CharSequence that) {
     int len = this.length();

internal-api/src/test/java/datadog/trace/util/StringsRegionContainsTest.java

@@ -0,0 +1,53 @@
+package datadog.trace.util;
+
+import static datadog.trace.util.Strings.regionContains;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import org.junit.jupiter.api.Test;
+
+/** Boundary semantics of {@link Strings#regionContains(String, int, int, String)}. */
+class StringsRegionContainsTest {
+
+  // "abXYZcd": a0 b1 X2 Y3 Z4 c5 d6  -> "XYZ" spans [2,5).
+  private static final String S = "abXYZcd";
+
+  @Test
+  void foundFullyInside() {
+    assertTrue(regionContains(S, 0, S.length(), "XYZ"));
+  }
+
+  @Test
+  void notPresent() {
+    assertFalse(regionContains(S, 0, S.length(), "QQ"));
+  }
+
+  @Test
+  void exactFit() {
+    // idx == 2, idx + len == 5 == endIndex -> included.
+    assertTrue(regionContains(S, 2, 5, "XYZ"));
+  }
+
+  @Test
+  void straddlingEndIndexExcluded() {
+    // endIndex == 4 cuts off the trailing 'Z' -> not fully inside.
+    assertFalse(regionContains(S, 2, 4, "XYZ"));
+  }
+
+  @Test
+  void occurrenceBeforeBeginIndexExcluded() {
+    // beginIndex == 3 starts past the needle's first char -> no occurrence at/after beginIndex.
+    assertFalse(regionContains(S, 3, S.length(), "XYZ"));
+  }
+
+  @Test
+  void emptyRegion() {
+    assertFalse(regionContains(S, 2, 2, "XYZ"));
+  }
+
+  @Test
+  void matchesWholeStringContains() {
+    assertTrue(regionContains("hello", 0, 5, "ll"));
+    assertFalse(regionContains("hello", 0, 5, "z"));
+  }
+}

internal-api/src/test/java/datadog/trace/util/SubSequenceTest.java

@@ -110,6 +110,20 @@ public void appendToBuilder() {
     assertEquals(expectedStr, builder1.toString());
   }
 
+  @Test
+  public void contains() {
+    // "/*ddps='svc',dde='x'*/ rest" -- the comment body "ddps='svc',dde='x'" spans [2, 20).
+    String s = "/*ddps='svc',dde='x'*/ rest";
+    SubSequence comment = SubSequence.of(s, 2, 20);
+    assertTrue(comment.contains("ddps="));
+    assertTrue(comment.contains("dde="));
+    assertFalse(comment.contains("ddh="));
+
+    // View-relative: a needle present in the backing string but outside this view is not found.
+    SubSequence dde = SubSequence.of(s, 13, 20); // "dde='x'"
+    assertFalse(dde.contains("ddps=")); // ddps= is before this view's range
+  }
+
   @Test
   public void equalsIgnoreCase() {
     SubSequence call = SubSequence.of("xx CALL yy", 3, 7); // "CALL"