Fix appsec.rasp.error and appsec.waf.error telemetry metrics

Remove counters from AppsecRequestContext as they are not used in the metrics
Fix appsec.waf.error metric tags as they didn't match the RFC
Fix that appsec.waf.error was created in the same loop using the rasp counter instead of using the waf counter
Increment metrics if UnclassifiedPowerwafException is thrown
Remove the hardcoded waf error codes (provisional enum is used until upgrade libddwaf lib)
Replace ConcurrentHashMap with AtomicLongArray for raspErrorCodeCounter to improve performance and memory efficiency

Author: jandro996

Diff for: dd-java-agent/appsec/src/main/java/com/datadog/appsec/gateway/AppSecRequestContext.java

@@ -74,9 +74,6 @@ public class AppSecRequestContext implements DataBundle, Closeable {
   public static final Set<String> RESPONSE_HEADERS_ALLOW_LIST =
       new TreeSet<>(
           Arrays.asList("content-length", "content-type", "content-encoding", "content-language"));
-  public static final int DD_WAF_RUN_INTERNAL_ERROR = -3;
-  public static final int DD_WAF_RUN_INVALID_OBJECT_ERROR = -2;
-  public static final int DD_WAF_RUN_INVALID_ARGUMENT_ERROR = -1;
 
   static {
     REQUEST_HEADERS_ALLOW_LIST.addAll(DEFAULT_REQUEST_HEADERS_ALLOW_LIST);
@@ -125,12 +122,6 @@ public class AppSecRequestContext implements DataBundle, Closeable {
   private volatile boolean blocked;
   private volatile int wafTimeouts;
   private volatile int raspTimeouts;
-  private volatile int raspInternalErrors;
-  private volatile int raspInvalidObjectErrors;
-  private volatile int raspInvalidArgumentErrors;
-  private volatile int wafInternalErrors;
-  private volatile int wafInvalidObjectErrors;
-  private volatile int wafInvalidArgumentErrors;
 
   // keep a reference to the last published usr.id
   private volatile String userId;
@@ -150,29 +141,6 @@ public class AppSecRequestContext implements DataBundle, Closeable {
   private static final AtomicIntegerFieldUpdater<AppSecRequestContext> RASP_TIMEOUTS_UPDATER =
       AtomicIntegerFieldUpdater.newUpdater(AppSecRequestContext.class, "raspTimeouts");
 
-  private static final AtomicIntegerFieldUpdater<AppSecRequestContext>
-      RASP_INTERNAL_ERRORS_UPDATER =
-          AtomicIntegerFieldUpdater.newUpdater(AppSecRequestContext.class, "raspInternalErrors");
-  private static final AtomicIntegerFieldUpdater<AppSecRequestContext>
-      RASP_INVALID_OBJECT_ERRORS_UPDATER =
-          AtomicIntegerFieldUpdater.newUpdater(
-              AppSecRequestContext.class, "raspInvalidObjectErrors");
-  private static final AtomicIntegerFieldUpdater<AppSecRequestContext>
-      RASP_INVALID_ARGUMENT_ERRORS_UPDATER =
-          AtomicIntegerFieldUpdater.newUpdater(
-              AppSecRequestContext.class, "raspInvalidArgumentErrors");
-
-  private static final AtomicIntegerFieldUpdater<AppSecRequestContext> WAF_INTERNAL_ERRORS_UPDATER =
-      AtomicIntegerFieldUpdater.newUpdater(AppSecRequestContext.class, "wafInternalErrors");
-  private static final AtomicIntegerFieldUpdater<AppSecRequestContext>
-      WAF_INVALID_OBJECT_ERRORS_UPDATER =
-          AtomicIntegerFieldUpdater.newUpdater(
-              AppSecRequestContext.class, "wafInvalidObjectErrors");
-  private static final AtomicIntegerFieldUpdater<AppSecRequestContext>
-      WAF_INVALID_ARGUMENT_ERRORS_UPDATER =
-          AtomicIntegerFieldUpdater.newUpdater(
-              AppSecRequestContext.class, "wafInvalidArgumentErrors");
-
   // to be called by the Event Dispatcher
   public void addAll(DataBundle newData) {
     for (Map.Entry<Address<?>, Object> entry : newData) {
@@ -222,38 +190,6 @@ public void increaseRaspTimeouts() {
     RASP_TIMEOUTS_UPDATER.incrementAndGet(this);
   }
 
-  public void increaseRaspErrorCode(int code) {
-    switch (code) {
-      case DD_WAF_RUN_INTERNAL_ERROR:
-        RASP_INTERNAL_ERRORS_UPDATER.incrementAndGet(this);
-        break;
-      case DD_WAF_RUN_INVALID_OBJECT_ERROR:
-        RASP_INVALID_OBJECT_ERRORS_UPDATER.incrementAndGet(this);
-        break;
-      case DD_WAF_RUN_INVALID_ARGUMENT_ERROR:
-        RASP_INVALID_ARGUMENT_ERRORS_UPDATER.incrementAndGet(this);
-        break;
-      default:
-        break;
-    }
-  }
-
-  public void increaseWafErrorCode(int code) {
-    switch (code) {
-      case DD_WAF_RUN_INTERNAL_ERROR:
-        WAF_INTERNAL_ERRORS_UPDATER.incrementAndGet(this);
-        break;
-      case DD_WAF_RUN_INVALID_OBJECT_ERROR:
-        WAF_INVALID_OBJECT_ERRORS_UPDATER.incrementAndGet(this);
-        break;
-      case DD_WAF_RUN_INVALID_ARGUMENT_ERROR:
-        WAF_INVALID_ARGUMENT_ERRORS_UPDATER.incrementAndGet(this);
-        break;
-      default:
-        break;
-    }
-  }
-
   public int getWafTimeouts() {
     return wafTimeouts;
   }
@@ -262,32 +198,6 @@ public int getRaspTimeouts() {
     return raspTimeouts;
   }
 
-  public int getRaspError(int code) {
-    switch (code) {
-      case DD_WAF_RUN_INTERNAL_ERROR:
-        return raspInternalErrors;
-      case DD_WAF_RUN_INVALID_OBJECT_ERROR:
-        return raspInvalidObjectErrors;
-      case DD_WAF_RUN_INVALID_ARGUMENT_ERROR:
-        return raspInvalidArgumentErrors;
-      default:
-        return 0;
-    }
-  }
-
-  public int getWafError(int code) {
-    switch (code) {
-      case DD_WAF_RUN_INTERNAL_ERROR:
-        return wafInternalErrors;
-      case DD_WAF_RUN_INVALID_OBJECT_ERROR:
-        return wafInvalidObjectErrors;
-      case DD_WAF_RUN_INVALID_ARGUMENT_ERROR:
-        return wafInvalidArgumentErrors;
-      default:
-        return 0;
-    }
-  }
-
   public Additive getOrCreateAdditive(PowerwafContext ctx, boolean createMetrics, boolean isRasp) {
 
     if (createMetrics) {

Diff for: dd-java-agent/appsec/src/main/java/com/datadog/appsec/powerwaf/PowerWAFModule.java

@@ -450,15 +450,11 @@ public void onDataAvailable(
           log.error("Error calling WAF", e);
         }
         WafMetricCollector.get().wafRequestError();
+        // TODO: replace -127 for e.code when UnclassifiedPowerwafException code is fixed
+        incrementErrorCodeMetric(gwCtx, -127);
         return;
       } catch (AbstractPowerwafException e) {
-        if (gwCtx.isRasp) {
-          reqCtx.increaseRaspErrorCode(e.code);
-          WafMetricCollector.get().raspErrorCode(gwCtx.raspRuleType, e.code);
-        } else {
-          reqCtx.increaseWafErrorCode(e.code);
-          WafMetricCollector.get().wafErrorCode(gwCtx.raspRuleType, e.code);
-        }
+        incrementErrorCodeMetric(gwCtx, e.code);
         return;
       } finally {
         if (log.isDebugEnabled()) {
@@ -654,6 +650,14 @@ private Powerwaf.ResultWithData runPowerwafAdditive(
     }
   }
 
+  private static void incrementErrorCodeMetric(GatewayContext gwCtx, int code) {
+    if (gwCtx.isRasp) {
+      WafMetricCollector.get().raspErrorCode(gwCtx.raspRuleType, code);
+    } else {
+      WafMetricCollector.get().wafErrorCode(code);
+    }
+  }
+
   private Powerwaf.ResultWithData runPowerwafTransient(
       Additive additive, PowerwafMetrics metrics, DataBundle bundle, CtxAndAddresses ctxAndAddr)
       throws AbstractPowerwafException {

Diff for: dd-java-agent/appsec/src/test/groovy/com/datadog/appsec/gateway/AppSecRequestContextSpecification.groovy

@@ -290,32 +290,6 @@ class AppSecRequestContextSpecification extends DDSpecification {
     ctx.getRaspTimeouts() == 2
   }
 
-  def "test increase and get RaspErrors"() {
-    when:
-    ctx.increaseRaspErrorCode(AppSecRequestContext.DD_WAF_RUN_INTERNAL_ERROR)
-    ctx.increaseRaspErrorCode(AppSecRequestContext.DD_WAF_RUN_INTERNAL_ERROR)
-    ctx.increaseRaspErrorCode(AppSecRequestContext.DD_WAF_RUN_INVALID_OBJECT_ERROR)
-
-    then:
-    ctx.getRaspError(AppSecRequestContext.DD_WAF_RUN_INTERNAL_ERROR) == 2
-    ctx.getRaspError(AppSecRequestContext.DD_WAF_RUN_INVALID_OBJECT_ERROR) == 1
-    ctx.getRaspError(AppSecRequestContext.DD_WAF_RUN_INVALID_ARGUMENT_ERROR) == 0
-    ctx.getRaspError(0) == 0
-  }
-
-  def "test increase and get WafErrors"() {
-    when:
-    ctx.increaseWafErrorCode(AppSecRequestContext.DD_WAF_RUN_INTERNAL_ERROR)
-    ctx.increaseWafErrorCode(AppSecRequestContext.DD_WAF_RUN_INTERNAL_ERROR)
-    ctx.increaseWafErrorCode(AppSecRequestContext.DD_WAF_RUN_INVALID_OBJECT_ERROR)
-
-    then:
-    ctx.getWafError(AppSecRequestContext.DD_WAF_RUN_INTERNAL_ERROR) == 2
-    ctx.getWafError(AppSecRequestContext.DD_WAF_RUN_INVALID_OBJECT_ERROR) == 1
-    ctx.getWafError(AppSecRequestContext.DD_WAF_RUN_INVALID_ARGUMENT_ERROR) == 0
-    ctx.getWafError(0) == 0
-  }
-
   void 'close logs if request end was not called'() {
     given:
     TestLogCollector.enable()

Diff for: internal-api/src/main/java/datadog/trace/api/telemetry/WafMetricCollector.java

@@ -2,21 +2,19 @@
 
 import java.util.Collection;
 import java.util.Collections;
+import java.util.HashMap;
 import java.util.LinkedList;
 import java.util.List;
+import java.util.Map;
 import java.util.concurrent.ArrayBlockingQueue;
 import java.util.concurrent.BlockingQueue;
-import java.util.concurrent.ConcurrentMap;
-import java.util.concurrent.ConcurrentSkipListMap;
 import java.util.concurrent.atomic.AtomicInteger;
 import java.util.concurrent.atomic.AtomicLong;
 import java.util.concurrent.atomic.AtomicLongArray;
 
 public class WafMetricCollector implements MetricCollector<WafMetricCollector.WafMetric> {
 
   public static WafMetricCollector INSTANCE = new WafMetricCollector();
-  private static final int ABSTRACT_POWERWAF_EXCEPTION_NUMBER =
-      3; // only 3 error codes are possible for now in AbstractPowerwafException
 
   public static WafMetricCollector get() {
     return WafMetricCollector.INSTANCE;
@@ -53,18 +51,10 @@ private WafMetricCollector() {
       new AtomicLongArray(RuleType.getNumValues());
   private static final AtomicLongArray raspTimeoutCounter =
       new AtomicLongArray(RuleType.getNumValues());
-  private static final ConcurrentMap<Integer, AtomicLongArray> raspErrorCodeCounter =
-      new ConcurrentSkipListMap<>();
-  private static final ConcurrentMap<Integer, AtomicLongArray> wafErrorCodeCounter =
-      new ConcurrentSkipListMap<>();
-
-  static {
-    for (int i = -1 * ABSTRACT_POWERWAF_EXCEPTION_NUMBER; i < 0; i++) {
-      raspErrorCodeCounter.put(i, new AtomicLongArray(RuleType.getNumValues()));
-      wafErrorCodeCounter.put(i, new AtomicLongArray(RuleType.getNumValues()));
-    }
-  }
-
+  private static final AtomicLongArray raspErrorCodeCounter =
+      new AtomicLongArray(WafErrorCode.values().length * RuleType.getNumValues());
+  private static final AtomicLongArray wafErrorCodeCounter =
+      new AtomicLongArray(WafErrorCode.values().length);
   private static final AtomicLongArray missingUserLoginQueue =
       new AtomicLongArray(LoginFramework.getNumValues() * LoginEvent.getNumValues());
   private static final AtomicLongArray missingUserIdQueue =
@@ -151,12 +141,23 @@ public void raspTimeout(final RuleType ruleType) {
     raspTimeoutCounter.incrementAndGet(ruleType.ordinal());
   }
 
-  public void raspErrorCode(final RuleType ruleType, final int ddwafRunErrorCode) {
-    raspErrorCodeCounter.get(ddwafRunErrorCode).incrementAndGet(ruleType.ordinal());
+  public void raspErrorCode(RuleType ruleType, final int errorCode) {
+    WafErrorCode wafErrorCode = WafErrorCode.fromCode(errorCode);
+    // Unsupported waf error code
+    if (wafErrorCode == null) {
+      return;
+    }
+    int index = wafErrorCode.ordinal() * RuleType.getNumValues() + ruleType.ordinal();
+    raspErrorCodeCounter.incrementAndGet(index);
   }
 
-  public void wafErrorCode(final RuleType ruleType, final int ddwafRunErrorCode) {
-    wafErrorCodeCounter.get(ddwafRunErrorCode).incrementAndGet(ruleType.ordinal());
+  public void wafErrorCode(final int errorCode) {
+    WafErrorCode wafErrorCode = WafErrorCode.fromCode(errorCode);
+    // Unsupported waf error code
+    if (wafErrorCode == null) {
+      return;
+    }
+    wafErrorCodeCounter.incrementAndGet(wafErrorCode.ordinal());
   }
 
   public void missingUserLogin(final LoginFramework framework, final LoginEvent eventType) {
@@ -321,16 +322,13 @@ public void prepareMetrics() {
     }
 
     // RASP rule type for each possible error code
-    for (int i = -1 * ABSTRACT_POWERWAF_EXCEPTION_NUMBER; i < 0; i++) {
+    for (WafErrorCode errorCode : WafErrorCode.values()) {
       for (RuleType ruleType : RuleType.values()) {
-        long counter = raspErrorCodeCounter.get(i).getAndSet(ruleType.ordinal(), 0);
-        if (counter > 0) {
-          if (!rawMetricsQueue.offer(
-              new RaspError(counter, ruleType, WafMetricCollector.wafVersion, i))) {
-            return;
-          }
+        int index = errorCode.ordinal() * RuleType.getNumValues() + ruleType.ordinal();
+        long count = raspErrorCodeCounter.getAndSet(index, 0);
+        if (count > 0) {
           if (!rawMetricsQueue.offer(
-              new WafError(counter, ruleType, WafMetricCollector.wafVersion, i))) {
+              new RaspError(count, ruleType, WafMetricCollector.wafVersion, errorCode.getCode()))) {
             return;
           }
         }
@@ -375,6 +373,17 @@ public void prepareMetrics() {
       }
     }
 
+    // WAF rule type for each possible error code
+    for (WafErrorCode errorCode : WafErrorCode.values()) {
+      long count = wafErrorCodeCounter.getAndSet(errorCode.ordinal(), 0);
+      if (count > 0) {
+        if (!rawMetricsQueue.offer(
+            new WafError(count, WafMetricCollector.wafVersion, errorCode.getCode()))) {
+          return;
+        }
+      }
+    }
+
     // RASP rule skipped per rule type for after-request reason
     for (RuleType ruleType : RuleType.values()) {
       long counter = raspRuleSkippedCounter.getAndSet(ruleType.ordinal(), 0);
@@ -569,27 +578,13 @@ public RaspError(
   }
 
   public static class WafError extends WafMetric {
-    public WafError(
-        final long counter,
-        final RuleType ruleType,
-        final String wafVersion,
-        final Integer ddwafRunError) {
+    public WafError(final long counter, final String wafVersion, final Integer ddwafRunError) {
       super(
           "waf.error",
           counter,
-          ruleType.variant != null
-              ? new String[] {
-                "rule_type:" + ruleType.type,
-                "rule_variant:" + ruleType.variant,
-                "waf_version:" + wafVersion,
-                "event_rules_version:" + rulesVersion,
-                "waf_error:" + ddwafRunError
-              }
-              : new String[] {
-                "rule_type:" + ruleType.type,
-                "waf_version:" + wafVersion,
-                "waf_error:" + ddwafRunError
-              });
+          "waf_version:" + wafVersion,
+          "event_rules_version:" + rulesVersion,
+          "waf_error:" + ddwafRunError);
     }
   }
 
@@ -614,4 +609,38 @@ public final void increment() {
       atomicLong.incrementAndGet();
     }
   }
+
+  // TODO This enum should be in liddwaf, remove it when available
+  public enum WafErrorCode {
+    INVALID_ARGUMENT(-1),
+    INVALID_OBJECT(-2),
+    INTERNAL_ERROR(-3),
+    BINDING_ERROR(
+        -127); // This is a special error code that is not returned by the WAF, is used to signal a
+    // binding error
+
+    private final int code;
+
+    private static final Map<Integer, WafErrorCode> CODE_MAP;
+
+    static {
+      Map<Integer, WafErrorCode> map = new HashMap<>();
+      for (WafErrorCode errorCode : values()) {
+        map.put(errorCode.code, errorCode);
+      }
+      CODE_MAP = Collections.unmodifiableMap(map);
+    }
+
+    WafErrorCode(int code) {
+      this.code = code;
+    }
+
+    public int getCode() {
+      return code;
+    }
+
+    public static WafErrorCode fromCode(int code) {
+      return CODE_MAP.get(code);
+    }
+  }
 }