Skip to content

Commit b6c9c6e

Browse files
rahul2393rahul2393
authored
Revert "feat: built in metrics for afe latency and connectivity error (#3724)" (#3818)
This reverts commit e13a2f9.
1 parent e13a2f9 commit b6c9c6e

File tree

11 files changed

+50
-245
lines changed

11 files changed

+50
-245
lines changed

‎google-cloud-spanner/clirr-ignored-differences.xml‎

Lines changed: 4 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -751,13 +751,6 @@
751751
<method>boolean isEnableBuiltInMetrics()</method>
752752
</difference>
753753

754-
<!-- Added AFE Server Timing option -->
755-
<difference>
756-
<differenceType>7012</differenceType>
757-
<className>com/google/cloud/spanner/SpannerOptions$SpannerEnvironment</className>
758-
<method>boolean isEnableAFEServerTiming()</method>
759-
</difference>
760-
761754
<!-- Added Monitoring host option -->
762755
<difference>
763756
<differenceType>7012</differenceType>
@@ -814,7 +807,7 @@
814807
<className>com/google/cloud/spanner/connection/Connection</className>
815808
<method>boolean isKeepTransactionAlive()</method>
816809
</difference>
817-
810+
818811
<!-- Automatic DML batching -->
819812
<difference>
820813
<differenceType>7012</differenceType>
@@ -846,7 +839,7 @@
846839
<className>com/google/cloud/spanner/connection/Connection</className>
847840
<method>boolean isAutoBatchDmlUpdateCountVerification()</method>
848841
</difference>
849-
842+
850843
<!-- Retry DML as Partitioned DML -->
851844
<difference>
852845
<differenceType>7012</differenceType>
@@ -870,7 +863,7 @@
870863
<className>com/google/cloud/spanner/connection/Connection</className>
871864
<method>java.lang.Object runTransaction(com.google.cloud.spanner.connection.Connection$TransactionCallable)</method>
872865
</difference>
873-
866+
874867
<!-- Added experimental host option -->
875868
<difference>
876869
<differenceType>7012</differenceType>
@@ -899,7 +892,7 @@
899892
<className>com/google/cloud/spanner/connection/Connection</className>
900893
<method>java.lang.String getDefaultSequenceKind()</method>
901894
</difference>
902-
895+
903896
<!-- Default isolation level -->
904897
<difference>
905898
<differenceType>7012</differenceType>

‎google-cloud-spanner/src/main/java/com/google/cloud/spanner/BuiltInMetricsConstant.java‎

Lines changed: 16 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,6 @@
2626
import io.opentelemetry.sdk.metrics.InstrumentSelector;
2727
import io.opentelemetry.sdk.metrics.InstrumentType;
2828
import io.opentelemetry.sdk.metrics.View;
29-
import java.util.List;
3029
import java.util.Map;
3130
import java.util.Set;
3231
import java.util.stream.Collectors;
@@ -38,9 +37,6 @@ public class BuiltInMetricsConstant {
3837
public static final String GAX_METER_NAME = OpenTelemetryMetricsRecorder.GAX_METER_NAME;
3938
static final String SPANNER_METER_NAME = "spanner-java";
4039
static final String GFE_LATENCIES_NAME = "gfe_latencies";
41-
static final String AFE_LATENCIES_NAME = "afe_latencies";
42-
static final String GFE_CONNECTIVITY_ERROR_NAME = "gfe_connectivity_error_count";
43-
static final String AFE_CONNECTIVITY_ERROR_NAME = "afe_connectivity_error_count";
4440
static final String OPERATION_LATENCIES_NAME = "operation_latencies";
4541
static final String ATTEMPT_LATENCIES_NAME = "attempt_latencies";
4642
static final String OPERATION_LATENCY_NAME = "operation_latency";
@@ -54,10 +50,7 @@ public class BuiltInMetricsConstant {
5450
ATTEMPT_LATENCIES_NAME,
5551
OPERATION_COUNT_NAME,
5652
ATTEMPT_COUNT_NAME,
57-
GFE_LATENCIES_NAME,
58-
AFE_LATENCIES_NAME,
59-
GFE_CONNECTIVITY_ERROR_NAME,
60-
AFE_CONNECTIVITY_ERROR_NAME)
53+
GFE_LATENCIES_NAME)
6154
.stream()
6255
.map(m -> METER_NAME + '/' + m)
6356
.collect(Collectors.toSet());
@@ -109,14 +102,14 @@ public class BuiltInMetricsConstant {
109102
DIRECT_PATH_ENABLED_KEY,
110103
DIRECT_PATH_USED_KEY);
111104

112-
static List<Double> BUCKET_BOUNDARIES =
113-
ImmutableList.of(
114-
0.0, 0.5, 1.0, 2.0, 3.0, 4.0, 5.0, 6.0, 7.0, 8.0, 9.0, 10.0, 11.0, 12.0, 13.0, 14.0, 15.0,
115-
16.0, 17.0, 18.0, 19.0, 20.0, 25.0, 30.0, 40.0, 50.0, 65.0, 80.0, 100.0, 130.0, 160.0,
116-
200.0, 250.0, 300.0, 400.0, 500.0, 650.0, 800.0, 1000.0, 2000.0, 5000.0, 10000.0, 20000.0,
117-
50000.0, 100000.0, 200000.0, 400000.0, 800000.0, 1600000.0, 3200000.0);
118105
static Aggregation AGGREGATION_WITH_MILLIS_HISTOGRAM =
119-
Aggregation.explicitBucketHistogram(BUCKET_BOUNDARIES);
106+
Aggregation.explicitBucketHistogram(
107+
ImmutableList.of(
108+
0.0, 0.5, 1.0, 2.0, 3.0, 4.0, 5.0, 6.0, 7.0, 8.0, 9.0, 10.0, 11.0, 12.0, 13.0, 14.0,
109+
15.0, 16.0, 17.0, 18.0, 19.0, 20.0, 25.0, 30.0, 40.0, 50.0, 65.0, 80.0, 100.0, 130.0,
110+
160.0, 200.0, 250.0, 300.0, 400.0, 500.0, 650.0, 800.0, 1000.0, 2000.0, 5000.0,
111+
10000.0, 20000.0, 50000.0, 100000.0, 200000.0, 400000.0, 800000.0, 1600000.0,
112+
3200000.0));
120113

121114
static Map<InstrumentSelector, View> getAllViews() {
122115
ImmutableMap.Builder<InstrumentSelector, View> views = ImmutableMap.builder();
@@ -136,6 +129,14 @@ static Map<InstrumentSelector, View> getAllViews() {
136129
BuiltInMetricsConstant.AGGREGATION_WITH_MILLIS_HISTOGRAM,
137130
InstrumentType.HISTOGRAM,
138131
"ms");
132+
defineView(
133+
views,
134+
BuiltInMetricsConstant.SPANNER_METER_NAME,
135+
BuiltInMetricsConstant.GFE_LATENCIES_NAME,
136+
BuiltInMetricsConstant.GFE_LATENCIES_NAME,
137+
BuiltInMetricsConstant.AGGREGATION_WITH_MILLIS_HISTOGRAM,
138+
InstrumentType.HISTOGRAM,
139+
"ms");
139140
defineView(
140141
views,
141142
BuiltInMetricsConstant.GAX_METER_NAME,

‎google-cloud-spanner/src/main/java/com/google/cloud/spanner/BuiltInMetricsRecorder.java‎

Lines changed: 2 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,6 @@
2323
import io.opentelemetry.api.common.Attributes;
2424
import io.opentelemetry.api.common.AttributesBuilder;
2525
import io.opentelemetry.api.metrics.DoubleHistogram;
26-
import io.opentelemetry.api.metrics.LongCounter;
2726
import io.opentelemetry.api.metrics.Meter;
2827
import java.util.Map;
2928

@@ -36,9 +35,6 @@
3635
class BuiltInMetricsRecorder extends OpenTelemetryMetricsRecorder {
3736

3837
private final DoubleHistogram gfeLatencyRecorder;
39-
private final DoubleHistogram afeLatencyRecorder;
40-
private final LongCounter gfeHeaderMissingCountRecorder;
41-
private final LongCounter afeHeaderMissingCountRecorder;
4238

4339
/**
4440
* Creates the following instruments for the following metrics:
@@ -63,27 +59,6 @@ class BuiltInMetricsRecorder extends OpenTelemetryMetricsRecorder {
6359
.setDescription(
6460
"Latency between Google's network receiving an RPC and reading back the first byte of the response")
6561
.setUnit("ms")
66-
.setExplicitBucketBoundariesAdvice(BuiltInMetricsConstant.BUCKET_BOUNDARIES)
67-
.build();
68-
this.afeLatencyRecorder =
69-
meter
70-
.histogramBuilder(serviceName + '/' + BuiltInMetricsConstant.AFE_LATENCIES_NAME)
71-
.setDescription(
72-
"Latency between Spanner API Frontend receiving an RPC and starting to write back the response.")
73-
.setExplicitBucketBoundariesAdvice(BuiltInMetricsConstant.BUCKET_BOUNDARIES)
74-
.setUnit("ms")
75-
.build();
76-
this.gfeHeaderMissingCountRecorder =
77-
meter
78-
.counterBuilder(serviceName + '/' + BuiltInMetricsConstant.GFE_CONNECTIVITY_ERROR_NAME)
79-
.setDescription("Number of requests that failed to reach the Google network.")
80-
.setUnit("1")
81-
.build();
82-
this.afeHeaderMissingCountRecorder =
83-
meter
84-
.counterBuilder(serviceName + '/' + BuiltInMetricsConstant.AFE_CONNECTIVITY_ERROR_NAME)
85-
.setDescription("Number of requests that failed to reach the Spanner API Frontend.")
86-
.setUnit("1")
8762
.build();
8863
}
8964

@@ -94,25 +69,8 @@ class BuiltInMetricsRecorder extends OpenTelemetryMetricsRecorder {
9469
* @param gfeLatency Attempt Latency in ms
9570
* @param attributes Map of the attributes to store
9671
*/
97-
void recordServerTimingHeaderMetrics(
98-
Long gfeLatency,
99-
Long afeLatency,
100-
Long gfeHeaderMissingCount,
101-
Long afeHeaderMissingCount,
102-
Map<String, String> attributes) {
103-
io.opentelemetry.api.common.Attributes otelAttributes = toOtelAttributes(attributes);
104-
if (gfeLatency != null) {
105-
gfeLatencyRecorder.record(gfeLatency, otelAttributes);
106-
}
107-
if (gfeHeaderMissingCount > 0) {
108-
gfeHeaderMissingCountRecorder.add(gfeHeaderMissingCount, otelAttributes);
109-
}
110-
if (afeLatency != null) {
111-
afeLatencyRecorder.record(afeLatency, otelAttributes);
112-
}
113-
if (afeHeaderMissingCount > 0) {
114-
afeHeaderMissingCountRecorder.add(afeHeaderMissingCount, otelAttributes);
115-
}
72+
void recordGFELatency(double gfeLatency, Map<String, String> attributes) {
73+
gfeLatencyRecorder.record(gfeLatency, toOtelAttributes(attributes));
11674
}
11775

11876
Attributes toOtelAttributes(Map<String, String> attributes) {

‎google-cloud-spanner/src/main/java/com/google/cloud/spanner/BuiltInMetricsTracer.java‎

Lines changed: 21 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -37,10 +37,8 @@ class BuiltInMetricsTracer extends MetricsTracer implements ApiTracer {
3737
private final BuiltInMetricsRecorder builtInOpenTelemetryMetricsRecorder;
3838
// These are RPC specific attributes and pertain to a specific API Trace
3939
private final Map<String, String> attributes = new HashMap<>();
40+
4041
private Long gfeLatency = null;
41-
private Long afeLatency = null;
42-
private long gfeHeaderMissingCount = 0;
43-
private long afeHeaderMissingCount = 0;
4442

4543
BuiltInMetricsTracer(
4644
MethodName methodName, BuiltInMetricsRecorder builtInOpenTelemetryMetricsRecorder) {
@@ -56,9 +54,10 @@ class BuiltInMetricsTracer extends MetricsTracer implements ApiTracer {
5654
@Override
5755
public void attemptSucceeded() {
5856
super.attemptSucceeded();
59-
attributes.put(STATUS_ATTRIBUTE, StatusCode.Code.OK.toString());
60-
builtInOpenTelemetryMetricsRecorder.recordServerTimingHeaderMetrics(
61-
gfeLatency, afeLatency, gfeHeaderMissingCount, afeHeaderMissingCount, attributes);
57+
if (gfeLatency != null) {
58+
attributes.put(STATUS_ATTRIBUTE, StatusCode.Code.OK.toString());
59+
builtInOpenTelemetryMetricsRecorder.recordGFELatency(gfeLatency, attributes);
60+
}
6261
}
6362

6463
/**
@@ -68,9 +67,10 @@ public void attemptSucceeded() {
6867
@Override
6968
public void attemptCancelled() {
7069
super.attemptCancelled();
71-
attributes.put(STATUS_ATTRIBUTE, StatusCode.Code.CANCELLED.toString());
72-
builtInOpenTelemetryMetricsRecorder.recordServerTimingHeaderMetrics(
73-
gfeLatency, afeLatency, gfeHeaderMissingCount, afeHeaderMissingCount, attributes);
70+
if (gfeLatency != null) {
71+
attributes.put(STATUS_ATTRIBUTE, StatusCode.Code.CANCELLED.toString());
72+
builtInOpenTelemetryMetricsRecorder.recordGFELatency(gfeLatency, attributes);
73+
}
7474
}
7575

7676
/**
@@ -84,9 +84,10 @@ public void attemptCancelled() {
8484
@Override
8585
public void attemptFailedDuration(Throwable error, java.time.Duration delay) {
8686
super.attemptFailedDuration(error, delay);
87-
attributes.put(STATUS_ATTRIBUTE, extractStatus(error));
88-
builtInOpenTelemetryMetricsRecorder.recordServerTimingHeaderMetrics(
89-
gfeLatency, afeLatency, gfeHeaderMissingCount, afeHeaderMissingCount, attributes);
87+
if (gfeLatency != null) {
88+
attributes.put(STATUS_ATTRIBUTE, extractStatus(error));
89+
builtInOpenTelemetryMetricsRecorder.recordGFELatency(gfeLatency, attributes);
90+
}
9091
}
9192

9293
/**
@@ -99,9 +100,10 @@ public void attemptFailedDuration(Throwable error, java.time.Duration delay) {
99100
@Override
100101
public void attemptFailedRetriesExhausted(Throwable error) {
101102
super.attemptFailedRetriesExhausted(error);
102-
attributes.put(STATUS_ATTRIBUTE, extractStatus(error));
103-
builtInOpenTelemetryMetricsRecorder.recordServerTimingHeaderMetrics(
104-
gfeLatency, afeLatency, gfeHeaderMissingCount, afeHeaderMissingCount, attributes);
103+
if (gfeLatency != null) {
104+
attributes.put(STATUS_ATTRIBUTE, extractStatus(error));
105+
builtInOpenTelemetryMetricsRecorder.recordGFELatency(gfeLatency, attributes);
106+
}
105107
}
106108

107109
/**
@@ -114,27 +116,16 @@ public void attemptFailedRetriesExhausted(Throwable error) {
114116
@Override
115117
public void attemptPermanentFailure(Throwable error) {
116118
super.attemptPermanentFailure(error);
117-
attributes.put(STATUS_ATTRIBUTE, extractStatus(error));
118-
builtInOpenTelemetryMetricsRecorder.recordServerTimingHeaderMetrics(
119-
gfeLatency, afeLatency, gfeHeaderMissingCount, afeHeaderMissingCount, attributes);
119+
if (gfeLatency != null) {
120+
attributes.put(STATUS_ATTRIBUTE, extractStatus(error));
121+
builtInOpenTelemetryMetricsRecorder.recordGFELatency(gfeLatency, attributes);
122+
}
120123
}
121124

122125
void recordGFELatency(Long gfeLatency) {
123126
this.gfeLatency = gfeLatency;
124127
}
125128

126-
void recordAFELatency(Long afeLatency) {
127-
this.afeLatency = afeLatency;
128-
}
129-
130-
void recordGfeHeaderMissingCount(Long value) {
131-
this.gfeHeaderMissingCount = value;
132-
}
133-
134-
void recordAfeHeaderMissingCount(Long value) {
135-
this.afeHeaderMissingCount = value;
136-
}
137-
138129
@Override
139130
public void addAttributes(Map<String, String> attributes) {
140131
super.addAttributes(attributes);

‎google-cloud-spanner/src/main/java/com/google/cloud/spanner/CompositeTracer.java‎

Lines changed: 0 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -198,28 +198,4 @@ public void recordGFELatency(Long gfeLatency) {
198198
}
199199
}
200200
}
201-
202-
public void recordGfeHeaderMissingCount(Long value) {
203-
for (ApiTracer child : children) {
204-
if (child instanceof BuiltInMetricsTracer) {
205-
((BuiltInMetricsTracer) child).recordGfeHeaderMissingCount(value);
206-
}
207-
}
208-
}
209-
210-
public void recordAFELatency(Long afeLatency) {
211-
for (ApiTracer child : children) {
212-
if (child instanceof BuiltInMetricsTracer) {
213-
((BuiltInMetricsTracer) child).recordAFELatency(afeLatency);
214-
}
215-
}
216-
}
217-
218-
public void recordAfeHeaderMissingCount(Long value) {
219-
for (ApiTracer child : children) {
220-
if (child instanceof BuiltInMetricsTracer) {
221-
((BuiltInMetricsTracer) child).recordAfeHeaderMissingCount(value);
222-
}
223-
}
224-
}
225201
}

‎google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/GapicSpannerRpc.java‎

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -683,10 +683,6 @@ private static boolean isEmulatorEnabled(SpannerOptions options, String emulator
683683
&& options.getHost().endsWith(emulatorHost);
684684
}
685685

686-
public static boolean isEnableAFEServerTiming() {
687-
return !Boolean.parseBoolean(System.getenv("SPANNER_DISABLE_AFE_SERVER_TIMING"));
688-
}
689-
690686
private static final RetrySettings ADMIN_REQUESTS_LIMIT_EXCEEDED_RETRY_SETTINGS =
691687
RetrySettings.newBuilder()
692688
.setInitialRetryDelayDuration(Duration.ofSeconds(5L))
@@ -2034,9 +2030,6 @@ <ReqT, RespT> GrpcCallContext newCallContext(
20342030
if (endToEndTracingEnabled) {
20352031
context = context.withExtraHeaders(metadataProvider.newEndToEndTracingHeader());
20362032
}
2037-
if (isEnableAFEServerTiming()) {
2038-
context = context.withExtraHeaders(metadataProvider.newAfeServerTimingHeader());
2039-
}
20402033
if (callCredentialsProvider != null) {
20412034
CallCredentials callCredentials = callCredentialsProvider.getCallCredentials();
20422035
if (callCredentials != null) {

‎google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/HeaderInterceptor.java‎

Lines changed: 1 addition & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,6 @@ class HeaderInterceptor implements ClientInterceptor {
7272
private static final Metadata.Key<String> SERVER_TIMING_HEADER_KEY =
7373
Metadata.Key.of("server-timing", Metadata.ASCII_STRING_MARSHALLER);
7474
private static final String GFE_TIMING_HEADER = "gfet4t7";
75-
private static final String AFE_TIMING_HEADER = "afet4t7";
7675
private static final Metadata.Key<String> GOOGLE_CLOUD_RESOURCE_PREFIX_KEY =
7776
Metadata.Key.of("google-cloud-resource-prefix", Metadata.ASCII_STRING_MARSHALLER);
7877
private static final Pattern SERVER_TIMING_PATTERN =
@@ -175,25 +174,13 @@ private void processHeader(
175174
if (compositeTracer != null) {
176175
compositeTracer.recordGFELatency(gfeLatency);
177176
}
177+
178178
if (span != null) {
179179
span.setAttribute("gfe_latency", String.valueOf(gfeLatency));
180180
}
181181
} else {
182182
measureMap.put(SPANNER_GFE_HEADER_MISSING_COUNT, 1L).record(tagContext);
183183
spannerRpcMetrics.recordGfeHeaderMissingCount(1L, attributes);
184-
if (compositeTracer != null) {
185-
compositeTracer.recordGfeHeaderMissingCount(1L);
186-
}
187-
}
188-
189-
// Record AFE metrics
190-
if (compositeTracer != null && GapicSpannerRpc.isEnableAFEServerTiming()) {
191-
if (serverTimingMetrics.containsKey(AFE_TIMING_HEADER)) {
192-
long afeLatency = serverTimingMetrics.get(AFE_TIMING_HEADER);
193-
compositeTracer.recordAFELatency(afeLatency);
194-
} else {
195-
compositeTracer.recordAfeHeaderMissingCount(1L);
196-
}
197184
}
198185
} catch (NumberFormatException e) {
199186
LOGGER.log(LEVEL, "Invalid server-timing object in header: {}", serverTiming);

0 commit comments

Comments
 (0)