Skip to content

Commit c7de6c6

Browse files
steveniemitzmutianf
authored andcommitted
chore: Fix flaky metrics tests (googleapis#1865)
This fixes a few flaky unit tests that relied on `Thread.sleep` to ensure that all metrics processing was done. Rather than using `Thread.sleep`, we can instead use an inline event queue in the OpenCensus stats component to execute all work inline, removing the need to wait for anything to finish.
1 parent db3df29 commit c7de6c6

4 files changed

Lines changed: 46 additions & 30 deletions

File tree

google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/BigtableTracerCallableTest.java

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
import static org.junit.Assert.fail;
2020

2121
import com.google.api.gax.rpc.ClientContext;
22+
import com.google.api.gax.rpc.ServerStream;
2223
import com.google.api.gax.rpc.UnavailableException;
2324
import com.google.bigtable.v2.BigtableGrpc.BigtableImplBase;
2425
import com.google.bigtable.v2.CheckAndMutateRowRequest;
@@ -54,7 +55,6 @@
5455
import io.grpc.Status;
5556
import io.grpc.StatusRuntimeException;
5657
import io.grpc.stub.StreamObserver;
57-
import io.opencensus.impl.stats.StatsComponentImpl;
5858
import io.opencensus.stats.StatsComponent;
5959
import io.opencensus.tags.TagKey;
6060
import io.opencensus.tags.TagValue;
@@ -74,7 +74,7 @@ public class BigtableTracerCallableTest {
7474

7575
private FakeService fakeService = new FakeService();
7676

77-
private final StatsComponent localStats = new StatsComponentImpl();
77+
private final StatsComponent localStats = new SimpleStatsComponent();
7878
private EnhancedBigtableStub stub;
7979
private EnhancedBigtableStub noHeaderStub;
8080
private int attempts;
@@ -157,10 +157,9 @@ public void tearDown() {
157157
}
158158

159159
@Test
160-
public void testGFELatencyMetricReadRows() throws InterruptedException {
161-
stub.readRowsCallable().call(Query.create(TABLE_ID));
162-
163-
Thread.sleep(WAIT_FOR_METRICS_TIME_MS);
160+
public void testGFELatencyMetricReadRows() {
161+
ServerStream<?> call = stub.readRowsCallable().call(Query.create(TABLE_ID));
162+
call.forEach(r -> {});
164163

165164
long latency =
166165
StatsTestUtils.getAggregationValueAsLong(

google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracerTest.java

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -109,6 +109,7 @@ public class BuiltinMetricsTracerTest {
109109
private static final long FAKE_SERVER_TIMING = 50;
110110
private static final long SERVER_LATENCY = 100;
111111
private static final long APPLICATION_LATENCY = 200;
112+
private static final long SLEEP_VARIABILITY = 15;
112113

113114
private static final long CHANNEL_BLOCKING_LATENCY = 75;
114115

@@ -353,7 +354,11 @@ public void onComplete() {
353354
.recordOperation(status.capture(), tableId.capture(), zone.capture(), cluster.capture());
354355

355356
assertThat(counter.get()).isEqualTo(fakeService.getResponseCounter().get());
356-
assertThat(applicationLatency.getValue()).isAtLeast(APPLICATION_LATENCY * counter.get());
357+
// Thread.sleep might not sleep for the requested amount depending on the interrupt period
358+
// defined by the OS.
359+
// On linux this is ~1ms but on windows may be as high as 15-20ms.
360+
assertThat(applicationLatency.getValue())
361+
.isAtLeast((APPLICATION_LATENCY - SLEEP_VARIABILITY) * counter.get());
357362
assertThat(applicationLatency.getValue())
358363
.isAtMost(operationLatency.getValue() - SERVER_LATENCY);
359364
}

google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/MetricsTracerTest.java

Lines changed: 8 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@
5555
import io.grpc.Status;
5656
import io.grpc.StatusRuntimeException;
5757
import io.grpc.stub.StreamObserver;
58-
import io.opencensus.impl.stats.StatsComponentImpl;
58+
import io.opencensus.stats.StatsComponent;
5959
import io.opencensus.tags.TagKey;
6060
import io.opencensus.tags.TagValue;
6161
import io.opencensus.tags.Tags;
@@ -85,6 +85,7 @@ public class MetricsTracerTest {
8585
private static final String INSTANCE_ID = "fake-instance";
8686
private static final String APP_PROFILE_ID = "default";
8787
private static final String TABLE_ID = "fake-table";
88+
private static final long SLEEP_VARIABILITY = 15;
8889

8990
private static final ReadRowsResponse DEFAULT_READ_ROWS_RESPONSES =
9091
ReadRowsResponse.newBuilder()
@@ -105,7 +106,7 @@ public class MetricsTracerTest {
105106
@Mock(answer = Answers.CALLS_REAL_METHODS)
106107
private BigtableGrpc.BigtableImplBase mockService;
107108

108-
private final StatsComponentImpl localStats = new StatsComponentImpl();
109+
private final StatsComponent localStats = new SimpleStatsComponent();
109110
private EnhancedBigtableStub stub;
110111
private BigtableDataSettings settings;
111112

@@ -157,9 +158,6 @@ public Object answer(InvocationOnMock invocation) throws Throwable {
157158
Lists.newArrayList(stub.readRowsCallable().call(Query.create(TABLE_ID)));
158159
long elapsed = stopwatch.elapsed(TimeUnit.MILLISECONDS);
159160

160-
// Give OpenCensus a chance to update the views asynchronously.
161-
Thread.sleep(100);
162-
163161
long opLatency =
164162
StatsTestUtils.getAggregationValueAsLong(
165163
localStats,
@@ -193,9 +191,6 @@ public Object answer(InvocationOnMock invocation) {
193191
Lists.newArrayList(stub.readRowsCallable().call(Query.create(TABLE_ID)));
194192
Lists.newArrayList(stub.readRowsCallable().call(Query.create(TABLE_ID)));
195193

196-
// Give OpenCensus a chance to update the views asynchronously.
197-
Thread.sleep(100);
198-
199194
long opLatency =
200195
StatsTestUtils.getAggregationValueAsLong(
201196
localStats,
@@ -247,8 +242,6 @@ public void testReadRowsFirstRow() throws InterruptedException {
247242
}
248243
long elapsed = stopwatch.elapsed(TimeUnit.MILLISECONDS);
249244

250-
// Give OpenCensus a chance to update the views asynchronously.
251-
Thread.sleep(100);
252245
executor.shutdown();
253246

254247
long firstRowLatency =
@@ -260,7 +253,10 @@ public void testReadRowsFirstRow() throws InterruptedException {
260253
INSTANCE_ID,
261254
APP_PROFILE_ID);
262255

263-
assertThat(firstRowLatency).isIn(Range.closed(beforeSleep, elapsed - afterSleep));
256+
assertThat(firstRowLatency)
257+
.isIn(
258+
Range.closed(
259+
beforeSleep - SLEEP_VARIABILITY, elapsed - afterSleep + SLEEP_VARIABILITY));
264260
}
265261

266262
@Test
@@ -292,9 +288,6 @@ public Object answer(InvocationOnMock invocation) {
292288

293289
Lists.newArrayList(stub.readRowsCallable().call(Query.create(TABLE_ID)));
294290

295-
// Give OpenCensus a chance to update the views asynchronously.
296-
Thread.sleep(100);
297-
298291
long opLatency =
299292
StatsTestUtils.getAggregationValueAsLong(
300293
localStats,
@@ -341,9 +334,6 @@ public Object answer(InvocationOnMock invocation) throws Throwable {
341334
Lists.newArrayList(stub.readRowsCallable().call(Query.create(TABLE_ID)));
342335
long elapsed = stopwatch.elapsed(TimeUnit.MILLISECONDS);
343336

344-
// Give OpenCensus a chance to update the views asynchronously.
345-
Thread.sleep(100);
346-
347337
long attemptLatency =
348338
StatsTestUtils.getAggregationValueAsLong(
349339
localStats,
@@ -360,12 +350,11 @@ public Object answer(InvocationOnMock invocation) throws Throwable {
360350
}
361351

362352
@Test
363-
public void testInvalidRequest() throws InterruptedException {
353+
public void testInvalidRequest() {
364354
try {
365355
stub.bulkMutateRowsCallable().call(BulkMutation.create(TABLE_ID));
366356
Assert.fail("Invalid request should throw exception");
367357
} catch (IllegalStateException e) {
368-
Thread.sleep(100);
369358
// Verify that the latency is recorded with an error code (in this case UNKNOWN)
370359
long attemptLatency =
371360
StatsTestUtils.getAggregationValueAsLong(
@@ -403,9 +392,6 @@ public Object answer(InvocationOnMock invocation) {
403392
batcher.add(ByteString.copyFromUtf8("row1"));
404393
batcher.sendOutstanding();
405394

406-
// Give OpenCensus a chance to update the views asynchronously.
407-
Thread.sleep(100);
408-
409395
long throttledTimeMetric =
410396
StatsTestUtils.getAggregationValueAsLong(
411397
localStats,
@@ -476,7 +462,6 @@ public Object answer(InvocationOnMock invocation) {
476462
batcher.add(RowMutationEntry.create("key"));
477463
batcher.sendOutstanding();
478464

479-
Thread.sleep(100);
480465
long throttledTimeMetric =
481466
StatsTestUtils.getAggregationValueAsLong(
482467
localStats,
Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
/*
2+
* Copyright 2020 Google LLC
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* https://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
package com.google.cloud.bigtable.data.v2.stub.metrics;
17+
18+
import io.opencensus.implcore.common.MillisClock;
19+
import io.opencensus.implcore.internal.SimpleEventQueue;
20+
import io.opencensus.implcore.stats.StatsComponentImplBase;
21+
22+
/** A StatsComponent implementation for testing that executes all events inline. */
23+
public class SimpleStatsComponent extends StatsComponentImplBase {
24+
public SimpleStatsComponent() {
25+
super(new SimpleEventQueue(), MillisClock.getInstance());
26+
}
27+
}

0 commit comments

Comments
 (0)