delay queue time rate limiting until event is committed (#7867)

* delay queue time rate limiting until event is committed

* use correct native image check

Author: richardstartin

dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/instrumentation/java/concurrent/QueueTimerHelper.java

@@ -4,6 +4,7 @@
 import datadog.trace.api.config.ProfilingConfig;
 import datadog.trace.api.profiling.QueueTiming;
 import datadog.trace.api.profiling.Timer;
+import datadog.trace.api.profiling.Timing;
 import datadog.trace.api.sampling.PerRecordingRateLimiter;
 import datadog.trace.bootstrap.ContextStore;
 import datadog.trace.bootstrap.config.provider.ConfigProvider;
@@ -35,21 +36,28 @@ public static <T> void startQueuingTimer(
   }
 
   public static void startQueuingTimer(State state, Class<?> schedulerClass, Object task) {
-    if (Platform.isNativeImageBuilder()) {
+    if (Platform.isNativeImage()) {
       // explicitly not supported for Graal native image
       return;
     }
     // avoid calling this before JFR is initialised because it will lead to reading the wrong
     // TSC frequency before JFR has set it up properly
-    if (task != null
-        && state != null
-        && InstrumentationBasedProfiling.isJFRReady()
-        && RateLimiterHolder.RATE_LIMITER.permit()) {
+    if (task != null && state != null && InstrumentationBasedProfiling.isJFRReady()) {
       QueueTiming timing =
           (QueueTiming) AgentTracer.get().getProfilingContext().start(Timer.TimerType.QUEUEING);
       timing.setTask(task);
       timing.setScheduler(schedulerClass);
       state.setTiming(timing);
     }
   }
+
+  public static void stopQueuingTimer(Timing timing) {
+    if (Platform.isNativeImage()) {
+      // explicitly not supported for Graal native image
+      return;
+    }
+    if (timing != null && timing.sample() && RateLimiterHolder.RATE_LIMITER.permit()) {
+      timing.report();
+    }
+  }
 }

dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/instrumentation/java/concurrent/State.java

@@ -85,7 +85,7 @@ public boolean isTimed() {
   public void stopTiming() {
     Timing timing = TIMING.getAndSet(this, null);
     if (timing != null) {
-      timing.close();
+      QueueTimerHelper.stopQueuingTimer(timing);
     }
   }
 }

dd-java-agent/agent-profiling/profiling-controller-openjdk/src/main/java/com/datadog/profiling/controller/openjdk/events/QueueTimeEvent.java

@@ -56,10 +56,12 @@ public void setScheduler(Class<?> scheduler) {
   }
 
   @Override
-  public void close() {
-    end();
-    if (shouldCommit()) {
-      commit();
-    }
+  public void report() {
+    commit();
+  }
+
+  @Override
+  public boolean sample() {
+    return shouldCommit();
   }
 }

dd-java-agent/agent-profiling/profiling-ddprof/src/main/java/com/datadog/profiling/ddprof/DatadogProfiler.java

@@ -402,17 +402,18 @@ public QueueTimeTracker newQueueTimeTracker() {
     return new QueueTimeTracker(this, profiler.getCurrentTicks());
   }
 
-  void recordQueueTimeEvent(
-      long startMillis, long startTicks, Object task, Class<?> scheduler, Thread origin) {
+  boolean shouldRecordQueueTimeEvent(long startMillis) {
+    return System.currentTimeMillis() - startMillis >= queueTimeThresholdMillis;
+  }
+
+  void recordQueueTimeEvent(long startTicks, Object task, Class<?> scheduler, Thread origin) {
     if (profiler != null) {
-      if (System.currentTimeMillis() - startMillis >= queueTimeThresholdMillis) {
-        // note: because this type traversal can update secondary_super_cache (see JDK-8180450)
-        // we avoid doing this unless we are absolutely certain we will record the event
-        Class<?> taskType = TaskWrapper.getUnwrappedType(task);
-        if (taskType != null) {
-          long endTicks = profiler.getCurrentTicks();
-          profiler.recordQueueTime(startTicks, endTicks, taskType, scheduler, origin);
-        }
+      // note: because this type traversal can update secondary_super_cache (see JDK-8180450)
+      // we avoid doing this unless we are absolutely certain we will record the event
+      Class<?> taskType = TaskWrapper.getUnwrappedType(task);
+      if (taskType != null) {
+        long endTicks = profiler.getCurrentTicks();
+        profiler.recordQueueTime(startTicks, endTicks, taskType, scheduler, origin);
       }
     }
   }

dd-java-agent/agent-profiling/profiling-ddprof/src/main/java/com/datadog/profiling/ddprof/QueueTimeTracker.java

@@ -32,12 +32,17 @@ public void setScheduler(Class<?> scheduler) {
   }
 
   @Override
-  public void close() {
+  public void report() {
     assert weakTask != null && scheduler != null;
     Object task = this.weakTask.get();
     if (task != null) {
       // indirection reduces shallow size of the tracker instance
-      profiler.recordQueueTimeEvent(startMillis, startTicks, task, scheduler, origin);
+      profiler.recordQueueTimeEvent(startTicks, task, scheduler, origin);
     }
   }
+
+  @Override
+  public boolean sample() {
+    return profiler.shouldRecordQueueTimeEvent(startMillis);
+  }
 }

dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/TestProfilingContextIntegration.groovy

@@ -99,7 +99,7 @@ class TestProfilingContextIntegration implements ProfilingContextIntegration {
     }
 
     @Override
-    void close() {
+    void report() {
       counter.decrementAndGet()
       AgentSpan span = AgentTracer.activeSpan()
       long activeSpanId = span == null ? 0 : span.getSpanId()
@@ -109,6 +109,10 @@ class TestProfilingContextIntegration implements ProfilingContextIntegration {
       closedTimings.offer(this)
     }
 
+    @Override
+    boolean sample() {
+      return true
+    }
 
     @Override
     String toString() {

internal-api/src/main/java/datadog/trace/api/profiling/Timing.java

@@ -1,14 +1,20 @@
 package datadog.trace.api.profiling;
 
-public interface Timing extends AutoCloseable {
-  @Override
-  void close();
+public interface Timing {
+  void report();
+
+  boolean sample();
 
   class NoOp implements Timing, QueueTiming {
     public static final Timing INSTANCE = new NoOp();
 
     @Override
-    public void close() {}
+    public void report() {}
+
+    @Override
+    public boolean sample() {
+      return false;
+    }
 
     @Override
     public void setTask(Object task) {}