Missing unit fields for Android measurements (#2204)

marandaneto · adinauer · getsentry-bot · web-flow · commit e2ea31c73c93 · 2022-09-08T08:15:58.000+02:00
Co-authored-by: Alexander Dinauer &lt;alexander.dinauer@sentry.io&gt;
Co-authored-by: Alexander Dinauer &lt;adinauer@users.noreply.github.com&gt;
Co-authored-by: Sentry Github Bot &lt;bot+github-bot@sentry.io&gt;
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -5,6 +5,7 @@
 ### Fixes
 
 - Fixed AbstractMethodError when getting Lifecycle ([#2228](https://github.com/getsentry/sentry-java/pull/2228))
+- Missing unit fields for Android measurements ([#2204](https://github.com/getsentry/sentry-java/pull/2204))
 - Avoid sending empty profiles ([#2232](https://github.com/getsentry/sentry-java/pull/2232))
 
 ## 6.4.1
diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/ActivityFramesTracker.java b/sentry-android-core/src/main/java/io/sentry/android/core/ActivityFramesTracker.java
@@ -1,5 +1,7 @@
 package io.sentry.android.core;
 
+import static io.sentry.protocol.MeasurementValue.NONE_UNIT;
+
 import android.app.Activity;
 import android.util.SparseIntArray;
 import androidx.core.app.FrameMetricsAggregator;
@@ -102,9 +104,9 @@ public synchronized void setMetrics(
       return;
     }
 
-    final MeasurementValue tfValues = new MeasurementValue(totalFrames);
-    final MeasurementValue sfValues = new MeasurementValue(slowFrames);
-    final MeasurementValue ffValues = new MeasurementValue(frozenFrames);
+    final MeasurementValue tfValues = new MeasurementValue(totalFrames, NONE_UNIT);
+    final MeasurementValue sfValues = new MeasurementValue(slowFrames, NONE_UNIT);
+    final MeasurementValue ffValues = new MeasurementValue(frozenFrames, NONE_UNIT);
     final Map<String, @NotNull MeasurementValue> measurements = new HashMap<>();
     measurements.put("frames_total", tfValues);
     measurements.put("frames_slow", sfValues);
diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/PerformanceAndroidEventProcessor.java b/sentry-android-core/src/main/java/io/sentry/android/core/PerformanceAndroidEventProcessor.java
@@ -3,6 +3,7 @@
 import static io.sentry.android.core.ActivityLifecycleIntegration.APP_START_COLD;
 import static io.sentry.android.core.ActivityLifecycleIntegration.APP_START_WARM;
 import static io.sentry.android.core.ActivityLifecycleIntegration.UI_LOAD_OP;
+import static io.sentry.protocol.MeasurementValue.MILLISECOND_UNIT;
 
 import io.sentry.EventProcessor;
 import io.sentry.Hint;
@@ -65,7 +66,8 @@ public SentryEvent process(@NotNull SentryEvent event, @NotNull Hint hint) {
       final Long appStartUpInterval = AppStartState.getInstance().getAppStartInterval();
       // if appStartUpInterval is null, metrics are not ready to be sent
       if (appStartUpInterval != null) {
-        final MeasurementValue value = new MeasurementValue((float) appStartUpInterval);
+        final MeasurementValue value =
+            new MeasurementValue((float) appStartUpInterval, MILLISECOND_UNIT);
 
         final String appStartKey =
             AppStartState.getInstance().isColdStart() ? "app_start_cold" : "app_start_warm";
diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/ActivityFramesTrackerTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/ActivityFramesTrackerTest.kt
@@ -42,6 +42,7 @@ class ActivityFramesTrackerTest {
         val totalFrames = metrics!!["frames_total"]
 
         assertEquals(totalFrames!!.value, 1f)
+        assertEquals(totalFrames.unit, "none")
     }
 
     @Test
@@ -57,6 +58,7 @@ class ActivityFramesTrackerTest {
         val frozenFrames = metrics!!["frames_frozen"]
 
         assertEquals(frozenFrames!!.value, 5f)
+        assertEquals(frozenFrames.unit, "none")
     }
 
     @Test
@@ -72,6 +74,7 @@ class ActivityFramesTrackerTest {
         val slowFrames = metrics!!["frames_slow"]
 
         assertEquals(slowFrames!!.value, 5f)
+        assertEquals(slowFrames.unit, "none")
     }
 
     @Test
diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/PerformanceAndroidEventProcessorTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/PerformanceAndroidEventProcessorTest.kt
@@ -10,10 +10,12 @@ import io.sentry.TracesSamplingDecision
 import io.sentry.TransactionContext
 import io.sentry.android.core.ActivityLifecycleIntegration.UI_LOAD_OP
 import io.sentry.protocol.MeasurementValue
+import io.sentry.protocol.MeasurementValue.MILLISECOND_UNIT
 import io.sentry.protocol.SentryTransaction
 import java.util.Date
 import kotlin.test.BeforeTest
 import kotlin.test.Test
+import kotlin.test.assertEquals
 import kotlin.test.assertTrue
 
 class PerformanceAndroidEventProcessorTest {
@@ -64,6 +66,19 @@ class PerformanceAndroidEventProcessorTest {
         assertTrue(tr.measurements.containsKey("app_start_warm"))
     }
 
+    @Test
+    fun `set app cold start unit measurement`() {
+        val sut = fixture.getSut()
+
+        var tr = getTransaction()
+        setAppStart()
+
+        tr = sut.process(tr, Hint())
+
+        val measurement = tr.measurements["app_start_cold"]
+        assertEquals("millisecond", measurement?.unit)
+    }
+
     @Test
     fun `do not add app start metric twice`() {
         val sut = fixture.getSut()
@@ -140,7 +155,7 @@ class PerformanceAndroidEventProcessorTest {
         val tracer = SentryTracer(context, fixture.hub)
         var tr = SentryTransaction(tracer)
 
-        val metrics = mapOf("frames_total" to MeasurementValue(1f))
+        val metrics = mapOf("frames_total" to MeasurementValue(1f, MILLISECOND_UNIT))
         whenever(fixture.activityFramesTracker.takeMetrics(any())).thenReturn(metrics)
 
         tr = sut.process(tr, Hint())
diff --git a/sentry/api/sentry.api b/sentry/api/sentry.api
@@ -517,6 +517,7 @@ public final class io/sentry/JsonObjectReader : io/sentry/vendor/gson/stream/Jso
 	public fun nextIntegerOrNull ()Ljava/lang/Integer;
 	public fun nextList (Lio/sentry/ILogger;Lio/sentry/JsonDeserializer;)Ljava/util/List;
 	public fun nextLongOrNull ()Ljava/lang/Long;
+	public fun nextMapOrNull (Lio/sentry/ILogger;Lio/sentry/JsonDeserializer;)Ljava/util/Map;
 	public fun nextObjectOrNull ()Ljava/lang/Object;
 	public fun nextOrNull (Lio/sentry/ILogger;Lio/sentry/JsonDeserializer;)Ljava/lang/Object;
 	public fun nextStringOrNull ()Ljava/lang/String;
@@ -2366,10 +2367,16 @@ public final class io/sentry/protocol/Gpu$JsonKeys {
 	public fun <init> ()V
 }
 
-public final class io/sentry/protocol/MeasurementValue : io/sentry/JsonSerializable {
-	public fun <init> (F)V
+public final class io/sentry/protocol/MeasurementValue : io/sentry/JsonSerializable, io/sentry/JsonUnknown {
+	public static final field MILLISECOND_UNIT Ljava/lang/String;
+	public static final field NONE_UNIT Ljava/lang/String;
+	public fun <init> (FLjava/lang/String;)V
+	public fun <init> (FLjava/lang/String;Ljava/util/Map;)V
+	public fun getUnit ()Ljava/lang/String;
+	public fun getUnknown ()Ljava/util/Map;
 	public fun getValue ()F
 	public fun serialize (Lio/sentry/JsonObjectWriter;Lio/sentry/ILogger;)V
+	public fun setUnknown (Ljava/util/Map;)V
 }
 
 public final class io/sentry/protocol/MeasurementValue$Deserializer : io/sentry/JsonDeserializer {
@@ -2379,6 +2386,7 @@ public final class io/sentry/protocol/MeasurementValue$Deserializer : io/sentry/
 }
 
 public final class io/sentry/protocol/MeasurementValue$JsonKeys {
+	public static final field UNIT Ljava/lang/String;
 	public static final field VALUE Ljava/lang/String;
 	public fun <init> ()V
 }
diff --git a/sentry/src/main/java/io/sentry/JsonObjectReader.java b/sentry/src/main/java/io/sentry/JsonObjectReader.java
@@ -6,6 +6,7 @@
 import java.io.Reader;
 import java.util.ArrayList;
 import java.util.Date;
+import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.TimeZone;
@@ -99,6 +100,27 @@ public void nextUnknown(ILogger logger, Map<String, Object> unknown, String name
     return list;
   }
 
+  public <T> @Nullable Map<String, T> nextMapOrNull(
+      @NotNull ILogger logger, @NotNull JsonDeserializer<T> deserializer) throws IOException {
+    if (peek() == JsonToken.NULL) {
+      nextNull();
+      return null;
+    }
+    beginObject();
+    Map<String, T> map = new HashMap<>();
+    do {
+      try {
+        String key = nextName();
+        map.put(key, deserializer.deserialize(this, logger));
+      } catch (Exception e) {
+        logger.log(SentryLevel.ERROR, "Failed to deserialize object in map.", e);
+      }
+    } while (peek() == JsonToken.BEGIN_OBJECT || peek() == JsonToken.NAME);
+
+    endObject();
+    return map;
+  }
+
   public <T> @Nullable T nextOrNull(
       @NotNull ILogger logger, @NotNull JsonDeserializer<T> deserializer) throws Exception {
     if (peek() == JsonToken.NULL) {
diff --git a/sentry/src/main/java/io/sentry/protocol/MeasurementValue.java b/sentry/src/main/java/io/sentry/protocol/MeasurementValue.java
@@ -5,36 +5,88 @@
 import io.sentry.JsonObjectReader;
 import io.sentry.JsonObjectWriter;
 import io.sentry.JsonSerializable;
+import io.sentry.JsonUnknown;
+import io.sentry.vendor.gson.stream.JsonToken;
 import java.io.IOException;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
 import org.jetbrains.annotations.ApiStatus;
 import org.jetbrains.annotations.NotNull;
+import org.jetbrains.annotations.Nullable;
 import org.jetbrains.annotations.TestOnly;
 
 @ApiStatus.Internal
-public final class MeasurementValue implements JsonSerializable {
+public final class MeasurementValue implements JsonUnknown, JsonSerializable {
+
+  public static final @NotNull String NONE_UNIT = "none";
+  public static final @NotNull String MILLISECOND_UNIT = "millisecond";
+
   @SuppressWarnings("UnusedVariable")
   private final float value;
 
-  public MeasurementValue(final float value) {
+  private final @Nullable String unit;
+
+  /** the unknown fields of breadcrumbs, internal usage only */
+  private @Nullable Map<String, Object> unknown;
+
+  public MeasurementValue(final float value, final @Nullable String unit) {
     this.value = value;
+    this.unit = unit;
+  }
+
+  @TestOnly
+  public MeasurementValue(
+      final float value, final @Nullable String unit, final @Nullable Map<String, Object> unknown) {
+    this.value = value;
+    this.unit = unit;
+    this.unknown = unknown;
   }
 
   @TestOnly
   public float getValue() {
     return value;
   }
 
+  public @Nullable String getUnit() {
+    return unit;
+  }
+
+  @Nullable
+  @Override
+  public Map<String, Object> getUnknown() {
+    return unknown;
+  }
+
+  @Override
+  public void setUnknown(@Nullable Map<String, Object> unknown) {
+    this.unknown = unknown;
+  }
+
   // JsonSerializable
 
   public static final class JsonKeys {
     public static final String VALUE = "value";
+    public static final String UNIT = "unit";
   }
 
   @Override
   public void serialize(@NotNull JsonObjectWriter writer, @NotNull ILogger logger)
       throws IOException {
     writer.beginObject();
     writer.name(JsonKeys.VALUE).value(value);
+
+    if (unit != null) {
+      writer.name(JsonKeys.UNIT).value(unit);
+    }
+
+    if (unknown != null) {
+      for (final String key : unknown.keySet()) {
+        final Object value = unknown.get(key);
+        writer.name(key);
+        writer.value(logger, value);
+      }
+    }
+
     writer.endObject();
   }
 
@@ -43,10 +95,34 @@ public static final class Deserializer implements JsonDeserializer<MeasurementVa
     public @NotNull MeasurementValue deserialize(
         @NotNull JsonObjectReader reader, @NotNull ILogger logger) throws Exception {
       reader.beginObject();
-      reader.nextName();
-      MeasurementValue measurementValue = new MeasurementValue(reader.nextFloat());
+
+      String unit = null;
+      float value = 0;
+      Map<String, Object> unknown = null;
+
+      while (reader.peek() == JsonToken.NAME) {
+        final String nextName = reader.nextName();
+        switch (nextName) {
+          case JsonKeys.VALUE:
+            value = reader.nextFloat();
+            break;
+          case JsonKeys.UNIT:
+            unit = reader.nextStringOrNull();
+            break;
+          default:
+            if (unknown == null) {
+              unknown = new ConcurrentHashMap<>();
+            }
+            reader.nextUnknown(logger, unknown, nextName);
+            break;
+        }
+      }
+
       reader.endObject();
-      return measurementValue;
+      final MeasurementValue measurement = new MeasurementValue(value, unit);
+      measurement.setUnknown(unknown);
+
+      return measurement;
     }
   }
 }
diff --git a/sentry/src/main/java/io/sentry/protocol/SentryTransaction.java b/sentry/src/main/java/io/sentry/protocol/SentryTransaction.java
@@ -280,8 +280,8 @@ public static final class Deserializer implements JsonDeserializer<SentryTransac
             reader.nextString(); // No need to assign, as it is final.
             break;
           case JsonKeys.MEASUREMENTS:
-            Map<String, @NotNull MeasurementValue> deserializedMeasurements =
-                (Map<String, @NotNull MeasurementValue>) reader.nextObjectOrNull();
+            Map<String, MeasurementValue> deserializedMeasurements =
+                reader.nextMapOrNull(logger, new MeasurementValue.Deserializer());
             if (deserializedMeasurements != null) {
               transaction.measurements.putAll(deserializedMeasurements);
             }
diff --git a/sentry/src/test/java/io/sentry/JsonObjectReaderTest.kt b/sentry/src/test/java/io/sentry/JsonObjectReaderTest.kt
@@ -146,6 +146,35 @@ class JsonObjectReaderTest {
         assertEquals(expected, actual)
     }
 
+    // nextMap
+
+    @Test
+    fun `returns null for null map`() {
+        val jsonString = "{\"key\": null}"
+        val reader = fixture.getSut(jsonString)
+        reader.beginObject()
+        reader.nextName()
+
+        assertNull(reader.nextMapOrNull(fixture.logger, Deserializable.Deserializer()))
+    }
+
+    @Test
+    fun `returns map of deserializables`() {
+        val deserializableA = "{\"foo\": \"foo\", \"bar\": \"bar\"}"
+        val deserializableB = "{\"foo\": \"fooo\", \"bar\": \"baar\"}"
+        val jsonString = "{\"deserializable\": { \"a\":$deserializableA,\"b\":$deserializableB}}"
+        val reader = fixture.getSut(jsonString)
+        reader.beginObject()
+        reader.nextName()
+
+        val expected = mapOf(
+            "a" to Deserializable("foo", "bar"),
+            "b" to Deserializable("fooo", "baar")
+        )
+        val actual = reader.nextMapOrNull(fixture.logger, Deserializable.Deserializer())
+        assertEquals(expected, actual)
+    }
+
     // nextDateOrNull
 
     @Test
diff --git a/sentry/src/test/java/io/sentry/protocol/MeasurementValueSerializationTest.kt b/sentry/src/test/java/io/sentry/protocol/MeasurementValueSerializationTest.kt
@@ -16,7 +16,7 @@ class MeasurementValueSerializationTest {
     class Fixture {
         val logger = mock<ILogger>()
         // float cannot represent 0.3 correctly https://docs.oracle.com/cd/E19957-01/806-3568/ncg_goldberg.html
-        fun getSut() = MeasurementValue(0.30000001192092896f)
+        fun getSut(value: Float = 0.30000001192092896f, unit: String = "test") = MeasurementValue(value, unit, mapOf<String, Any>("new_type" to "newtype"))
     }
     private val fixture = Fixture()
 
diff --git a/sentry/src/test/java/io/sentry/protocol/SentryTransactionSerializationTest.kt b/sentry/src/test/java/io/sentry/protocol/SentryTransactionSerializationTest.kt
@@ -25,7 +25,8 @@ class SentryTransactionSerializationTest {
                 SentrySpanSerializationTest.Fixture().getSut()
             ),
             mapOf(
-                "386384cb-1162-49e7-aea1-db913d4fca63" to MeasurementValueSerializationTest.Fixture().getSut()
+                "386384cb-1162-49e7-aea1-db913d4fca63" to MeasurementValueSerializationTest.Fixture().getSut(),
+                "186384cb-1162-49e7-aea1-db913d4fca63" to MeasurementValueSerializationTest.Fixture().getSut(0.4000000059604645f, "test2")
             ),
             TransactionInfo(TransactionNameSource.CUSTOM.apiName())
         ).apply {
@@ -49,6 +50,14 @@ class SentryTransactionSerializationTest {
         assertEquals(expectedJson, actualJson)
     }
 
+    @Test
+    fun `deserialize without measurement unit`() {
+        val expectedJson = sanitizedFile("json/sentry_transaction_no_measurement_unit.json")
+        val actual = deserialize(expectedJson)
+        val actualJson = serialize(actual)
+        assertEquals(expectedJson, actualJson)
+    }
+
     @Test
     fun `deserialize legacy date format and missing transaction name source`() {
         val expectedJson = sanitizedFile("json/sentry_transaction_legacy_date_format.json")
diff --git a/sentry/src/test/resources/json/measurement_value.json b/sentry/src/test/resources/json/measurement_value.json
diff --git a/sentry/src/test/resources/json/sentry_transaction.json b/sentry/src/test/resources/json/sentry_transaction.json
diff --git a/sentry/src/test/resources/json/sentry_transaction_legacy_date_format.json b/sentry/src/test/resources/json/sentry_transaction_legacy_date_format.json
diff --git a/sentry/src/test/resources/json/sentry_transaction_no_measurement_unit.json b/sentry/src/test/resources/json/sentry_transaction_no_measurement_unit.json

Original file line number	Diff line number	Diff line change
`@@ -42,6 +42,7 @@ class ActivityFramesTrackerTest {`
`42`	`42`	`val totalFrames = metrics!!["frames_total"]`
`43`	`43`
`44`	`44`	`assertEquals(totalFrames!!.value, 1f)`
	`45`	`+ assertEquals(totalFrames.unit, "none")`
`45`	`46`	`}`
`46`	`47`
`47`	`48`	`@Test`
`@@ -57,6 +58,7 @@ class ActivityFramesTrackerTest {`
`57`	`58`	`val frozenFrames = metrics!!["frames_frozen"]`
`58`	`59`
`59`	`60`	`assertEquals(frozenFrames!!.value, 5f)`
	`61`	`+ assertEquals(frozenFrames.unit, "none")`
`60`	`62`	`}`
`61`	`63`
`62`	`64`	`@Test`
`@@ -72,6 +74,7 @@ class ActivityFramesTrackerTest {`
`72`	`74`	`val slowFrames = metrics!!["frames_slow"]`
`73`	`75`
`74`	`76`	`assertEquals(slowFrames!!.value, 5f)`
	`77`	`+ assertEquals(slowFrames.unit, "none")`
`75`	`78`	`}`
`76`	`79`
`77`	`80`	`@Test`