Skip to content

Commit fe64756

Browse files
vkryachkovkryachko
authored
Fix metrics counter reporting. (#3128)
* Fix metrics counter reporting. The issue was that we were never resetting counters after a successful metrics upload. This resulted in ever increasing counter values that did not represent reality. * Add license
1 parent 8068a9b commit fe64756

File tree

4 files changed

+270
-15
lines changed

4 files changed

+270
-15
lines changed

‎transport/transport-runtime/src/androidTest/java/com/google/android/datatransport/runtime/TestDestination.java

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
// Copyright 2021 Google LLC
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
package com.google.android.datatransport.runtime;
16+
17+
import androidx.annotation.NonNull;
18+
import androidx.annotation.Nullable;
19+
20+
public class TestDestination implements Destination {
21+
private final String name;
22+
private final byte[] extras;
23+
24+
public TestDestination(String name, byte[] extras) {
25+
this.name = name;
26+
this.extras = extras;
27+
}
28+
29+
@NonNull
30+
@Override
31+
public String getName() {
32+
return name;
33+
}
34+
35+
@Nullable
36+
@Override
37+
public byte[] getExtras() {
38+
return extras;
39+
}
40+
}

‎transport/transport-runtime/src/androidTest/java/com/google/android/datatransport/runtime/UploaderIntegrationTest.java

Lines changed: 204 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -36,13 +36,19 @@
3636
import com.google.android.datatransport.runtime.backends.BackendRequest;
3737
import com.google.android.datatransport.runtime.backends.BackendResponse;
3838
import com.google.android.datatransport.runtime.backends.TransportBackend;
39+
import com.google.android.datatransport.runtime.firebase.transport.LogEventDropped;
40+
import com.google.android.datatransport.runtime.firebase.transport.LogSourceMetrics;
3941
import com.google.android.datatransport.runtime.scheduling.jobscheduling.WorkScheduler;
4042
import com.google.android.datatransport.runtime.scheduling.persistence.EventStore;
4143
import com.google.android.datatransport.runtime.scheduling.persistence.PersistedEvent;
44+
import com.google.android.datatransport.runtime.scheduling.persistence.SQLiteEventStore;
4245
import com.google.android.datatransport.runtime.synchronization.SynchronizationException;
4346
import java.nio.charset.Charset;
47+
import java.util.Arrays;
4448
import java.util.Collections;
49+
import java.util.List;
4550
import java.util.UUID;
51+
import org.junit.After;
4652
import org.junit.Before;
4753
import org.junit.Rule;
4854
import org.junit.Test;
@@ -77,6 +83,12 @@ public void setUp() {
7783
invocation -> invocation.<EventInternal>getArgument(0).toBuilder().build());
7884
}
7985

