Add an overload to FirebaseCrashlytics.recordException to attach additional custom key value pairs (#6528)

This PR adds a method `public void recordException(@nonnull Throwable
throwable, CustomKeysAndValues keysAndValues)` as an overload to the
existing `recordException` method in Crashlytics to allow attaching
additional custom keys to the specific event.

Details:
- The total number of custom key/value pairs are still restricted to 64
- App level custom keys take precedence over event specific custom keys
for the 64 key/value pair limit
- The values of event keys override the value of global custom keys if
they're identical

Additionally:
- Creates a new EventMetadata class to represent `sessionId` and
`timestamp` attached to non fatal events.

Author: tejasd

firebase-crashlytics/CHANGELOG.md

@@ -1,5 +1,6 @@
 # Unreleased
-
+* [feature] Added an overload for `recordException` that allows logging additional custom
+  keys to the non fatal event [#3551]
 
 # 19.3.0
 * [fixed] Fixed inefficiency in the Kotlin `FirebaseCrashlytics.setCustomKeys` extension.

firebase-crashlytics/api.txt

@@ -20,10 +20,11 @@ package com.google.firebase.crashlytics {
     method public void deleteUnsentReports();
     method public boolean didCrashOnPreviousExecution();
     method @NonNull public static com.google.firebase.crashlytics.FirebaseCrashlytics getInstance();
+    method public boolean isCrashlyticsCollectionEnabled();
     method public void log(@NonNull String);
     method public void recordException(@NonNull Throwable);
+    method public void recordException(@NonNull Throwable, @NonNull com.google.firebase.crashlytics.CustomKeysAndValues);
     method public void sendUnsentReports();
-    method public boolean isCrashlyticsCollectionEnabled();
     method public void setCrashlyticsCollectionEnabled(boolean);
     method public void setCrashlyticsCollectionEnabled(@Nullable Boolean);
     method public void setCustomKey(@NonNull String, boolean);

firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/common/CrashlyticsControllerTest.java

@@ -44,6 +44,7 @@
 import com.google.firebase.crashlytics.internal.NativeSessionFileProvider;
 import com.google.firebase.crashlytics.internal.analytics.AnalyticsEventLogger;
 import com.google.firebase.crashlytics.internal.concurrency.CrashlyticsWorkers;
+import com.google.firebase.crashlytics.internal.metadata.EventMetadata;
 import com.google.firebase.crashlytics.internal.metadata.LogFileManager;
 import com.google.firebase.crashlytics.internal.metadata.UserMetadata;
 import com.google.firebase.crashlytics.internal.model.CrashlyticsReport;
@@ -57,6 +58,7 @@
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
+import java.util.Map;
 import java.util.TreeSet;
 import java.util.concurrent.Executor;
 import java.util.concurrent.TimeUnit;
@@ -216,14 +218,14 @@ public void testWriteNonFatal_callsSessionReportingCoordinatorPersistNonFatal()
     when(mockSessionReportingCoordinator.listSortedOpenSessionIds())
         .thenReturn(new TreeSet<>(Collections.singleton(sessionId)));
 
-    controller.writeNonFatalException(thread, nonFatal);
+    controller.writeNonFatalException(thread, nonFatal, Map.of());
     crashlyticsWorkers.common.submit(() -> controller.doCloseSessions(testSettingsProvider));
 
     crashlyticsWorkers.common.await();
     crashlyticsWorkers.diskWrite.await();
 
     verify(mockSessionReportingCoordinator)
-        .persistNonFatalEvent(eq(nonFatal), eq(thread), eq(sessionId), anyLong());
+        .persistNonFatalEvent(eq(nonFatal), eq(thread), any(EventMetadata.class));
   }
 
   @SdkSuppress(minSdkVersion = 30) // ApplicationExitInfo
@@ -377,7 +379,7 @@ public void testLoggedExceptionsAfterCrashOk() {
         testSettingsProvider, Thread.currentThread(), new RuntimeException());
 
     // This should not throw.
-    controller.writeNonFatalException(Thread.currentThread(), new RuntimeException());
+    controller.writeNonFatalException(Thread.currentThread(), new RuntimeException(), Map.of());
   }
 
   /**

firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/common/CrashlyticsCoreTest.java

@@ -159,6 +159,143 @@ public void testCustomAttributes() throws Exception {
     assertEquals(longValue, metadata.getCustomKeys().get(key1));
   }
 
+  @Test
+  public void testCustomAttributes_retrievedWithEmptyEventKeys() throws Exception {
+    UserMetadata metadata = crashlyticsCore.getController().getUserMetadata();
+
+    assertTrue(metadata.getCustomKeys(Map.of()).isEmpty());
+
+    final String id = "id012345";
+    crashlyticsCore.setUserId(id);
+    crashlyticsWorkers.common.await();
+    assertEquals(id, metadata.getUserId());
+
+    final StringBuffer idBuffer = new StringBuffer(id);
+    while (idBuffer.length() < UserMetadata.MAX_ATTRIBUTE_SIZE) {
+      idBuffer.append("0");
+    }
+    final String longId = idBuffer.toString();
+    final String superLongId = longId + "more chars";
+
+    crashlyticsCore.setUserId(superLongId);
+    crashlyticsWorkers.common.await();
+    assertEquals(longId, metadata.getUserId());
+
+    final String key1 = "key1";
+    final String value1 = "value1";
+    crashlyticsCore.setCustomKey(key1, value1);
+    crashlyticsWorkers.common.await();
+    assertEquals(value1, metadata.getCustomKeys(Map.of()).get(key1));
+
+    // Adding an existing key with the same value should return false
+    assertFalse(metadata.setCustomKey(key1, value1));
+    assertTrue(metadata.setCustomKey(key1, "someOtherValue"));
+    assertTrue(metadata.setCustomKey(key1, value1));
+    assertFalse(metadata.setCustomKey(key1, value1));
+
+    final String longValue = longId.replaceAll("0", "x");
+    final String superLongValue = longValue + "some more chars";
+
+    // test truncation of custom keys and attributes
+    crashlyticsCore.setCustomKey(superLongId, superLongValue);
+    crashlyticsWorkers.common.await();
+    assertNull(metadata.getCustomKeys(Map.of()).get(superLongId));
+    assertEquals(longValue, metadata.getCustomKeys().get(longId));
+
+    // test the max number of attributes. We've already set 2.
+    for (int i = 2; i < UserMetadata.MAX_ATTRIBUTES; ++i) {
+      final String key = "key" + i;
+      final String value = "value" + i;
+      crashlyticsCore.setCustomKey(key, value);
+      crashlyticsWorkers.common.await();
+      assertEquals(value, metadata.getCustomKeys(Map.of()).get(key));
+    }
+    // should be full now, extra key, value pairs will be dropped.
+    final String key = "new key";
+    crashlyticsCore.setCustomKey(key, "some value");
+    crashlyticsWorkers.common.await();
+    assertFalse(metadata.getCustomKeys(Map.of()).containsKey(key));
+
+    // should be able to update existing keys
+    crashlyticsCore.setCustomKey(key1, longValue);
+    crashlyticsWorkers.common.await();
+    assertEquals(longValue, metadata.getCustomKeys(Map.of()).get(key1));
+
+    // when we set a key to null, it should still exist with an empty value
+    crashlyticsCore.setCustomKey(key1, null);
+    crashlyticsWorkers.common.await();
+    assertEquals("", metadata.getCustomKeys(Map.of()).get(key1));
+
+    // keys and values are trimmed.
+    crashlyticsCore.setCustomKey(" " + key1 + " ", " " + longValue + " ");
+    crashlyticsWorkers.common.await();
+    assertTrue(metadata.getCustomKeys(Map.of()).containsKey(key1));
+    assertEquals(longValue, metadata.getCustomKeys(Map.of()).get(key1));
+  }
+
+  @Test
+  public void testCustomKeysMergedWithEventKeys() throws Exception {
+    UserMetadata metadata = crashlyticsCore.getController().getUserMetadata();
+
+    Map<String, String> keysAndValues = new HashMap<>();
+    keysAndValues.put("customKey1", "value");
+    keysAndValues.put("customKey2", "value");
+    keysAndValues.put("customKey3", "value");
+
+    crashlyticsCore.setCustomKeys(keysAndValues);
+    crashlyticsWorkers.common.await();
+
+    Map<String, String> eventKeysAndValues = new HashMap<>();
+    eventKeysAndValues.put("eventKey1", "eventValue");
+    eventKeysAndValues.put("eventKey2", "eventValue");
+
+    // Tests reading custom keys with event keys.
+    assertEquals(keysAndValues.size(), metadata.getCustomKeys().size());
+    assertEquals(keysAndValues.size(), metadata.getCustomKeys(Map.of()).size());
+    assertEquals(
+        keysAndValues.size() + eventKeysAndValues.size(),
+        metadata.getCustomKeys(eventKeysAndValues).size());
+
+    // Tests event keys don't add to custom keys in future reads.
+    assertEquals(keysAndValues.size(), metadata.getCustomKeys().size());
+    assertEquals(keysAndValues.size(), metadata.getCustomKeys(Map.of()).size());
+
+    // Tests additional event keys.
+    eventKeysAndValues.put("eventKey3", "eventValue");
+    eventKeysAndValues.put("eventKey4", "eventValue");
+    assertEquals(
+        keysAndValues.size() + eventKeysAndValues.size(),
+        metadata.getCustomKeys(eventKeysAndValues).size());
+
+    // Tests overriding custom key with event keys.
+    keysAndValues.put("eventKey1", "value");
+    crashlyticsCore.setCustomKeys(keysAndValues);
+    crashlyticsWorkers.common.await();
+
+    assertEquals("value", metadata.getCustomKeys().get("eventKey1"));
+    assertEquals("value", metadata.getCustomKeys(Map.of()).get("eventKey1"));
+    assertEquals("eventValue", metadata.getCustomKeys(eventKeysAndValues).get("eventKey1"));
+
+    // Test the event key behavior when the count of custom keys is max.
+    for (int i = keysAndValues.size(); i < UserMetadata.MAX_ATTRIBUTES; ++i) {
+      final String key = "key" + i;
+      final String value = "value" + i;
+      crashlyticsCore.setCustomKey(key, value);
+      crashlyticsWorkers.common.await();
+      assertEquals(value, metadata.getCustomKeys().get(key));
+    }
+
+    assertEquals(UserMetadata.MAX_ATTRIBUTES, metadata.getCustomKeys().size());
+
+    // Tests event keys override global custom keys when the key exists.
+    assertEquals("value", metadata.getCustomKeys().get("eventKey1"));
+    assertEquals("value", metadata.getCustomKeys(Map.of()).get("eventKey1"));
+    assertEquals("eventValue", metadata.getCustomKeys(eventKeysAndValues).get("eventKey1"));
+
+    // Test when event keys *don't* override global custom keys.
+    assertNull(metadata.getCustomKeys(eventKeysAndValues).get("eventKey2"));
+  }
+
   @Test
   public void testBulkCustomKeys() throws Exception {
     final double DELTA = 1e-15;