Change hash computation for protobuf to better represent impacting changes + save proto number in schema

dd-java-agent/instrumentation/avro/src/main/java/datadog/trace/instrumentation/avro/SchemaExtractor.java

@@ -104,7 +104,7 @@ public static boolean extractProperty(
     }
 
     return builder.addProperty(
-        schemaName, fieldName, array, type, description, ref, format, enumValues);
+        schemaName, fieldName, array, type, description, ref, format, enumValues, null);
   }
 
   public static boolean extractSchema(

dd-java-agent/instrumentation/protobuf/src/main/java/datadog/trace/instrumentation/protobuf_java/SchemaExtractor.java

@@ -10,6 +10,8 @@
 import datadog.trace.bootstrap.instrumentation.api.Schema;
 import datadog.trace.bootstrap.instrumentation.api.SchemaBuilder;
 import datadog.trace.bootstrap.instrumentation.api.SchemaIterator;
+import java.util.Comparator;
+import java.util.HashMap;
 import java.util.List;
 import java.util.stream.Collectors;
 
@@ -57,7 +59,8 @@ public static boolean extractProperty(
     if (field.isRepeated()) {
       array = true;
     }
-    switch (field.getType().toProto().getNumber()) {
+    int typeCode = field.getType().toProto().getNumber();
+    switch (typeCode) {
       case TYPE_DOUBLE:
         type = "number";
         format = "double";
@@ -107,6 +110,7 @@ public static boolean extractProperty(
         if (!extractSchema(field.getMessageType(), builder, depth)) {
           return false;
         }
+        builder.addToHash(field.getMessageType().getFullName());
         break;
       case TYPE_BYTES:
         type = "string";
@@ -123,6 +127,7 @@ public static boolean extractProperty(
         enumValues =
             field.getEnumType().getValues().stream()
                 .map(Descriptors.EnumValueDescriptor::getName)
+                .peek(builder::addToHash)
                 .collect(Collectors.toList());
         break;
       case TYPE_SFIXED32:
@@ -140,8 +145,13 @@ public static boolean extractProperty(
         description = "Unknown type";
         break;
     }
+    builder.addToHash(field.getNumber());
+    builder.addToHash(typeCode);
+    builder.addToHash(depth);
+    HashMap<String, String> extensions = new HashMap<String, String>(1);
+    extensions.put("x-protobuf-number", Integer.toString(field.getNumber()));
     return builder.addProperty(
-        schemaName, fieldName, array, type, description, ref, format, enumValues);
+        schemaName, fieldName, array, type, description, ref, format, enumValues, extensions);
   }
 
   public static boolean extractSchema(Descriptor descriptor, SchemaBuilder builder, int depth) {
@@ -150,7 +160,11 @@ public static boolean extractSchema(Descriptor descriptor, SchemaBuilder builder
     if (!builder.shouldExtractSchema(schemaName, depth)) {
       return false;
     }
-    for (FieldDescriptor field : descriptor.getFields()) {
+    // iterate fields in number order to ensure hash stability
+    for (FieldDescriptor field :
+        descriptor.getFields().stream()
+            .sorted(Comparator.comparingInt(FieldDescriptor::getNumber))
+            .collect(Collectors.toList())) {
       if (!extractProperty(field, schemaName, field.getName(), builder, depth)) {
         return false;
       }

dd-java-agent/instrumentation/protobuf/src/test/groovy/com/datadog/instrumentation/protobuf/AbstractMessageInstrumentationTest.groovy

@@ -1,23 +1,23 @@
 package com.datadog.instrumentation.protobuf
 
+import com.datadog.instrumentation.protobuf.generated.Message.MyMessage
+import com.datadog.instrumentation.protobuf.generated.Message.OtherMessage
 import com.google.protobuf.InvalidProtocolBufferException
 import datadog.trace.agent.test.AgentTestRunner
 import datadog.trace.api.DDTags
 import datadog.trace.bootstrap.instrumentation.api.AgentSpan
 
 import static datadog.trace.agent.test.utils.TraceUtils.runUnderTrace
 import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.activeSpan
-import com.datadog.instrumentation.protobuf.generated.Message.MyMessage
-import com.datadog.instrumentation.protobuf.generated.Message.OtherMessage
 
 class AbstractMessageInstrumentationTest extends AgentTestRunner {
   @Override
   protected boolean isDataStreamsEnabled() {
     return true
   }
 
-  String schema = "{\"components\":{\"schemas\":{\"com.datadog.instrumentation.protobuf.generated.MyMessage\":{\"properties\":{\"id\":{\"type\":\"string\"},\"value\":{\"type\":\"string\"},\"other_message\":{\"items\":{\"\$ref\":\"#/components/schemas/com.datadog.instrumentation.protobuf.generated.OtherMessage\"},\"type\":\"array\"}},\"type\":\"object\"},\"com.datadog.instrumentation.protobuf.generated.OtherMessage\":{\"properties\":{\"name\":{\"type\":\"string\"},\"age\":{\"format\":\"int32\",\"type\":\"integer\"}},\"type\":\"object\"}}},\"openapi\":\"3.0.0\"}"
-  String schemaID = "9054678588020233022"
+  String expectedSchema = "{\"components\":{\"schemas\":{\"com.datadog.instrumentation.protobuf.generated.MyMessage\":{\"properties\":{\"id\":{\"extensions\":{\"x-protobuf-number\":\"1\"},\"type\":\"string\"},\"value\":{\"extensions\":{\"x-protobuf-number\":\"2\"},\"type\":\"string\"},\"other_message\":{\"extensions\":{\"x-protobuf-number\":\"3\"},\"items\":{\"\$ref\":\"#/components/schemas/com.datadog.instrumentation.protobuf.generated.OtherMessage\"},\"type\":\"array\"}},\"type\":\"object\"},\"com.datadog.instrumentation.protobuf.generated.OtherMessage\":{\"properties\":{\"name\":{\"extensions\":{\"x-protobuf-number\":\"1\"},\"type\":\"string\"},\"age\":{\"extensions\":{\"x-protobuf-number\":\"2\"},\"format\":\"int32\",\"type\":\"integer\"}},\"type\":\"object\"}}},\"openapi\":\"3.0.0\"}"
+  String expectedSchemaID = "4690647329509494987"
 
 
   void 'test extract protobuf schema on serialize & deserialize'() {
@@ -50,12 +50,12 @@ class AbstractMessageInstrumentationTest extends AgentTestRunner {
           errored false
           measured false
           tags {
-            "$DDTags.SCHEMA_DEFINITION" schema
+            "$DDTags.SCHEMA_DEFINITION" expectedSchema
             "$DDTags.SCHEMA_WEIGHT" 1
             "$DDTags.SCHEMA_TYPE" "protobuf"
             "$DDTags.SCHEMA_NAME" "com.datadog.instrumentation.protobuf.generated.MyMessage"
             "$DDTags.SCHEMA_OPERATION" "serialization"
-            "$DDTags.SCHEMA_ID" schemaID
+            "$DDTags.SCHEMA_ID" expectedSchemaID
             defaultTags(false)
           }
         }
@@ -68,12 +68,12 @@ class AbstractMessageInstrumentationTest extends AgentTestRunner {
           errored false
           measured false
           tags {
-            "$DDTags.SCHEMA_DEFINITION" schema
+            "$DDTags.SCHEMA_DEFINITION" expectedSchema
             "$DDTags.SCHEMA_WEIGHT" 1
             "$DDTags.SCHEMA_TYPE" "protobuf"
             "$DDTags.SCHEMA_NAME" "com.datadog.instrumentation.protobuf.generated.MyMessage"
             "$DDTags.SCHEMA_OPERATION" "deserialization"
-            "$DDTags.SCHEMA_ID" schemaID
+            "$DDTags.SCHEMA_ID" expectedSchemaID
             defaultTags(false)
           }
         }

dd-java-agent/instrumentation/protobuf/src/test/groovy/com/datadog/instrumentation/protobuf/DynamicMessageInstrumentationTest.groovy

@@ -1,10 +1,10 @@
 package com.datadog.instrumentation.protobuf
 
+import com.datadog.instrumentation.protobuf.generated.Message.MyMessage
 import com.google.protobuf.DynamicMessage
 import datadog.trace.agent.test.AgentTestRunner
 import datadog.trace.api.DDTags
 import datadog.trace.bootstrap.instrumentation.api.AgentSpan
-import com.datadog.instrumentation.protobuf.generated.Message.MyMessage
 
 import static datadog.trace.agent.test.utils.TraceUtils.runUnderTrace
 import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.activeSpan
@@ -22,8 +22,8 @@ class DynamicMessageInstrumentationTest extends AgentTestRunner {
       .setValue("Hello from Protobuf!")
       .build()
     when:
-    String schema = "{\"components\":{\"schemas\":{\"com.datadog.instrumentation.protobuf.generated.MyMessage\":{\"properties\":{\"id\":{\"type\":\"string\"},\"value\":{\"type\":\"string\"},\"other_message\":{\"items\":{\"\$ref\":\"#/components/schemas/com.datadog.instrumentation.protobuf.generated.OtherMessage\"},\"type\":\"array\"}},\"type\":\"object\"},\"com.datadog.instrumentation.protobuf.generated.OtherMessage\":{\"properties\":{\"name\":{\"type\":\"string\"},\"age\":{\"format\":\"int32\",\"type\":\"integer\"}},\"type\":\"object\"}}},\"openapi\":\"3.0.0\"}"
-    String schemaID = "9054678588020233022"
+    String expectedSchema = "{\"components\":{\"schemas\":{\"com.datadog.instrumentation.protobuf.generated.MyMessage\":{\"properties\":{\"id\":{\"extensions\":{\"x-protobuf-number\":\"1\"},\"type\":\"string\"},\"value\":{\"extensions\":{\"x-protobuf-number\":\"2\"},\"type\":\"string\"},\"other_message\":{\"extensions\":{\"x-protobuf-number\":\"3\"},\"items\":{\"\$ref\":\"#/components/schemas/com.datadog.instrumentation.protobuf.generated.OtherMessage\"},\"type\":\"array\"}},\"type\":\"object\"},\"com.datadog.instrumentation.protobuf.generated.OtherMessage\":{\"properties\":{\"name\":{\"extensions\":{\"x-protobuf-number\":\"1\"},\"type\":\"string\"},\"age\":{\"extensions\":{\"x-protobuf-number\":\"2\"},\"format\":\"int32\",\"type\":\"integer\"}},\"type\":\"object\"}}},\"openapi\":\"3.0.0\"}"
+    String expectedSchemaID = "4690647329509494987"
     var bytes
     runUnderTrace("parent_serialize") {
       AgentSpan span = activeSpan()
@@ -46,12 +46,12 @@ class DynamicMessageInstrumentationTest extends AgentTestRunner {
           errored false
           measured false
           tags {
-            "$DDTags.SCHEMA_DEFINITION" schema
+            "$DDTags.SCHEMA_DEFINITION" expectedSchema
             "$DDTags.SCHEMA_WEIGHT" 1
             "$DDTags.SCHEMA_TYPE" "protobuf"
             "$DDTags.SCHEMA_NAME" "com.datadog.instrumentation.protobuf.generated.MyMessage"
             "$DDTags.SCHEMA_OPERATION" "serialization"
-            "$DDTags.SCHEMA_ID" schemaID
+            "$DDTags.SCHEMA_ID" expectedSchemaID
             defaultTags(false)
           }
         }
@@ -64,12 +64,12 @@ class DynamicMessageInstrumentationTest extends AgentTestRunner {
           errored false
           measured false
           tags {
-            "$DDTags.SCHEMA_DEFINITION" schema
+            "$DDTags.SCHEMA_DEFINITION" expectedSchema
             "$DDTags.SCHEMA_WEIGHT" 1
             "$DDTags.SCHEMA_TYPE" "protobuf"
             "$DDTags.SCHEMA_NAME" "com.datadog.instrumentation.protobuf.generated.MyMessage"
             "$DDTags.SCHEMA_OPERATION" "deserialization"
-            "$DDTags.SCHEMA_ID" schemaID
+            "$DDTags.SCHEMA_ID" expectedSchemaID
             defaultTags(false)
           }
         }

dd-trace-core/src/main/java/datadog/trace/core/datastreams/SchemaBuilder.java

@@ -17,6 +17,8 @@ public class SchemaBuilder implements datadog.trace.bootstrap.instrumentation.ap
   private static final DDCache<String, Schema> CACHE = DDCaches.newFixedSizeCache(32);
   private static final int maxDepth = 10;
   private static final int maxProperties = 1000;
+  private static final long HASH_INIT = FNV64Hash.generateHash(new byte[0], FNV64Hash.Version.v1A);
+  private long currentHash = HASH_INIT;
   private int properties;
   private final SchemaIterator iterator;
 
@@ -33,27 +35,41 @@ public boolean addProperty(
       String description,
       String ref,
       String format,
-      List<String> enumValues) {
+      List<String> enumValues,
+      Map<String, String> extensions) {
     if (properties >= maxProperties) {
       return false;
     }
     properties++;
     OpenApiSchema.Property property =
-        new OpenApiSchema.Property(type, description, ref, format, enumValues, null);
+        new OpenApiSchema.Property(
+            type, description, ref, format, enumValues, isArray ? null : extensions, null);
     if (isArray) {
-      property = new OpenApiSchema.Property("array", null, null, null, null, property);
+      property = new OpenApiSchema.Property("array", null, null, null, null, extensions, property);
     }
     schema.components.schemas.get(schemaName).properties.put(fieldName, property);
     return true;
   }
 
+  public void addToHash(int value) {
+    addToHash(Integer.toString(value));
+  }
+
+  public void addToHash(String value) {
+    currentHash = FNV64Hash.continueHash(currentHash, value, FNV64Hash.Version.v1A);
+  }
+
   public Schema build() {
     this.iterator.iterateOverSchema(this);
     Moshi moshi = new Moshi.Builder().build();
     JsonAdapter<OpenApiSchema> jsonAdapter = moshi.adapter(OpenApiSchema.class);
     String definition = jsonAdapter.toJson(this.schema);
-    String id = Long.toUnsignedString(FNV64Hash.generateHash(definition, FNV64Hash.Version.v1A));
-    return new Schema(definition, id);
+    if (currentHash == HASH_INIT) {
+      // if hash was not computed along the way,
+      // we fall back to computing it from the json representation of the schema
+      currentHash = FNV64Hash.generateHash(definition, FNV64Hash.Version.v1A);
+    }
+    return new Schema(definition, Long.toUnsignedString(currentHash));
   }
 
   @Override
@@ -93,6 +109,7 @@ public static class Property {
       @Json(name = "enum")
       public List<String> enumValues;
 
+      public final Map<String, String> extensions;
       public Property items;
 
       public Property(
@@ -101,12 +118,14 @@ public Property(
           String ref,
           String format,
           List<String> enumValues,
+          Map<String, String> extensions,
           Property items) {
         this.type = type;
         this.description = description;
         this.ref = ref;
         this.format = format;
         this.enumValues = enumValues;
+        this.extensions = extensions;
         this.items = items;
       }
     }

dd-trace-core/src/test/groovy/datadog/trace/core/datastreams/SchemaBuilderTest.groovy

@@ -10,12 +10,15 @@ class SchemaBuilderTest extends DDCoreSpecification {
 
     @Override
     void iterateOverSchema(datadog.trace.bootstrap.instrumentation.api.SchemaBuilder builder) {
-      builder.addProperty("person", "name", false, "string", "name of the person", null, null, null)
-      builder.addProperty("person", "phone_numbers", true, "string", null, null, null, null)
-      builder.addProperty("person", "person_name", false, "string", null, null, null, null)
-      builder.addProperty("person", "address", false, "object", null, "#/components/schemas/address", null, null)
-      builder.addProperty("address", "zip", false, "number", null, null, "int", null)
-      builder.addProperty("address", "street", false, "string", null, null, null, null)
+      HashMap<String, String> extension = new HashMap<String, String>(1)
+      extension.put("x-test-extension-1", "hello")
+      extension.put("x-test-extension-2", "world")
+      builder.addProperty("person", "name", false, "string", "name of the person", null, null, null, null)
+      builder.addProperty("person", "phone_numbers", true, "string", null, null, null, null, null)
+      builder.addProperty("person", "person_name", false, "string", null, null, null, null, null)
+      builder.addProperty("person", "address", false, "object", null, "#/components/schemas/address", null, null, null)
+      builder.addProperty("address", "zip", false, "number", null, null, "int", null, null)
+      builder.addProperty("address", "street", false, "string", null, null, null, null, extension)
     }
   }
 
@@ -31,8 +34,8 @@ class SchemaBuilderTest extends DDCoreSpecification {
     Schema schema = builder.build()
 
     then:
-    "{\"components\":{\"schemas\":{\"person\":{\"properties\":{\"name\":{\"description\":\"name of the person\",\"type\":\"string\"},\"phone_numbers\":{\"items\":{\"type\":\"string\"},\"type\":\"array\"},\"person_name\":{\"type\":\"string\"},\"address\":{\"\$ref\":\"#/components/schemas/address\",\"type\":\"object\"}},\"type\":\"object\"},\"address\":{\"properties\":{\"zip\":{\"format\":\"int\",\"type\":\"number\"},\"street\":{\"type\":\"string\"}},\"type\":\"object\"}}},\"openapi\":\"3.0.0\"}" == schema.definition
-    "14950130709604290100" == schema.id
+    "{\"components\":{\"schemas\":{\"person\":{\"properties\":{\"name\":{\"description\":\"name of the person\",\"type\":\"string\"},\"phone_numbers\":{\"items\":{\"type\":\"string\"},\"type\":\"array\"},\"person_name\":{\"type\":\"string\"},\"address\":{\"\$ref\":\"#/components/schemas/address\",\"type\":\"object\"}},\"type\":\"object\"},\"address\":{\"properties\":{\"zip\":{\"format\":\"int\",\"type\":\"number\"},\"street\":{\"extensions\":{\"x-test-extension-1\":\"hello\",\"x-test-extension-2\":\"world\"},\"type\":\"string\"}},\"type\":\"object\"}}},\"openapi\":\"3.0.0\"}" == schema.definition
+    "16548065305426330543" == schema.id
     shouldExtractPerson
     shouldExtractAddress
     !shouldExtractPerson2

internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/SchemaBuilder.java

@@ -1,6 +1,7 @@
 package datadog.trace.bootstrap.instrumentation.api;
 
 import java.util.List;
+import java.util.Map;
 
 public interface SchemaBuilder {
   boolean addProperty(
@@ -11,7 +12,12 @@ boolean addProperty(
       String description,
       String ref,
       String format,
-      List<String> enumValues);
+      List<String> enumValue,
+      Map<String, String> extensions);
+
+  void addToHash(int value);
+
+  void addToHash(String value);
 
   boolean shouldExtractSchema(String schemaName, int depth);
 }