Skip custom attributes when the length of the key or the value is 0

firebase-perf/CHANGELOG.md

@@ -23,6 +23,7 @@ Refer [GMaven](https://maven.google.com/web/index.html?q=firebase-perf#com.googl
 
 *   {{fixed}} Fixed a bug where screen traces were not capturing frame metrics for multi-activity apps.
 *   {{feature}} Added support for measuring screen performance metrics for "Activity Fragments" out-of-the-box.
+*   {{fixed}} Excluded custom attributes whose key/value length is 0.
 
 ## Released
 

firebase-perf/src/main/java/com/google/firebase/perf/FirebasePerformance.java

@@ -14,6 +14,8 @@
 
 package com.google.firebase.perf;
 
+import static com.google.firebase.perf.metrics.validator.PerfMetricValidator.*;
+
 import android.content.Context;
 import android.content.pm.ApplicationInfo;
 import android.content.pm.PackageManager;
@@ -34,7 +36,6 @@
 import com.google.firebase.perf.logging.ConsoleUrlGenerator;
 import com.google.firebase.perf.metrics.HttpMetric;
 import com.google.firebase.perf.metrics.Trace;
-import com.google.firebase.perf.metrics.validator.PerfMetricValidator;
 import com.google.firebase.perf.session.SessionManager;
 import com.google.firebase.perf.transport.TransportManager;
 import com.google.firebase.perf.util.Constants;
@@ -44,7 +45,6 @@
 import java.lang.annotation.Retention;
 import java.lang.annotation.RetentionPolicy;
 import java.net.URL;
-import java.util.AbstractMap;
 import java.util.HashMap;
 import java.util.Locale;
 import java.util.Map;
@@ -364,10 +364,7 @@ private void checkAttribute(@Nullable String key, @Nullable String value) {
               Constants.MAX_TRACE_CUSTOM_ATTRIBUTES));
     }
 
