Skip to content

Commit 28eebf0

Browse files
diegomarquezpdiegomarquezp
authored
fix(o11y): do not record error.type in successful runs (#12620)
When `error` is null, return `null` since there is no error, hence no error type. Also, added tests in `ITOtelTracing` to confirm that the error attrs are not recorded.
1 parent 9276fbf commit 28eebf0

6 files changed

Lines changed: 69 additions & 34 deletions

File tree

‎sdk-platform-java/gax-java/gax/src/main/java/com/google/api/gax/tracing/ErrorTypeUtil.java‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -112,14 +112,14 @@ enum ErrorType {
112112
* </ol>
113113
*
114114
* @param error the Throwable from which to extract the error type string.
115-
* @return a low-cardinality string representing the specific error type, or {@code
116-
* ErrorType.INTERNAL.toString()} if the provided error is {@code null} or non-determined.
115+
* @return a low-cardinality string representing the specific error type, or {@code null} if the
116+
* provided error is {@code null} or non-determined.
117117
*/
118118
// Requirement source: go/clo:product-requirements-v1
119119
public static String extractErrorType(@Nullable Throwable error) {
120120
if (error == null) {
121-
// No information about the error; we default to INTERNAL.
122-
return ErrorType.INTERNAL.toString();
121+
// No information about the error; return null so the attribute is not recorded.
122+
return null;
123123
}
124124

125125
// 1. Unwrap standard wrapper exceptions if present

‎sdk-platform-java/gax-java/gax/src/main/java/com/google/api/gax/tracing/SpanTracer.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -205,9 +205,6 @@ private void recordErrorAndEndAttempt(Throwable error) {
205205
return;
206206
}
207207

208-
attemptSpan.setAttribute(
209-
ObservabilityAttributes.ERROR_TYPE_ATTRIBUTE, ObservabilityUtils.extractErrorType(error));
210-
211208
Map<String, Object> statusAttributes = new HashMap<>();
212209
ObservabilityUtils.populateStatusAttributes(
213210
statusAttributes, error, this.apiTracerContext.transport());
@@ -220,6 +217,9 @@ private void recordErrorAndEndAttempt(Throwable error) {
220217
return;
221218
}
222219

220+
attemptSpan.setAttribute(
221+
ObservabilityAttributes.ERROR_TYPE_ATTRIBUTE, ObservabilityUtils.extractErrorType(error));
222+
223223
attemptSpan.setAttribute(
224224
ObservabilityAttributes.EXCEPTION_TYPE_ATTRIBUTE, error.getClass().getName());
225225

‎sdk-platform-java/gax-java/gax/src/test/java/com/google/api/gax/tracing/ErrorTypeUtilTest.java‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -53,8 +53,7 @@ class ErrorTypeUtilTest {
5353

5454
@Test
5555
void testExtractErrorType_null() {
56-
assertThat(ErrorTypeUtil.extractErrorType(null))
57-
.isEqualTo(ErrorTypeUtil.ErrorType.INTERNAL.toString());
56+
assertThat(ErrorTypeUtil.extractErrorType(null)).isNull();
5857
}
5958

6059
@Test

‎sdk-platform-java/gax-java/gax/src/test/java/com/google/api/gax/tracing/SpanTracerTest.java‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -565,10 +565,8 @@ void testAttemptFailed_internalFallback_nullError() {
565565
// For an anonymous inner class Throwable, getSimpleName() is empty string,
566566
// which triggers the
567567
// fallback
568-
verify(span)
569-
.setAttribute(
570-
ObservabilityAttributes.ERROR_TYPE_ATTRIBUTE,
571-
ErrorTypeUtil.ErrorType.INTERNAL.toString());
568+
verify(span, never())
569+
.setAttribute(eq(ObservabilityAttributes.ERROR_TYPE_ATTRIBUTE), anyString());
572570
verify(span).end();
573571
}
574572

‎sdk-platform-java/java-showcase/gapic-showcase/src/test/java/com/google/showcase/v1beta1/it/ITCompositeTracer.java‎

Lines changed: 23 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,6 @@
3434

3535
import com.google.api.gax.tracing.CompositeTracerFactory;
3636
import com.google.api.gax.tracing.GoldenSignalsMetricsTracerFactory;
37-
import com.google.api.gax.tracing.LoggingTracerFactory;
3837
import com.google.api.gax.tracing.ObservabilityAttributes;
3938
import com.google.api.gax.tracing.SpanTracerFactory;
4039
import com.google.showcase.v1beta1.EchoClient;
@@ -43,13 +42,13 @@
4342
import io.opentelemetry.api.GlobalOpenTelemetry;
4443
import io.opentelemetry.api.common.AttributeKey;
4544
import io.opentelemetry.sdk.OpenTelemetrySdk;
45+
import io.opentelemetry.sdk.metrics.SdkMeterProvider;
4646
import io.opentelemetry.sdk.metrics.data.MetricData;
47+
import io.opentelemetry.sdk.testing.exporter.InMemoryMetricReader;
4748
import io.opentelemetry.sdk.testing.exporter.InMemorySpanExporter;
4849
import io.opentelemetry.sdk.trace.SdkTracerProvider;
4950
import io.opentelemetry.sdk.trace.data.SpanData;
5051
import io.opentelemetry.sdk.trace.export.SimpleSpanProcessor;
51-
import io.opentelemetry.sdk.testing.exporter.InMemoryMetricReader;
52-
import io.opentelemetry.sdk.metrics.SdkMeterProvider;
5352
import java.util.Arrays;
5453
import java.util.Collection;
5554
import java.util.List;
@@ -94,15 +93,16 @@ void tearDown() {
9493

9594
private CompositeTracerFactory createCompositeTracerFactory() {
9695
SpanTracerFactory spanTracerFactory = new SpanTracerFactory(openTelemetrySdk);
97-
GoldenSignalsMetricsTracerFactory metricsTracerFactory = new GoldenSignalsMetricsTracerFactory(openTelemetrySdk);
96+
GoldenSignalsMetricsTracerFactory metricsTracerFactory =
97+
new GoldenSignalsMetricsTracerFactory(openTelemetrySdk);
9898

9999
return new CompositeTracerFactory(Arrays.asList(spanTracerFactory, metricsTracerFactory));
100100
}
101101

102102
@Test
103103
void testCompositeTracer() throws Exception {
104104
try (EchoClient client =
105-
TestClientInitializer.createGrpcEchoClientOpentelemetry(createCompositeTracerFactory())) {
105+
TestClientInitializer.createGrpcEchoClientOpentelemetry(createCompositeTracerFactory())) {
106106

107107
client.echo(EchoRequest.newBuilder().setContent("composite-tracing-test").build());
108108

@@ -111,31 +111,37 @@ void testCompositeTracer() throws Exception {
111111
assertThat(actualSpans).isNotEmpty();
112112

113113
SpanData attemptSpan =
114-
actualSpans.stream()
115-
.filter(span -> span.getName().equals("google.showcase.v1beta1.Echo/Echo"))
116-
.findFirst()
117-
.orElseThrow(() -> new AssertionError("Incorrect span name"));
114+
actualSpans.stream()
115+
.filter(span -> span.getName().equals("google.showcase.v1beta1.Echo/Echo"))
116+
.findFirst()
117+
.orElseThrow(() -> new AssertionError("Incorrect span name"));
118118
assertThat(attemptSpan.getInstrumentationScopeInfo().getName()).isEqualTo(SHOWCASE_ARTIFACT);
119119
assertThat(
120120
attemptSpan
121-
.getAttributes()
122-
.get(AttributeKey.stringKey(ObservabilityAttributes.SERVER_ADDRESS_ATTRIBUTE)))
123-
.isEqualTo(SHOWCASE_SERVER_ADDRESS);
121+
.getAttributes()
122+
.get(AttributeKey.stringKey(ObservabilityAttributes.SERVER_ADDRESS_ATTRIBUTE)))
123+
.isEqualTo(SHOWCASE_SERVER_ADDRESS);
124124

125125
// Verify metric name and one basic attribute server.address
126126
Collection<MetricData> actualMetrics = metricReader.collectAllMetrics();
127127

128128
assertThat(actualMetrics).isNotEmpty();
129-
MetricData metricData = actualMetrics.stream()
129+
MetricData metricData =
130+
actualMetrics.stream()
130131
.filter(metricData1 -> metricData1.getName().equals("gcp.client.request.duration"))
131132
.findFirst()
132133
.orElseThrow(() -> new AssertionError("Incorrect metric name"));
133134
assertThat(metricData.getInstrumentationScopeInfo().getName()).isEqualTo(SHOWCASE_ARTIFACT);
134135

135-
assertThat(metricData.getHistogramData().getPoints().iterator().next()
136-
.getAttributes()
137-
.get(AttributeKey.stringKey(ObservabilityAttributes.SERVER_ADDRESS_ATTRIBUTE)))
138-
.isEqualTo(SHOWCASE_SERVER_ADDRESS);
136+
assertThat(
137+
metricData
138+
.getHistogramData()
139+
.getPoints()
140+
.iterator()
141+
.next()
142+
.getAttributes()
143+
.get(AttributeKey.stringKey(ObservabilityAttributes.SERVER_ADDRESS_ATTRIBUTE)))
144+
.isEqualTo(SHOWCASE_SERVER_ADDRESS);
139145
}
140146
}
141147
}

‎sdk-platform-java/java-showcase/gapic-showcase/src/test/java/com/google/showcase/v1beta1/it/ITOtelTracing.java‎

Lines changed: 36 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -180,6 +180,21 @@ void testTracing_successfulEcho_grpc() throws Exception {
180180
.getAttributes()
181181
.get(AttributeKey.stringKey(ObservabilityAttributes.URL_DOMAIN_ATTRIBUTE)))
182182
.isEqualTo("showcase.googleapis.com");
183+
assertThat(
184+
attemptSpan
185+
.getAttributes()
186+
.get(AttributeKey.stringKey(ObservabilityAttributes.ERROR_TYPE_ATTRIBUTE)))
187+
.isNull();
188+
assertThat(
189+
attemptSpan
190+
.getAttributes()
191+
.get(AttributeKey.stringKey(ObservabilityAttributes.STATUS_MESSAGE_ATTRIBUTE)))
192+
.isNull();
193+
assertThat(
194+
attemptSpan
195+
.getAttributes()
196+
.get(AttributeKey.stringKey(ObservabilityAttributes.EXCEPTION_TYPE_ATTRIBUTE)))
197+
.isNull();
183198
assertThat(attemptSpan.getInstrumentationScopeInfo().getName()).isEqualTo(SHOWCASE_ARTIFACT);
184199
}
185200
}
@@ -256,6 +271,21 @@ void testTracing_successfulEcho_httpjson() throws Exception {
256271
.getAttributes()
257272
.get(AttributeKey.stringKey(ObservabilityAttributes.HTTP_URL_FULL_ATTRIBUTE)))
258273
.isEqualTo(SHOWCASE_USER_URL);
274+
assertThat(
275+
attemptSpan
276+
.getAttributes()
277+
.get(AttributeKey.stringKey(ObservabilityAttributes.ERROR_TYPE_ATTRIBUTE)))
278+
.isNull();
279+
assertThat(
280+
attemptSpan
281+
.getAttributes()
282+
.get(AttributeKey.stringKey(ObservabilityAttributes.STATUS_MESSAGE_ATTRIBUTE)))
283+
.isNull();
284+
assertThat(
285+
attemptSpan
286+
.getAttributes()
287+
.get(AttributeKey.stringKey(ObservabilityAttributes.EXCEPTION_TYPE_ATTRIBUTE)))
288+
.isNull();
259289
EchoResponse fetchedEcho = EchoResponse.newBuilder().setContent("tracing-test").build();
260290
long expectedMagnitude = computeExpectedHttpJsonResponseSize(fetchedEcho);
261291
Long observedMagnitude =
@@ -491,8 +521,9 @@ public void sendMessage(ReqT message) {}
491521

492522
assertThrows(UnavailableException.class, () -> client.echo(echoRequest));
493523
verifyErrorTypeAttribute("UNAVAILABLE");
494-
}
495-
}
524+
}
525+
}
526+
496527
@Test
497528
void testTracing_statusCodes_grpc() throws Exception {
498529
SpanTracerFactory tracingFactory = new SpanTracerFactory(openTelemetrySdk);
@@ -626,8 +657,9 @@ public String getHeaderValue(int index) {
626657

627658
assertThrows(UnavailableException.class, () -> client.echo(echoRequest));
628659
verifyErrorTypeAttribute("503");
629-
}
630-
}
660+
}
661+
}
662+
631663
void testTracing_statusCodes_httpjson() throws Exception {
632664
SpanTracerFactory tracingFactory = new SpanTracerFactory(openTelemetrySdk);
633665
EchoRequest errorRequest =

0 commit comments

Comments
 (0)