Finalize implementation

Author: amarziali

dd-trace-core/src/main/java/datadog/trace/common/metrics/MetricKey.java

@@ -5,12 +5,12 @@
 import datadog.trace.bootstrap.instrumentation.api.UTF8BytesString;
 import java.util.Collections;
 import java.util.List;
+import java.util.Objects;
 
 /** The aggregation key for tracked metrics. */
 public final class MetricKey {
   private final UTF8BytesString resource;
   private final UTF8BytesString service;
-  // note: it won't participate to equals/hashcode since not part of aggregation
   private final UTF8BytesString serviceSource;
   private final UTF8BytesString operationName;
   private final UTF8BytesString type;
@@ -58,18 +58,34 @@ public MetricKey(
 
     // Only include httpMethod and httpEndpoint in hash if they are not null
     // This ensures backward compatibility when the feature is disabled
-    this.hash =
-        -196_513_505 * Boolean.hashCode(this.isTraceRoot)
-            + -1_807_454_463 * this.spanKind.hashCode()
-            + 887_503_681 * this.peerTags.hashCode()
-            + (this.httpMethod != null ? 28_629_151 * this.httpMethod.hashCode() : 0)
-            + (this.httpEndpoint != null ? 923_521 * this.httpEndpoint.hashCode() : 0)
-            + 29_791 * this.resource.hashCode()
-            + 961 * this.service.hashCode()
-            + 31 * this.operationName.hashCode()
-            + this.type.hashCode()
-            + 31 * httpStatusCode
-            + (this.synthetics ? 1 : 0);
+
+    // Note: all the multiplication got constant folded at compile time (see javap -verbose ...)
+    int tmpHash =
+        (int) (31L * 31 * 31 * 31 * 31 * 31 * 31 * 31) * Boolean.hashCode(this.isTraceRoot) // 8
+            + (int) (31L * 31 * 31 * 31 * 31 * 31 * 31) * this.spanKind.hashCode() // 7
+            + 31 * 31 * 31 * 31 * 31 * 31 * this.peerTags.hashCode() // 6
+            + 31 * 31 * 31 * 31 * 31 * this.resource.hashCode() // 5
+            + 31 * 31 * 31 * 31 * this.service.hashCode() // 4
+            + 31 * 31 * 31 * this.operationName.hashCode() // 3
+            + 31 * 31 * this.type.hashCode() // 2
+            + 31 * this.httpStatusCode // 1
+            + (this.synthetics ? 1 : 0); // 0
+    // optional fields
+    if (this.serviceSource != null) {
+      tmpHash +=
+          (int) (31L * 31 * 31 * 31 * 31 * 31 * 31 * 31 * 31) * this.serviceSource.hashCode(); // 9
+    }
+    if (this.httpEndpoint != null) {
+      tmpHash +=
+          (int) (31L * 31 * 31 * 31 * 31 * 31 * 31 * 31 * 31 * 31)
+              * this.httpEndpoint.hashCode(); // 10
+    }
+    if (this.httpMethod != null) {
+      tmpHash +=
+          (int) (31L * 31 * 31 * 31 * 31 * 31 * 31 * 31 * 31 * 31 * 31)
+              * this.httpMethod.hashCode(); // 11
+    }
+    this.hash = tmpHash;
   }
 
   public UTF8BytesString getResource() {
@@ -127,29 +143,19 @@ public boolean equals(Object o) {
     }
     if ((o instanceof MetricKey)) {
       MetricKey metricKey = (MetricKey) o;
-      boolean basicEquals =
-          hash == metricKey.hash
-              && synthetics == metricKey.synthetics
-              && httpStatusCode == metricKey.httpStatusCode
-              && resource.equals(metricKey.resource)
-              && service.equals(metricKey.service)
-              && operationName.equals(metricKey.operationName)
-              && type.equals(metricKey.type)
-              && isTraceRoot == metricKey.isTraceRoot
-              && spanKind.equals(metricKey.spanKind)
-              && peerTags.equals(metricKey.peerTags);
-
-      // Only compare httpMethod and httpEndpoint if at least one of them is not null
-      // This ensures backward compatibility when the feature is disabled
-      boolean thisHasEndpoint = httpMethod != null || httpEndpoint != null;
-      boolean otherHasEndpoint = metricKey.httpMethod != null || metricKey.httpEndpoint != null;
-
-      if (thisHasEndpoint || otherHasEndpoint) {
-        return basicEquals
-            && java.util.Objects.equals(httpMethod, metricKey.httpMethod)
-            && java.util.Objects.equals(httpEndpoint, metricKey.httpEndpoint);
-      }
-      return basicEquals;
+      return hash == metricKey.hash
+          && synthetics == metricKey.synthetics
+          && httpStatusCode == metricKey.httpStatusCode
+          && resource.equals(metricKey.resource)
+          && service.equals(metricKey.service)
+          && operationName.equals(metricKey.operationName)
+          && type.equals(metricKey.type)
+          && isTraceRoot == metricKey.isTraceRoot
+          && spanKind.equals(metricKey.spanKind)
+          && peerTags.equals(metricKey.peerTags)
+          && Objects.equals(serviceSource, metricKey.serviceSource)
+          && Objects.equals(httpMethod, metricKey.httpMethod)
+          && Objects.equals(httpEndpoint, metricKey.httpEndpoint);
     }
     return false;
   }

dd-trace-core/src/main/java/datadog/trace/common/metrics/SerializingMetricWriter.java

@@ -37,7 +37,7 @@ public final class SerializingMetricWriter implements MetricWriter {
   private static final byte[] PEER_TAGS = "PeerTags".getBytes(ISO_8859_1);
   private static final byte[] HTTP_METHOD = "HTTPMethod".getBytes(ISO_8859_1);
   private static final byte[] HTTP_ENDPOINT = "HTTPEndpoint".getBytes(ISO_8859_1);
-  private static final byte[] SERVICE_SOURCE = "ServiceSource".getBytes(ISO_8859_1);
+  private static final byte[] SERVICE_SOURCE = "srv_src".getBytes(ISO_8859_1);
 
   // Constant declared here for compile-time folding
   public static final int TRISTATE_TRUE = TriState.TRUE.serialValue;

dd-trace-core/src/test/groovy/datadog/trace/common/metrics/ConflatingMetricAggregatorTest.groovy

@@ -797,22 +797,26 @@ class ConflatingMetricAggregatorTest extends DDSpecification {
     features.supportsMetrics() >> true
     features.peerTags() >> []
     ConflatingMetricsAggregator aggregator = new ConflatingMetricsAggregator(empty,
-      features, HealthMetrics.NO_OP, sink, writer, 10, queueSize, reportingInterval, SECONDS, true)
+      features, HealthMetrics.NO_OP, sink, writer, 10, queueSize, reportingInterval, SECONDS, false)
     aggregator.start()
 
-    when: "publish span with a service name source"
+    when: "publish spans with different service name source"
     CountDownLatch latch = new CountDownLatch(1)
     long duration = 100
     aggregator.publish([
+      new SimpleSpan("service", "operation", "resource", "type", true, true, false, 0, duration, 200, false, 0, "source")
+      .setTag(SPAN_KIND, "server"),
+      new SimpleSpan("service", "operation", "resource", "type", true, true, false, 0, duration, 200, false, 0, null)
+      .setTag(SPAN_KIND, "server"),
       new SimpleSpan("service", "operation", "resource", "type", true, true, false, 0, duration, 200, false, 0, "source")
       .setTag(SPAN_KIND, "server")
     ])
     aggregator.report()
     def latchTriggered = latch.await(2, SECONDS)
 
-    then: "should create the same metric keys for spans with and without s"
+    then: "should create the different metric keys for spans with and without sources"
     latchTriggered
-    1 * writer.startBucket(1, _, _)
+    1 * writer.startBucket(2, _, _)
     1 * writer.add(new MetricKey(
       "resource",
       "service",
@@ -826,6 +830,22 @@ class ConflatingMetricAggregatorTest extends DDSpecification {
       [],
       null,
       null
+      ), { AggregateMetric value ->
+        value.getHitCount() == 2 && value.getDuration() == 2 * duration
+      })
+    1 * writer.add(new MetricKey(
+      "resource",
+      "service",
+      "operation",
+      null,
+      "type",
+      200,
+      false,
+      false,
+      "server",
+      [],
+      null,
+      null
       ), { AggregateMetric value ->
         value.getHitCount() == 1 && value.getDuration() == duration
       })

dd-trace-core/src/test/groovy/datadog/trace/common/metrics/SerializingMetricWriterTest.groovy

@@ -230,7 +230,7 @@ class SerializingMetricWriterTest extends DDSpecification {
         ++elementCount
         // Service source is only present when the service name has been overridden by the tracer
         if (hasServiceSource) {
-          assert unpacker.unpackString() == "ServiceSource"
+          assert unpacker.unpackString() == "srv_src"
           assert unpacker.unpackString() == key.getServiceSource().toString()
           ++elementCount
         }