Skip to content

Commit bd7b2df

Browse files
visumickeyvisumickey
authored
Skip custom attributes when the length of the key or the value is 0 (#3660)
1 parent 2131b84 commit bd7b2df

File tree

7 files changed

+106
-76
lines changed

7 files changed

+106
-76
lines changed

‎firebase-perf/CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ Refer [GMaven](https://maven.google.com/web/index.html?q=firebase-perf#com.googl
2323

2424
* {{fixed}} Fixed a bug where screen traces were not capturing frame metrics for multi-activity apps.
2525
* {{feature}} Added support for measuring screen performance metrics for "Activity Fragments" out-of-the-box.
26+
* {{fixed}} Excluded custom attributes whose key/value length is 0.
2627

2728
## Released
2829

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

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,8 @@
1414

1515
package com.google.firebase.perf;
1616

17+
import static com.google.firebase.perf.metrics.validator.PerfMetricValidator.*;
18+
1719
import android.content.Context;
1820
import android.content.pm.ApplicationInfo;
1921
import android.content.pm.PackageManager;
@@ -34,7 +36,6 @@
3436
import com.google.firebase.perf.logging.ConsoleUrlGenerator;
3537
import com.google.firebase.perf.metrics.HttpMetric;
3638
import com.google.firebase.perf.metrics.Trace;
37-
import com.google.firebase.perf.metrics.validator.PerfMetricValidator;
3839
import com.google.firebase.perf.session.SessionManager;
3940
import com.google.firebase.perf.transport.TransportManager;
4041
import com.google.firebase.perf.util.Constants;
@@ -44,7 +45,6 @@
4445
import java.lang.annotation.Retention;
4546
import java.lang.annotation.RetentionPolicy;
4647
import java.net.URL;
47-
import java.util.AbstractMap;
4848
import java.util.HashMap;
4949
import java.util.Locale;
5050
import java.util.Map;
@@ -364,10 +364,7 @@ private void checkAttribute(@Nullable String key, @Nullable String value) {
364364
Constants.MAX_TRACE_CUSTOM_ATTRIBUTES));
365365
}
366366

367-
String err = PerfMetricValidator.validateAttribute(new AbstractMap.SimpleEntry<>(key, value));
368-
if (err != null) {
369-
throw new IllegalArgumentException(err);
370-
}
367+
validateAttribute(key, value);
371368
}
372369

