Skip to content

Commit 1f8cfd7

Browse files
mutianfmutianf
authored
fix: fix the connectivity error count caculation (#1401)
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly: - [ ] Make sure to open an issue as a [bug/issue](https://togithub.com/googleapis/java-bigtable/issues/new/choose) before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea - [ ] Ensure the tests and linter pass - [ ] Code coverage does not decrease (if any source code was changed) - [ ] Appropriate docs were updated (if necessary) Fixes #<issue_number_goes_here> ☕️ If you write sample code, please follow the [samples format]( https://togithub.com/GoogleCloudPlatform/java-docs-samples/blob/main/SAMPLE_FORMAT.md).
1 parent 74779e3 commit 1f8cfd7

File tree

4 files changed

+63
-116
lines changed

4 files changed

+63
-116
lines changed

‎google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/BigtableTracerStreamingCallable.java‎

Lines changed: 2 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -21,12 +21,9 @@
2121
import com.google.api.gax.rpc.ResponseObserver;
2222
import com.google.api.gax.rpc.ServerStreamingCallable;
2323
import com.google.api.gax.rpc.StreamController;
24-
import com.google.bigtable.v2.ResponseParams;
2524
import com.google.cloud.bigtable.data.v2.stub.SafeResponseObserver;
2625
import com.google.common.base.Preconditions;
2726
import com.google.common.base.Stopwatch;
28-
import com.google.protobuf.InvalidProtocolBufferException;
29-
import io.grpc.Metadata;
3027
import java.util.concurrent.TimeUnit;
3128
import javax.annotation.Nonnull;
3229

@@ -101,65 +98,13 @@ protected void onResponseImpl(ResponseT response) {
10198

10299
@Override
103100
protected void onErrorImpl(Throwable t) {
104-
// server-timing metric will be added through GrpcResponseMetadata#onHeaders(Metadata),
105-
// so it's not checking trailing metadata here.
106-
Metadata metadata = responseMetadata.getMetadata();
107-
Long latency = Util.getGfeLatency(metadata);
108-
tracer.recordGfeMetadata(latency, t);
109-
try {
110-
// Check both headers and trailers because in different environments the metadata
111-
// could be returned in headers or trailers
112-
if (metadata != null) {
113-
byte[] trailers = metadata.get(Util.METADATA_KEY);
114-
if (trailers == null) {
115-
Metadata trailingMetadata = responseMetadata.getTrailingMetadata();
116-
if (trailingMetadata != null) {
117-
trailers = trailingMetadata.get(Util.METADATA_KEY);
118-
}
119-
}
120-
// If the response is terminated abnormally and we didn't get location information in
121-
// trailers or headers, skip setting the locations
122-
if (trailers != null) {
123-
ResponseParams decodedTrailers = ResponseParams.parseFrom(trailers);
124-
tracer.setLocations(decodedTrailers.getZoneId(), decodedTrailers.getClusterId());
125-
}
126-
}
127-
} catch (InvalidProtocolBufferException e) {
128-
t.addSuppressed(t);
129-
}
130-
101+
Util.recordMetricsFromMetadata(responseMetadata, tracer, t);
131102
outerObserver.onError(t);
132103
}
133104

134105
@Override
135106
protected void onCompleteImpl() {
136-
Metadata metadata = responseMetadata.getMetadata();
137-
Long latency = Util.getGfeLatency(metadata);
138-
tracer.recordGfeMetadata(latency, null);
139-
try {
140-
// Check both headers and trailers because in different environments the metadata
141-
// could be returned in headers or trailers
142-
if (metadata != null) {
143-
byte[] trailers = metadata.get(Util.METADATA_KEY);
144-
if (trailers == null) {
145-
Metadata trailingMetadata = responseMetadata.getTrailingMetadata();
146-
if (trailingMetadata != null) {
147-
trailers = trailingMetadata.get(Util.METADATA_KEY);
148-
}
149-
}
150-
// If the response is terminated abnormally and we didn't get location information in
151-
// trailers or headers, skip setting the locations
152-
if (trailers != null) {
153-
ResponseParams decodedTrailers = ResponseParams.parseFrom(trailers);
154-
tracer.setLocations(decodedTrailers.getZoneId(), decodedTrailers.getClusterId());
155-
}
156-
}
157-
} catch (InvalidProtocolBufferException e) {
158-
// InvalidProtocolBufferException will only throw if something changed on
159-
// the server side. Location info won't be populated as a result. Ignore
160-
// this error and don't bubble it up to user.
161-
}
162-
107+
Util.recordMetricsFromMetadata(responseMetadata, tracer, null);
163108
outerObserver.onComplete();
164109
}
165110
}

‎google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/BigtableTracerUnaryCallable.java‎

Lines changed: 2 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -22,11 +22,8 @@
2222
import com.google.api.gax.grpc.GrpcResponseMetadata;
2323
import com.google.api.gax.rpc.ApiCallContext;
2424
import com.google.api.gax.rpc.UnaryCallable;
25-
import com.google.bigtable.v2.ResponseParams;
2625
import com.google.common.base.Preconditions;
2726
import com.google.common.util.concurrent.MoreExecutors;
28-
import com.google.protobuf.InvalidProtocolBufferException;
29-
import io.grpc.Metadata;
3027
import javax.annotation.Nonnull;
3128

3229
/**
@@ -79,56 +76,12 @@ class BigtableTracerUnaryCallback<ResponseT> implements ApiFutureCallback<Respon
7976

8077
@Override
8178
public void onFailure(Throwable throwable) {
82-
Metadata metadata = responseMetadata.getMetadata();
83-
Long latency = Util.getGfeLatency(metadata);
84-
tracer.recordGfeMetadata(latency, throwable);
85-
try {
86-
// Check both headers and trailers because in different environments the metadata
87-
// could be returned in headers or trailers
88-
if (metadata != null) {
89-
byte[] trailers = metadata.get(Util.METADATA_KEY);
90-
if (trailers == null) {
91-
Metadata trailingMetadata = responseMetadata.getTrailingMetadata();
92-
if (trailingMetadata != null) {
93-
trailers = trailingMetadata.get(Util.METADATA_KEY);
94-
}
95-
}
96-
// If the response is terminated abnormally and we didn't get location information in
97-
// trailers or headers, skip setting the locations
98-
if (trailers != null) {
99-
ResponseParams decodedTrailers = ResponseParams.parseFrom(trailers);
100-
tracer.setLocations(decodedTrailers.getZoneId(), decodedTrailers.getClusterId());
101-
}
102-
}
103-
} catch (InvalidProtocolBufferException e) {
104-
}
79+
Util.recordMetricsFromMetadata(responseMetadata, tracer, throwable);
10580
}
10681

10782
@Override
10883
public void onSuccess(ResponseT response) {
109-
Metadata metadata = responseMetadata.getMetadata();
110-
Long latency = Util.getGfeLatency(metadata);
111-
tracer.recordGfeMetadata(latency, null);
112-
try {
113-
// Check both headers and trailers because in different environments the metadata
114-
// could be returned in headers or trailers
115-
if (metadata != null) {
116-
byte[] trailers = metadata.get(Util.METADATA_KEY);
117-
if (trailers == null) {
118-
Metadata trailingMetadata = responseMetadata.getTrailingMetadata();
119-
if (trailingMetadata != null) {
120-
trailers = trailingMetadata.get(Util.METADATA_KEY);
121-
}
122-
}
123-
// If the response is terminated abnormally and we didn't get location information in
124-
// trailers or headers, skip setting the locations
125-
if (trailers != null) {
126-
ResponseParams decodedTrailers = ResponseParams.parseFrom(trailers);
127-
tracer.setLocations(decodedTrailers.getZoneId(), decodedTrailers.getClusterId());
128-
}
129-
}
130-
} catch (InvalidProtocolBufferException e) {
131-
}
84+
Util.recordMetricsFromMetadata(responseMetadata, tracer, null);
13285
}
13386
}
13487
}

‎google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/Util.java‎

Lines changed: 58 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
package com.google.cloud.bigtable.data.v2.stub.metrics;
1717

1818
import com.google.api.core.InternalApi;
19+
import com.google.api.gax.grpc.GrpcResponseMetadata;
1920
import com.google.api.gax.rpc.ApiCallContext;
2021
import com.google.api.gax.rpc.ApiException;
2122
import com.google.api.gax.rpc.StatusCode;
@@ -25,10 +26,12 @@
2526
import com.google.bigtable.v2.MutateRowsRequest;
2627
import com.google.bigtable.v2.ReadModifyWriteRowRequest;
2728
import com.google.bigtable.v2.ReadRowsRequest;
29+
import com.google.bigtable.v2.ResponseParams;
2830
import com.google.bigtable.v2.SampleRowKeysRequest;
2931
import com.google.bigtable.v2.TableName;
3032
import com.google.common.base.Strings;
3133
import com.google.common.collect.ImmutableMap;
34+
import com.google.protobuf.InvalidProtocolBufferException;
3235
import io.grpc.Metadata;
3336
import io.grpc.Status;
3437
import io.grpc.StatusException;
@@ -57,7 +60,7 @@ public class Util {
5760
private static final Metadata.Key<String> SERVER_TIMING_HEADER_KEY =
5861
Metadata.Key.of("server-timing", Metadata.ASCII_STRING_MARSHALLER);
5962
private static final Pattern SERVER_TIMING_HEADER_PATTERN = Pattern.compile(".*dur=(?<dur>\\d+)");
60-
static final Metadata.Key<byte[]> METADATA_KEY =
63+
static final Metadata.Key<byte[]> LOCATION_METADATA_KEY =
6164
Metadata.Key.of("x-goog-ext-425905942-bin", Metadata.BINARY_BYTE_MARSHALLER);
6265

6366
/** Convert an exception into a value that can be used to create an OpenCensus tag value. */
@@ -136,16 +139,62 @@ static Map<String, List<String>> createStatsHeaders(ApiCallContext apiCallContex
136139
return headers.build();
137140
}
138141

139-
static Long getGfeLatency(Metadata metadata) {
140-
if (metadata != null && metadata.get(SERVER_TIMING_HEADER_KEY) != null) {
141-
String serverTiming = metadata.get(SERVER_TIMING_HEADER_KEY);
142-
Matcher matcher = SERVER_TIMING_HEADER_PATTERN.matcher(serverTiming);
143-
// this should always be true
144-
if (matcher.find()) {
145-
long latency = Long.valueOf(matcher.group("dur"));
146-
return latency;
142+
private static Long getGfeLatency(@Nullable Metadata metadata) {
143+
if (metadata == null) {
144+
return null;
145+
}
146+
String serverTiming = metadata.get(SERVER_TIMING_HEADER_KEY);
147+
if (serverTiming == null) {
148+
return null;
149+
}
150+
Matcher matcher = SERVER_TIMING_HEADER_PATTERN.matcher(serverTiming);
151+
// this should always be true
152+
if (matcher.find()) {
153+
long latency = Long.valueOf(matcher.group("dur"));
154+
return latency;
155+
}
156+
return null;
157+
}
158+
159+
private static ResponseParams getResponseParams(@Nullable Metadata metadata) {
160+
if (metadata == null) {
161+
return null;
162+
}
163+
byte[] responseParams = metadata.get(Util.LOCATION_METADATA_KEY);
164+
if (responseParams != null) {
165+
try {
166+
return ResponseParams.parseFrom(responseParams);
167+
} catch (InvalidProtocolBufferException e) {
147168
}
148169
}
149170
return null;
150171
}
172+
173+
static void recordMetricsFromMetadata(
174+
GrpcResponseMetadata responseMetadata, BigtableTracer tracer, Throwable throwable) {
175+
Metadata metadata = responseMetadata.getMetadata();
176+
177+
// Get the response params from the metadata. Check both headers and trailers
178+
// because in different environments the metadata could be returned in headers or trailers
179+
@Nullable ResponseParams responseParams = getResponseParams(responseMetadata.getMetadata());
180+
if (responseParams == null) {
181+
responseParams = getResponseParams(responseMetadata.getTrailingMetadata());
182+
}
183+
// Set tracer locations if response params is not null
184+
if (responseParams != null) {
185+
tracer.setLocations(responseParams.getZoneId(), responseParams.getClusterId());
186+
}
187+
188+
// server-timing metric will be added through GrpcResponseMetadata#onHeaders(Metadata),
189+
// so it's not checking trailing metadata here.
190+
@Nullable Long latency = getGfeLatency(metadata);
191+
// For direct path, we won't see GFE server-timing header. However, if we received the
192+
// location info, we know that there isn't a connectivity issue. Set the latency to
193+
// 0 so gfe missing header won't get incremented.
194+
if (responseParams != null && latency == null) {
195+
latency = 0L;
196+
}
197+
// Record gfe metrics
198+
tracer.recordGfeMetadata(latency, throwable);
199+
}
151200
}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -127,7 +127,7 @@ public void sendHeaders(Metadata headers) {
127127
ResponseParams params =
128128
ResponseParams.newBuilder().setZoneId(ZONE).setClusterId(CLUSTER).build();
129129
byte[] byteArray = params.toByteArray();
130-
headers.put(Util.METADATA_KEY, byteArray);
130+
headers.put(Util.LOCATION_METADATA_KEY, byteArray);
131131

132132
super.sendHeaders(headers);
133133
}

0 commit comments

Comments
 (0)