86+
@After
87+
public void tearDown() {
88+
component.getEventStore().clearDb();
89+
component.getEventStore().resetClientMetrics();
90+
}
91+
8092
private String generateBackendName() {
8193
return UUID.randomUUID().toString().replace("-", "");
8294
}
@@ -201,6 +213,61 @@ public void uploader_dbException_shouldReschedule() {
201213
Transport<String> transport =
202214
factory.getTransport(testTransport, String.class, String::getBytes);
203215
Event<String> stringEvent = Event.ofTelemetry("TelemetryData");
216+
217+
transport.send(stringEvent);
218+
verify(spyScheduler, times(1)).schedule(any(), eq(2));
219+
assertThat(store.getNextCallTime(transportContext)).isEqualTo(0);
220+
}
221+
222+
@Test
223+
public void uploader_invalidPayload_shouldNotReschedule() {
224+
TransportRuntime runtime = TransportRuntime.getInstance();
225+
SQLiteEventStore store = component.getEventStore();
226+
String mockBackendName = generateBackendName();
227+
byte[] mockExtras = "extras".getBytes(Charset.defaultCharset());
228+
229+
when(mockRegistry.get(mockBackendName)).thenReturn(mockBackend);
230+
when(mockBackend.send(any())).thenReturn(BackendResponse.invalidPayload());
231+
232+
Transport<String> transport =
233+
runtime
234+
.newFactory(new TestDestination(mockBackendName, mockExtras))
235+
.getTransport(testTransport, String.class, String::getBytes);
236+
transport.send(Event.ofTelemetry("TelemetryData"));
237+
238+
verify(spyScheduler, times(0)).schedule(any(), eq(2));
239+
240+
List<LogSourceMetrics> logSourceMetrics = store.loadClientMetrics().getLogSourceMetricsList();
241+
assertThat(logSourceMetrics).hasSize(1);
242+
LogSourceMetrics metrics = logSourceMetrics.get(0);
243+
List<LogEventDropped> logEventDroppedList = metrics.getLogEventDroppedList();
244+
assertThat(logEventDroppedList).hasSize(1);
245+
LogEventDropped logEventDropped = logEventDroppedList.get(0);
246+
assertThat(logEventDropped.getEventsDroppedCount()).isEqualTo(1);
247+
assertThat(logEventDropped.getReason()).isEqualTo(LogEventDropped.Reason.INVALID_PAYLOD);
248+
}
249+
250+
@Test
251+
public void uploader_withPendingClientMetricsAndSuccessfulUpload_shouldResetCounters() {
252+
TransportRuntime runtime = TransportRuntime.getInstance();
253+
SQLiteEventStore store = component.getEventStore();
254+
String anotherTransport = "anotherTransport";
255+
store.recordLogEventDropped(20, LogEventDropped.Reason.CACHE_FULL, testTransport);
256+
store.recordLogEventDropped(1, LogEventDropped.Reason.MAX_RETRIES_REACHED, testTransport);
257+
store.recordLogEventDropped(1, LogEventDropped.Reason.INVALID_PAYLOD, anotherTransport);
258+
259+
assertThat(store.loadClientMetrics().getLogSourceMetricsList()).hasSize(2);
260+
261+
String mockBackendName = generateBackendName();
262+
byte[] mockExtras = "extras".getBytes(Charset.defaultCharset());
263+
264+
when(mockRegistry.get(mockBackendName)).thenReturn(mockBackend);
265+
when(mockBackend.send(any())).thenReturn(BackendResponse.ok(1));
266+
267+
Transport<String> transport =
268+
runtime
269+
.newFactory(new TestDestination(mockBackendName, mockExtras))
270+
.getTransport(testTransport, String.class, String::getBytes);
204271
EventInternal expectedEvent =
205272
EventInternal.builder()
206273
.setEventMillis(3)
@@ -210,8 +277,142 @@ public void uploader_dbException_shouldReschedule() {
210277
new EncodedPayload(
211278
PROTOBUF_ENCODING, "TelemetryData".getBytes(Charset.defaultCharset())))
212279
.build();
213-
transport.send(stringEvent);
214-
verify(spyScheduler, times(1)).schedule(any(), eq(2));
215-
assertThat(store.getNextCallTime(transportContext)).isEqualTo(0);
280+
EventInternal metricsEvent = component.getUploader().createMetricsEvent(mockBackend);
281+
transport.send(Event.ofTelemetry("TelemetryData"));
282+
verify(mockBackend, times(1))
283+
.send(
284+
eq(
285+
BackendRequest.builder()
286+
.setEvents(Arrays.asList(expectedEvent, metricsEvent))
287+
.setExtras(mockExtras)
288+
.build()));
289+
290+
verify(spyScheduler, times(0)).schedule(any(), eq(2));
291+
292+
assertThat(store.loadClientMetrics().getLogSourceMetricsList()).isEmpty();
293+
}
294+
295+
@Test
296+
public void uploader_withPendingClientMetricsAndInvalidPayload_shouldNotResetCounters() {
297+
TransportRuntime runtime = TransportRuntime.getInstance();
298+
SQLiteEventStore store = component.getEventStore();
299+
String anotherTransport = "anotherTransport";
300+
store.recordLogEventDropped(1, LogEventDropped.Reason.INVALID_PAYLOD, anotherTransport);
301+
302+
assertThat(store.loadClientMetrics().getLogSourceMetricsList()).hasSize(1);
303+
304+
String mockBackendName = generateBackendName();
305+
byte[] mockExtras = "extras".getBytes(Charset.defaultCharset());
306+
307+
when(mockRegistry.get(mockBackendName)).thenReturn(mockBackend);
308+
when(mockBackend.send(any())).thenReturn(BackendResponse.invalidPayload());
309+
310+
Transport<String> transport =
311+
runtime
312+
.newFactory(new TestDestination(mockBackendName, mockExtras))
313+
.getTransport(testTransport, String.class, String::getBytes);
314+
EventInternal expectedEvent =
315+
EventInternal.builder()
316+
.setEventMillis(3)
317+
.setUptimeMillis(1)
318+
.setTransportName(testTransport)
319+
.setEncodedPayload(
320+
new EncodedPayload(
321+
PROTOBUF_ENCODING, "TelemetryData".getBytes(Charset.defaultCharset())))
322+
.build();
323+
EventInternal metricsEvent = component.getUploader().createMetricsEvent(mockBackend);
324+
transport.send(Event.ofTelemetry("TelemetryData"));
325+
verify(mockBackend, times(1))
326+
.send(
327+
eq(
328+
BackendRequest.builder()
329+
.setEvents(Arrays.asList(expectedEvent, metricsEvent))
330+
.setExtras(mockExtras)
331+
.build()));
332+
333+
verify(spyScheduler, times(0)).schedule(any(), eq(2));
334+
335+
assertThat(store.loadClientMetrics().getLogSourceMetricsList()).hasSize(2);
336+
}
337+
338+
@Test
339+
public void uploader_withPendingClientMetricsAndFatalError_shouldNotResetCounters() {
340+
TransportRuntime runtime = TransportRuntime.getInstance();
341+
SQLiteEventStore store = component.getEventStore();
342+
String anotherTransport = "anotherTransport";
343+
store.recordLogEventDropped(1, LogEventDropped.Reason.INVALID_PAYLOD, anotherTransport);
344+
345+
assertThat(store.loadClientMetrics().getLogSourceMetricsList()).hasSize(1);
346+
347+
String mockBackendName = generateBackendName();
348+
byte[] mockExtras = "extras".getBytes(Charset.defaultCharset());
349+
350+
when(mockRegistry.get(mockBackendName)).thenReturn(mockBackend);
351+
when(mockBackend.send(any())).thenReturn(BackendResponse.fatalError());
352+
353+
Transport<String> transport =
354+
runtime
355+
.newFactory(new TestDestination(mockBackendName, mockExtras))
356+
.getTransport(testTransport, String.class, String::getBytes);
357+
EventInternal expectedEvent =
358+
EventInternal.builder()
359+
.setEventMillis(3)
360+
.setUptimeMillis(1)
361+
.setTransportName(testTransport)
362+
.setEncodedPayload(
363+
new EncodedPayload(
364+
PROTOBUF_ENCODING, "TelemetryData".getBytes(Charset.defaultCharset())))
365+
.build();
366+
EventInternal metricsEvent = component.getUploader().createMetricsEvent(mockBackend);
367+
transport.send(Event.ofTelemetry("TelemetryData"));
368+
verify(mockBackend, times(1))
369+
.send(
370+
eq(
371+
BackendRequest.builder()
372+
.setEvents(Arrays.asList(expectedEvent, metricsEvent))
373+
.setExtras(mockExtras)
374+
.build()));
375+
376+
verify(spyScheduler, times(0)).schedule(any(), eq(2));
377+
378+
assertThat(store.loadClientMetrics().getLogSourceMetricsList()).hasSize(1);
379+
}
380+
381+
@Test
382+
public void
383+
uploader_withPendingClientMetricsAndSuccessfulUploadButNotLegacyTarget_shouldNotResetCounters() {
384+
TransportRuntime runtime = TransportRuntime.getInstance();
385+
SQLiteEventStore store = component.getEventStore();
386+
String anotherTransport = "anotherTransport";
387+
store.recordLogEventDropped(1, LogEventDropped.Reason.INVALID_PAYLOD, anotherTransport);
388+
389+
assertThat(store.loadClientMetrics().getLogSourceMetricsList()).hasSize(1);
390+
391+
String mockBackendName = generateBackendName();
392+
393+
when(mockRegistry.get(mockBackendName)).thenReturn(mockBackend);
394+
when(mockBackend.send(any())).thenReturn(BackendResponse.ok(1));
395+
396+
Transport<String> transport =
397+
runtime
398+
.newFactory(new TestDestination(mockBackendName, null))
399+
.getTransport(testTransport, String.class, String::getBytes);
400+
EventInternal expectedEvent =
401+
EventInternal.builder()
402+
.setEventMillis(3)
403+
.setUptimeMillis(1)
404+
.setTransportName(testTransport)
405+
.setEncodedPayload(
406+
new EncodedPayload(
407+
PROTOBUF_ENCODING, "TelemetryData".getBytes(Charset.defaultCharset())))
408+
.build();
409+
410+
transport.send(Event.ofTelemetry("TelemetryData"));
411+
verify(mockBackend, times(1))
412+
.send(eq(BackendRequest.create(Collections.singletonList(expectedEvent))));
413+
414+
verify(spyScheduler, times(0)).schedule(any(), eq(2));
415+
416+
assertThat(store.loadClientMetrics().getLogSourceMetricsList()).hasSize(1);
216417
}
217418
}