-    String err = PerfMetricValidator.validateAttribute(new AbstractMap.SimpleEntry<>(key, value));
-    if (err != null) {
-      throw new IllegalArgumentException(err);
-    }
+    validateAttribute(key, value);
   }
 
   /**

firebase-perf/src/main/java/com/google/firebase/perf/metrics/HttpMetric.java

@@ -14,18 +14,18 @@
 
 package com.google.firebase.perf.metrics;
 
+import static com.google.firebase.perf.metrics.validator.PerfMetricValidator.*;
+
 import androidx.annotation.NonNull;
 import androidx.annotation.Nullable;
 import com.google.firebase.perf.FirebasePerformance.HttpMethod;
 import com.google.firebase.perf.FirebasePerformanceAttributable;
 import com.google.firebase.perf.config.ConfigResolver;
 import com.google.firebase.perf.logging.AndroidLogger;
-import com.google.firebase.perf.metrics.validator.PerfMetricValidator;
 import com.google.firebase.perf.transport.TransportManager;
 import com.google.firebase.perf.util.Constants;
 import com.google.firebase.perf.util.Timer;
 import java.net.URL;
-import java.util.AbstractMap;
 import java.util.HashMap;
 import java.util.Locale;
 import java.util.Map;
@@ -181,27 +181,21 @@ public void putAttribute(@NonNull String attribute, @NonNull String value) {
     }
   }
 
-  private void checkAttribute(@Nullable String attribute, @Nullable String value) {
+  private void checkAttribute(@NonNull String key, @NonNull String value) {
     if (isStopped) {
       throw new IllegalArgumentException(
           "HttpMetric has been logged already so unable to modify attributes");
     }
-    if (attribute == null || value == null) {
-      throw new IllegalArgumentException("Attribute must not have null key or value.");
-    }
-    if (!customAttributesMap.containsKey(attribute)
+
+    if (!customAttributesMap.containsKey(key)
         && customAttributesMap.size() >= Constants.MAX_TRACE_CUSTOM_ATTRIBUTES) {
       throw new IllegalArgumentException(
           String.format(
               Locale.ENGLISH,
               "Exceeds max limit of number of attributes - %d",
               Constants.MAX_TRACE_CUSTOM_ATTRIBUTES));
     }
-    String err =
-        PerfMetricValidator.validateAttribute(new AbstractMap.SimpleEntry<>(attribute, value));
-    if (err != null) {
-      throw new IllegalArgumentException(err);
-    }
+    validateAttribute(key, value);
   }
 
   /**

firebase-perf/src/main/java/com/google/firebase/perf/metrics/Trace.java

@@ -14,6 +14,8 @@
 
 package com.google.firebase.perf.metrics;
 
+import static com.google.firebase.perf.metrics.validator.PerfMetricValidator.*;
+
 import android.os.Parcel;
 import android.os.Parcelable;
 import androidx.annotation.Keep;
@@ -25,7 +27,6 @@
 import com.google.firebase.perf.application.AppStateUpdateHandler;
 import com.google.firebase.perf.config.ConfigResolver;
 import com.google.firebase.perf.logging.AndroidLogger;
-import com.google.firebase.perf.metrics.validator.PerfMetricValidator;
 import com.google.firebase.perf.session.PerfSession;
 import com.google.firebase.perf.session.SessionAwareObject;
 import com.google.firebase.perf.session.SessionManager;
@@ -35,7 +36,6 @@
 import com.google.firebase.perf.util.Constants;
 import com.google.firebase.perf.util.Timer;
 import java.lang.ref.WeakReference;
-import java.util.AbstractMap;
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.HashMap;
@@ -209,7 +209,7 @@ public void start() {
       return;
     }
 
-    String err = PerfMetricValidator.validateTraceName(name);
+    String err = validateTraceName(name);
 
     if (err != null) {
       logger.error("Cannot start trace '%s'. Trace name is invalid.(%s)", name, err);
@@ -326,7 +326,7 @@ private Counter obtainOrCreateCounterByName(@NonNull String counterName) {
    */
   @Keep
   public void incrementMetric(@NonNull String metricName, long incrementBy) {
-    String err = PerfMetricValidator.validateMetricName(metricName);
+    String err = validateMetricName(metricName);
     if (err != null) {
       logger.error("Cannot increment metric '%s'. Metric name is invalid.(%s)", metricName, err);
       return;
@@ -382,7 +382,7 @@ public long getLongMetric(@NonNull String metricName) {
    */
   @Keep
   public void putMetric(@NonNull String metricName, long value) {
-    String err = PerfMetricValidator.validateMetricName(metricName);
+    String err = validateMetricName(metricName);
     if (err != null) {
       logger.error(
           "Cannot set value for metric '%s'. Metric name is invalid.(%s)", metricName, err);
@@ -630,6 +630,7 @@ private void checkAttribute(@NonNull String key, @NonNull String value) {
       throw new IllegalArgumentException(
           String.format(Locale.ENGLISH, "Trace '%s' has been stopped", name));
     }
+
     if (!customAttributesMap.containsKey(key)
         && customAttributesMap.size() >= Constants.MAX_TRACE_CUSTOM_ATTRIBUTES) {
       throw new IllegalArgumentException(
@@ -638,10 +639,7 @@ private void checkAttribute(@NonNull String key, @NonNull String value) {
               "Exceeds max limit of number of attributes - %d",
               Constants.MAX_TRACE_CUSTOM_ATTRIBUTES));
     }
-    String err = PerfMetricValidator.validateAttribute(new AbstractMap.SimpleEntry<>(key, value));
-    if (err != null) {
-      throw new IllegalArgumentException(err);
-    }
+    validateAttribute(key, value);
   }
 
   /**

firebase-perf/src/main/java/com/google/firebase/perf/metrics/validator/FirebasePerfTraceValidator.java

@@ -14,6 +14,8 @@
 
 package com.google.firebase.perf.metrics.validator;
 
+import static com.google.firebase.perf.metrics.validator.PerfMetricValidator.*;
+
 import androidx.annotation.NonNull;
 import androidx.annotation.Nullable;
 import com.google.firebase.perf.logging.AndroidLogger;
@@ -144,7 +146,7 @@ private boolean isValidTrace(@Nullable TraceMetric trace, int deep) {
         return false;
       }
     }
-    if (!hasValidAttributes(trace.getCustomAttributesMap())) {
+    if (!areAllAttributesValid(trace.getCustomAttributesMap())) {
       return false;
     }
     return true;
@@ -178,11 +180,12 @@ private boolean isValidCounterId(@Nullable String counterId) {
     return true;
   }
 
-  private boolean hasValidAttributes(Map<String, String> customAttributes) {
+  private boolean areAllAttributesValid(Map<String, String> customAttributes) {
     for (Map.Entry<String, String> entry : customAttributes.entrySet()) {
-      String err = PerfMetricValidator.validateAttribute(entry);
-      if (err != null) {
-        logger.warn(err);
+      try {
+        validateAttribute(entry.getKey(), entry.getValue());
+      } catch (IllegalArgumentException exception) {
+        logger.warn(exception.getLocalizedMessage());
         return false;
       }
     }

firebase-perf/src/main/java/com/google/firebase/perf/metrics/validator/PerfMetricValidator.java

@@ -23,7 +23,6 @@
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Locale;
-import java.util.Map;
 
 /** An abstract class providing an interface to validate PerfMetric */
 public abstract class PerfMetricValidator {
@@ -135,33 +134,41 @@ public static String validateMetricName(@Nullable String str) {
   /**
    * Checks whether the given map entry fits key/value constraints for a Trace Attribute.
    *
-   * @param attribute A key/value pair for an Attribute
+   * @param key Key for the Attribute
+   * @param value Value for the Attribute
    * @return null if the entry can be used as an Attribute, if not, an error string explaining why
    *     it can't be used.
    */
-  @Nullable
-  public static String validateAttribute(@NonNull Map.Entry<String, String> attribute) {
-    String key = attribute.getKey();
-    String value = attribute.getValue();
-    if (key == null) {
-      return "Attribute key must not be null";
-    } else if (value == null) {
-      return "Attribute value must not be null";
-    } else if (key.length() > Constants.MAX_ATTRIBUTE_KEY_LENGTH) {
-      return String.format(
-          Locale.US,
-          "Attribute key length must not exceed %d characters",
-          Constants.MAX_ATTRIBUTE_KEY_LENGTH);
-    } else if (value.length() > Constants.MAX_ATTRIBUTE_VALUE_LENGTH) {
-      return String.format(
-          Locale.US,
-          "Attribute value length must not exceed %d characters",
-          Constants.MAX_ATTRIBUTE_VALUE_LENGTH);
-    } else if (!key.matches("^(?!(firebase_|google_|ga_))[A-Za-z][A-Za-z_0-9]*")) {
-      return "Attribute key must start with letter, must only contain alphanumeric characters and"
-          + " underscore and must not start with \"firebase_\", \"google_\" and \"ga_";
+  public static void validateAttribute(@NonNull String key, @NonNull String value) {
+    if (key == null || key.length() == 0) {
+      throw new IllegalArgumentException("Attribute key must not be null or empty");
+    }
+
+    if (value == null || value.length() == 0) {
+      throw new IllegalArgumentException("Attribute value must not be null or empty");
+    }
+
+    if (key.length() > Constants.MAX_ATTRIBUTE_KEY_LENGTH) {
+      throw new IllegalArgumentException(
+          String.format(
+              Locale.US,
+              "Attribute key length must not exceed %d characters",
+              Constants.MAX_ATTRIBUTE_KEY_LENGTH));
+    }
+
+    if (value.length() > Constants.MAX_ATTRIBUTE_VALUE_LENGTH) {
+      throw new IllegalArgumentException(
+          String.format(
+              Locale.US,
+              "Attribute value length must not exceed %d characters",
+              Constants.MAX_ATTRIBUTE_VALUE_LENGTH));
+    }
+
+    if (!key.matches("^(?!(firebase_|google_|ga_))[A-Za-z][A-Za-z_0-9]*")) {
+      throw new IllegalArgumentException(
+          "Attribute key must start with letter, must only contain alphanumeric characters and"
+              + " underscore and must not start with \"firebase_\", \"google_\" and \"ga_");
     }
-    return null;
   }
 
   public abstract boolean isValidPerfMetric();

firebase-perf/src/test/java/com/google/firebase/perf/metrics/validator/FirebasePerfTraceValidatorTest.java

@@ -169,47 +169,77 @@ public void screenTrace_shouldNotAllowNonPositiveTotalFrames() {
   }
 
   @Test
-  public void testInvalidCustomAttribute() {
+  public void traceValidator_customAttributeWithUnderscorePrefix_marksPerfMetricInvalid() {
     TraceMetric.Builder trace = createValidTraceMetric().putCustomAttributes("_test", "value");
     assertThat(new FirebasePerfTraceValidator(trace.build()).isValidPerfMetric()).isFalse();
+  }
 
-    trace = createValidTraceMetric();
-    trace.clearCustomAttributes().putCustomAttributes("0_test", "value");
+  @Test
+  public void traceValidator_customAttributeWithNumberPrefix_marksPerfMetricInvalid() {
+    TraceMetric.Builder trace = createValidTraceMetric().putCustomAttributes("0_test", "value");
     assertThat(new FirebasePerfTraceValidator(trace.build()).isValidPerfMetric()).isFalse();
+  }
 
-    trace = trace.clone();
-    trace.clearCustomAttributes().putCustomAttributes("google_test", "value");
+  @Test
+  public void traceValidator_customAttributeWithGooglePrefix_marksPerfMetricInvalid() {
+    TraceMetric.Builder trace =
+        createValidTraceMetric().putCustomAttributes("google_test", "value");
+    assertThat(new FirebasePerfTraceValidator(trace.build()).isValidPerfMetric()).isFalse();
+  }
+
+  @Test
+  public void traceValidator_customAttributeWithFirebasePrefix_marksPerfMetricInvalid() {
+    TraceMetric.Builder trace =
+        createValidTraceMetric().putCustomAttributes("firebase_test", "value");
     assertThat(new FirebasePerfTraceValidator(trace.build()).isValidPerfMetric()).isFalse();
+  }
 
-    trace = trace.clone();
-    trace.clearCustomAttributes().putCustomAttributes("firebase_test", "value");
+  @Test
+  public void traceValidator_customAttributeWithGAPrefix_marksPerfMetricInvalid() {
+    TraceMetric.Builder trace = createValidTraceMetric().putCustomAttributes("ga_test", "value");
     assertThat(new FirebasePerfTraceValidator(trace.build()).isValidPerfMetric()).isFalse();
+  }
 
-    trace = trace.clone();
-    trace.clearCustomAttributes().putCustomAttributes("ga_test", "value");
+  @Test
+  public void traceValidator_customAttributeEmptyValue_marksPerfMetricInvalid() {
+    TraceMetric.Builder trace = createValidTraceMetric().putCustomAttributes("key", "");
     assertThat(new FirebasePerfTraceValidator(trace.build()).isValidPerfMetric()).isFalse();
+  }
+
+  @Test
+  public void traceValidator_customAttributeEmptyKey_marksPerfMetricInvalid() {
+    TraceMetric.Builder trace = createValidTraceMetric().putCustomAttributes("", "value");
+    assertThat(new FirebasePerfTraceValidator(trace.build()).isValidPerfMetric()).isFalse();
+  }
+
+  @Test
+  public void traceValidator_customAttributeEmptyKeyAndValue_marksPerfMetricInvalid() {
+    TraceMetric.Builder trace = createValidTraceMetric().putCustomAttributes("", "");
+    assertThat(new FirebasePerfTraceValidator(trace.build()).isValidPerfMetric()).isFalse();
+  }
+
+  @Test
+  public void traceValidator_customAttributeWithLongKey_marksPerfMetricInvalid() {
 
     StringBuilder longString = new StringBuilder();
     for (int i = 0; i <= Constants.MAX_ATTRIBUTE_KEY_LENGTH; i++) {
       longString.append("a");
     }
-
-    trace = trace.clone();
-    trace.clearCustomAttributes().putCustomAttributes(longString.toString(), "value");
+    TraceMetric.Builder trace =
+        createValidTraceMetric().putCustomAttributes(longString.toString(), "value");
     assertThat(new FirebasePerfTraceValidator(trace.build()).isValidPerfMetric()).isFalse();
+  }
+
+  @Test
+  public void traceValidator_customAttributeWithLongValue_marksPerfMetricInvalid() {
 
-    longString = new StringBuilder();
+    StringBuilder longString = new StringBuilder();
     for (int i = 0; i <= Constants.MAX_ATTRIBUTE_VALUE_LENGTH; i++) {
       longString.append("a");
     }
-
-    trace = trace.clone();
-    trace.clearCustomAttributes().putCustomAttributes("key", longString.toString());
+    TraceMetric.Builder trace =
+        createValidTraceMetric().putCustomAttributes("key", longString.toString());
     assertThat(new FirebasePerfTraceValidator(trace.build()).isValidPerfMetric()).isFalse();
-
-    trace = trace.clone();
-    trace.clearCustomAttributes().putCustomAttributes("test", "value");
-    assertThat(new FirebasePerfTraceValidator(trace.build()).isValidPerfMetric()).isTrue();
   }
 
   @Test