373370
/**

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

Lines changed: 6 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -14,18 +14,18 @@
1414

1515
package com.google.firebase.perf.metrics;
1616

17+
import static com.google.firebase.perf.metrics.validator.PerfMetricValidator.*;
18+
1719
import androidx.annotation.NonNull;
1820
import androidx.annotation.Nullable;
1921
import com.google.firebase.perf.FirebasePerformance.HttpMethod;
2022
import com.google.firebase.perf.FirebasePerformanceAttributable;
2123
import com.google.firebase.perf.config.ConfigResolver;
2224
import com.google.firebase.perf.logging.AndroidLogger;
23-
import com.google.firebase.perf.metrics.validator.PerfMetricValidator;
2425
import com.google.firebase.perf.transport.TransportManager;
2526
import com.google.firebase.perf.util.Constants;
2627
import com.google.firebase.perf.util.Timer;
2728
import java.net.URL;
28-
import java.util.AbstractMap;
2929
import java.util.HashMap;
3030
import java.util.Locale;
3131
import java.util.Map;
@@ -181,27 +181,21 @@ public void putAttribute(@NonNull String attribute, @NonNull String value) {
181181
}
182182
}
183183

184-
private void checkAttribute(@Nullable String attribute, @Nullable String value) {
184+
private void checkAttribute(@NonNull String key, @NonNull String value) {
185185
if (isStopped) {
186186
throw new IllegalArgumentException(
187187
"HttpMetric has been logged already so unable to modify attributes");
188188
}
189-
if (attribute == null || value == null) {
190-
throw new IllegalArgumentException("Attribute must not have null key or value.");
191-
}
192-
if (!customAttributesMap.containsKey(attribute)
189+
190+
if (!customAttributesMap.containsKey(key)
193191
&& customAttributesMap.size() >= Constants.MAX_TRACE_CUSTOM_ATTRIBUTES) {
194192
throw new IllegalArgumentException(
195193
String.format(
196194
Locale.ENGLISH,
197195
"Exceeds max limit of number of attributes - %d",
198196
Constants.MAX_TRACE_CUSTOM_ATTRIBUTES));
199197
}
200-
String err =
201-
PerfMetricValidator.validateAttribute(new AbstractMap.SimpleEntry<>(attribute, value));
202-
if (err != null) {
203-
throw new IllegalArgumentException(err);
204-
}
198+
validateAttribute(key, value);
205199
}
206200

207201
/**

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

Lines changed: 7 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,8 @@
1414

1515
package com.google.firebase.perf.metrics;
1616

17+
import static com.google.firebase.perf.metrics.validator.PerfMetricValidator.*;
18+
1719
import android.os.Parcel;
1820
import android.os.Parcelable;
1921
import androidx.annotation.Keep;
@@ -25,7 +27,6 @@
2527
import com.google.firebase.perf.application.AppStateUpdateHandler;
2628
import com.google.firebase.perf.config.ConfigResolver;
2729
import com.google.firebase.perf.logging.AndroidLogger;
28-
import com.google.firebase.perf.metrics.validator.PerfMetricValidator;
2930
import com.google.firebase.perf.session.PerfSession;
3031
import com.google.firebase.perf.session.SessionAwareObject;
3132
import com.google.firebase.perf.session.SessionManager;
@@ -35,7 +36,6 @@
3536
import com.google.firebase.perf.util.Constants;
3637
import com.google.firebase.perf.util.Timer;
3738
import java.lang.ref.WeakReference;
38-
import java.util.AbstractMap;
3939
import java.util.ArrayList;
4040
import java.util.Collections;
4141
import java.util.HashMap;
@@ -209,7 +209,7 @@ public void start() {
209209
return;
210210
}
211211

212-
String err = PerfMetricValidator.validateTraceName(name);
212+
String err = validateTraceName(name);
213213

214214
if (err != null) {
215215
logger.error("Cannot start trace '%s'. Trace name is invalid.(%s)", name, err);
@@ -326,7 +326,7 @@ private Counter obtainOrCreateCounterByName(@NonNull String counterName) {
326326
*/
327327
@Keep
328328
public void incrementMetric(@NonNull String metricName, long incrementBy) {
329-
String err = PerfMetricValidator.validateMetricName(metricName);
329+
String err = validateMetricName(metricName);
330330
if (err != null) {
331331
logger.error("Cannot increment metric '%s'. Metric name is invalid.(%s)", metricName, err);
332332
return;
@@ -382,7 +382,7 @@ public long getLongMetric(@NonNull String metricName) {
382382
*/
383383
@Keep
384384
public void putMetric(@NonNull String metricName, long value) {
385-
String err = PerfMetricValidator.validateMetricName(metricName);
385+
String err = validateMetricName(metricName);
386386
if (err != null) {
387387
logger.error(
388388
"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) {
630630
throw new IllegalArgumentException(
631631
String.format(Locale.ENGLISH, "Trace '%s' has been stopped", name));
632632
}
633+
633634
if (!customAttributesMap.containsKey(key)
634635
&& customAttributesMap.size() >= Constants.MAX_TRACE_CUSTOM_ATTRIBUTES) {
635636
throw new IllegalArgumentException(
@@ -638,10 +639,7 @@ private void checkAttribute(@NonNull String key, @NonNull String value) {
638639
"Exceeds max limit of number of attributes - %d",
639640
Constants.MAX_TRACE_CUSTOM_ATTRIBUTES));
640641
}
641-
String err = PerfMetricValidator.validateAttribute(new AbstractMap.SimpleEntry<>(key, value));
642-
if (err != null) {
643-
throw new IllegalArgumentException(err);
644-
}
642+
validateAttribute(key, value);
645643
}
646644

647645
/**

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

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,8 @@
1414

1515
package com.google.firebase.perf.metrics.validator;
1616

17+
import static com.google.firebase.perf.metrics.validator.PerfMetricValidator.*;
18+
1719
import androidx.annotation.NonNull;
1820
import androidx.annotation.Nullable;
1921
import com.google.firebase.perf.logging.AndroidLogger;
@@ -144,7 +146,7 @@ private boolean isValidTrace(@Nullable TraceMetric trace, int deep) {
144146
return false;
145147
}
146148
}
147-
if (!hasValidAttributes(trace.getCustomAttributesMap())) {
149+
if (!areAllAttributesValid(trace.getCustomAttributesMap())) {
148150
return false;
149151
}
150152
return true;
@@ -178,11 +180,12 @@ private boolean isValidCounterId(@Nullable String counterId) {
178180
return true;
179181
}
180182

181-
private boolean hasValidAttributes(Map<String, String> customAttributes) {
183+
private boolean areAllAttributesValid(Map<String, String> customAttributes) {
182184
for (Map.Entry<String, String> entry : customAttributes.entrySet()) {
183-
String err = PerfMetricValidator.validateAttribute(entry);
184-
if (err != null) {
185-
logger.warn(err);
185+
try {
186+
validateAttribute(entry.getKey(), entry.getValue());
187+
} catch (IllegalArgumentException exception) {
188+
logger.warn(exception.getLocalizedMessage());
186189
return false;
187190
}
188191
}

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

Lines changed: 31 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,6 @@
2323
import java.util.ArrayList;
2424
import java.util.List;
2525
import java.util.Locale;
26-
import java.util.Map;
2726

2827
/** An abstract class providing an interface to validate PerfMetric */
2928
public abstract class PerfMetricValidator {
@@ -135,33 +134,41 @@ public static String validateMetricName(@Nullable String str) {
135134
/**
136135
* Checks whether the given map entry fits key/value constraints for a Trace Attribute.
137136
*
138-
* @param attribute A key/value pair for an Attribute
137+
* @param key Key for the Attribute
138+
* @param value Value for the Attribute
139139
* @return null if the entry can be used as an Attribute, if not, an error string explaining why
140140
* it can't be used.
141141
*/
142-
@Nullable
143-
public static String validateAttribute(@NonNull Map.Entry<String, String> attribute) {
144-
String key = attribute.getKey();
145-
String value = attribute.getValue();
146-
if (key == null) {
147-
return "Attribute key must not be null";
148-
} else if (value == null) {
149-
return "Attribute value must not be null";
150-
} else if (key.length() > Constants.MAX_ATTRIBUTE_KEY_LENGTH) {
151-
return String.format(
152-
Locale.US,
153-
"Attribute key length must not exceed %d characters",
154-
Constants.MAX_ATTRIBUTE_KEY_LENGTH);
155-
} else if (value.length() > Constants.MAX_ATTRIBUTE_VALUE_LENGTH) {
156-
return String.format(
157-
Locale.US,
158-
"Attribute value length must not exceed %d characters",
159-
Constants.MAX_ATTRIBUTE_VALUE_LENGTH);
160-
} else if (!key.matches("^(?!(firebase_|google_|ga_))[A-Za-z][A-Za-z_0-9]*")) {
161-
return "Attribute key must start with letter, must only contain alphanumeric characters and"
162-
+ " underscore and must not start with \"firebase_\", \"google_\" and \"ga_";
142+
public static void validateAttribute(@NonNull String key, @NonNull String value) {
143+
if (key == null || key.length() == 0) {
144+
throw new IllegalArgumentException("Attribute key must not be null or empty");
145+
}
146+
147+
if (value == null || value.length() == 0) {
148+
throw new IllegalArgumentException("Attribute value must not be null or empty");
149+
}
150+
151+
if (key.length() > Constants.MAX_ATTRIBUTE_KEY_LENGTH) {
152+
throw new IllegalArgumentException(
153+
String.format(
154+
Locale.US,
155+
"Attribute key length must not exceed %d characters",
156+
Constants.MAX_ATTRIBUTE_KEY_LENGTH));
157+
}
158+
159+
if (value.length() > Constants.MAX_ATTRIBUTE_VALUE_LENGTH) {
160+
throw new IllegalArgumentException(
161+
String.format(
162+
Locale.US,
163+
"Attribute value length must not exceed %d characters",
164+
Constants.MAX_ATTRIBUTE_VALUE_LENGTH));
165+
}
166+
167+
if (!key.matches("^(?!(firebase_|google_|ga_))[A-Za-z][A-Za-z_0-9]*")) {
168+
throw new IllegalArgumentException(
169+
"Attribute key must start with letter, must only contain alphanumeric characters and"
170+
+ " underscore and must not start with \"firebase_\", \"google_\" and \"ga_");
163171
}
164-
return null;
165172
}
166173

167174
public abstract boolean isValidPerfMetric();

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

Lines changed: 50 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -169,47 +169,77 @@ public void screenTrace_shouldNotAllowNonPositiveTotalFrames() {
169169
}
170170

171171
@Test
172-
public void testInvalidCustomAttribute() {
172+
public void traceValidator_customAttributeWithUnderscorePrefix_marksPerfMetricInvalid() {
173173
TraceMetric.Builder trace = createValidTraceMetric().putCustomAttributes("_test", "value");
174174
assertThat(new FirebasePerfTraceValidator(trace.build()).isValidPerfMetric()).isFalse();
175+
}
175176

176-
trace = createValidTraceMetric();
177-
trace.clearCustomAttributes().putCustomAttributes("0_test", "value");
177+
@Test
178+
public void traceValidator_customAttributeWithNumberPrefix_marksPerfMetricInvalid() {
179+
TraceMetric.Builder trace = createValidTraceMetric().putCustomAttributes("0_test", "value");
178180
assertThat(new FirebasePerfTraceValidator(trace.build()).isValidPerfMetric()).isFalse();
181+
}
179182

180-
trace = trace.clone();
181-
trace.clearCustomAttributes().putCustomAttributes("google_test", "value");
183+
@Test
184+
public void traceValidator_customAttributeWithGooglePrefix_marksPerfMetricInvalid() {
185+
TraceMetric.Builder trace =
186+
createValidTraceMetric().putCustomAttributes("google_test", "value");
187+
assertThat(new FirebasePerfTraceValidator(trace.build()).isValidPerfMetric()).isFalse();
188+
}
189+
190+
@Test
191+
public void traceValidator_customAttributeWithFirebasePrefix_marksPerfMetricInvalid() {
192+
TraceMetric.Builder trace =
193+
createValidTraceMetric().putCustomAttributes("firebase_test", "value");
182194
assertThat(new FirebasePerfTraceValidator(trace.build()).isValidPerfMetric()).isFalse();
195+
}
183196

184-
trace = trace.clone();
185-
trace.clearCustomAttributes().putCustomAttributes("firebase_test", "value");
197+
@Test
198+
public void traceValidator_customAttributeWithGAPrefix_marksPerfMetricInvalid() {
199+
TraceMetric.Builder trace = createValidTraceMetric().putCustomAttributes("ga_test", "value");
186200
assertThat(new FirebasePerfTraceValidator(trace.build()).isValidPerfMetric()).isFalse();
201+
}
187202

188-
trace = trace.clone();
189-
trace.clearCustomAttributes().putCustomAttributes("ga_test", "value");
203+
@Test
204+
public void traceValidator_customAttributeEmptyValue_marksPerfMetricInvalid() {
205+
TraceMetric.Builder trace = createValidTraceMetric().putCustomAttributes("key", "");
190206
assertThat(new FirebasePerfTraceValidator(trace.build()).isValidPerfMetric()).isFalse();
207+
}
208+
209+
@Test
210+
public void traceValidator_customAttributeEmptyKey_marksPerfMetricInvalid() {
211+
TraceMetric.Builder trace = createValidTraceMetric().putCustomAttributes("", "value");
212+
assertThat(new FirebasePerfTraceValidator(trace.build()).isValidPerfMetric()).isFalse();
213+
}
214+
215+
@Test
216+
public void traceValidator_customAttributeEmptyKeyAndValue_marksPerfMetricInvalid() {
217+
TraceMetric.Builder trace = createValidTraceMetric().putCustomAttributes("", "");
218+
assertThat(new FirebasePerfTraceValidator(trace.build()).isValidPerfMetric()).isFalse();
219+
}
220+
221+
@Test
222+
public void traceValidator_customAttributeWithLongKey_marksPerfMetricInvalid() {
191223

192224
StringBuilder longString = new StringBuilder();
193225
for (int i = 0; i <= Constants.MAX_ATTRIBUTE_KEY_LENGTH; i++) {
194226
longString.append("a");
195227
}
196-
197-
trace = trace.clone();
198-
trace.clearCustomAttributes().putCustomAttributes(longString.toString(), "value");
228+
TraceMetric.Builder trace =
229+
createValidTraceMetric().putCustomAttributes(longString.toString(), "value");
199230
assertThat(new FirebasePerfTraceValidator(trace.build()).isValidPerfMetric()).isFalse();
231+
}
232+
233+
@Test
234+
public void traceValidator_customAttributeWithLongValue_marksPerfMetricInvalid() {
200235

201-
longString = new StringBuilder();
236+
StringBuilder longString = new StringBuilder();
202237
for (int i = 0; i <= Constants.MAX_ATTRIBUTE_VALUE_LENGTH; i++) {
203238
longString.append("a");
204239
}
205-
206-
trace = trace.clone();
207-
trace.clearCustomAttributes().putCustomAttributes("key", longString.toString());
240+
TraceMetric.Builder trace =
241+
createValidTraceMetric().putCustomAttributes("key", longString.toString());
208242
assertThat(new FirebasePerfTraceValidator(trace.build()).isValidPerfMetric()).isFalse();
209-
210-
trace = trace.clone();
211-
trace.clearCustomAttributes().putCustomAttributes("test", "value");
212-
assertThat(new FirebasePerfTraceValidator(trace.build()).isValidPerfMetric()).isTrue();
213243
}
214244

215245
@Test

0 commit comments

Comments
 (0)