‎transport/transport-runtime/src/androidTest/java/com/google/android/datatransport/runtime/UploaderTestRuntimeComponent.java

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616

1717
import android.content.Context;
1818
import com.google.android.datatransport.runtime.backends.BackendRegistry;
19+
import com.google.android.datatransport.runtime.scheduling.jobscheduling.Uploader;
1920
import com.google.android.datatransport.runtime.scheduling.jobscheduling.WorkScheduler;
2021
import com.google.android.datatransport.runtime.scheduling.persistence.SQLiteEventStore;
2122
import com.google.android.datatransport.runtime.scheduling.persistence.SpyEventStoreModule;
@@ -44,6 +45,8 @@ abstract class UploaderTestRuntimeComponent extends TransportRuntimeComponent {
4445

4546
abstract WorkScheduler getWorkScheduler();
4647

48+
abstract Uploader getUploader();
49+
4750
@Override
4851
public void close() throws IOException {
4952
getEventStore().clearDb();

‎transport/transport-runtime/src/main/java/com/google/android/datatransport/runtime/scheduling/jobscheduling/Uploader.java

Lines changed: 23 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
import android.content.Context;
1818
import android.net.ConnectivityManager;
1919
import android.net.NetworkInfo;
20+
import androidx.annotation.VisibleForTesting;
2021
import com.google.android.datatransport.Encoding;
2122
import com.google.android.datatransport.runtime.EncodedPayload;
2223
import com.google.android.datatransport.runtime.EventInternal;
@@ -136,18 +137,7 @@ void logAndUpdateState(TransportContext transportContext, int attemptNumber) {
136137
}
137138

138139
if (transportContext.shouldUploadClientHealthMetrics()) {
139-
ClientMetrics clientMetrics =
140-
guard.runCriticalSection(clientHealthMetricsStore::loadClientMetrics);
141-
EventInternal eventInternal =
142-
EventInternal.builder()
143-
.setEventMillis(clock.getTime())
144-
.setUptimeMillis(uptimeClock.getTime())
145-
.setTransportName(CLIENT_HEALTH_METRICS_LOG_SOURCE)
146-
.setEncodedPayload(
147-
new EncodedPayload(Encoding.of("proto"), clientMetrics.toByteArray()))
148-
.build();
149-
EventInternal decoratedEvent = backend.decorate(eventInternal);
150-
eventInternals.add(decoratedEvent);
140+
eventInternals.add(createMetricsEvent(backend));
151141
}
152142

153143
response =
@@ -177,6 +167,13 @@ void logAndUpdateState(TransportContext transportContext, int attemptNumber) {
177167
if (response.getStatus() == BackendResponse.Status.OK) {
178168
maxNextRequestWaitMillis =
179169
Math.max(maxNextRequestWaitMillis, response.getNextRequestWaitMillis());
170+
if (transportContext.shouldUploadClientHealthMetrics()) {
171+
guard.runCriticalSection(
172+
() -> {
173+
clientHealthMetricsStore.resetClientMetrics();
174+
return null;
175+
});
176+
}
180177
} else if (response.getStatus() == BackendResponse.Status.INVALID_PAYLOAD) {
181178
Map<String, Integer> countMap = new HashMap<>();
182179
for (PersistedEvent persistedEvent : persistedEvents) {
@@ -206,4 +203,18 @@ void logAndUpdateState(TransportContext transportContext, int attemptNumber) {
206203
return null;
207204
});
208205
}
206+
207+
@VisibleForTesting
208+
public EventInternal createMetricsEvent(TransportBackend backend) {
209+
ClientMetrics clientMetrics =
210+
guard.runCriticalSection(clientHealthMetricsStore::loadClientMetrics);
211+
return backend.decorate(
212+
EventInternal.builder()
213+
.setEventMillis(clock.getTime())
214+
.setUptimeMillis(uptimeClock.getTime())
215+
.setTransportName(CLIENT_HEALTH_METRICS_LOG_SOURCE)
216+
.setEncodedPayload(
217+
new EncodedPayload(Encoding.of("proto"), clientMetrics.toByteArray()))
218+
.build());
219+
}
209220
}

0 commit comments

Comments
 (0)