From 331ce287d21b84617c371d035713fd7bff70a4a9 Mon Sep 17 00:00:00 2001 From: Mateusz Rzeszutek Date: Tue, 14 Dec 2021 10:07:18 +0100 Subject: [PATCH] Don't use HttpStatusConverter outside of instrumentation-api (#4870) * Don't use HttpStatusConverter outside of instrumentation-api * add comment --- .../http/HttpCommonAttributesExtractor.java | 2 +- .../HttpClientAttributesExtractorTest.java | 37 ++++++++++++++++--- .../HttpUrlConnectionInstrumentation.java | 14 +------ .../httpurlconnection/HttpUrlState.java | 2 + 4 files changed, 36 insertions(+), 19 deletions(-) diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpCommonAttributesExtractor.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpCommonAttributesExtractor.java index 5d1abb734def..10a01af0a3ef 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpCommonAttributesExtractor.java +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpCommonAttributesExtractor.java @@ -61,7 +61,7 @@ public void onEnd( if (response != null) { Integer statusCode = statusCode(request, response); - if (statusCode != null) { + if (statusCode != null && statusCode > 0) { set(attributes, SemanticAttributes.HTTP_STATUS_CODE, (long) statusCode); } set( diff --git a/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpClientAttributesExtractorTest.java b/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpClientAttributesExtractorTest.java index 60f8d4dbaad2..d98b5cbd51c0 100644 --- a/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpClientAttributesExtractorTest.java +++ b/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpClientAttributesExtractorTest.java @@ -7,6 +7,7 @@ import static io.opentelemetry.sdk.testing.assertj.OpenTelemetryAssertions.assertThat; import static java.util.Arrays.asList; +import static java.util.Collections.emptyList; import static java.util.Collections.singletonList; import static org.assertj.core.api.Assertions.entry; @@ -40,18 +41,21 @@ protected String url(Map request) { @Override protected List requestHeader(Map request, String name) { - return asList(request.get("header." + name).split(",")); + String value = request.get("header." + name); + return value == null ? emptyList() : asList(value.split(",")); } @Override protected Long requestContentLength(Map request, Map response) { - return Long.parseLong(request.get("requestContentLength")); + String value = request.get("requestContentLength"); + return value == null ? null : Long.parseLong(value); } @Override protected Long requestContentLengthUncompressed( Map request, Map response) { - return Long.parseLong(request.get("requestContentLengthUncompressed")); + String value = request.get("requestContentLengthUncompressed"); + return value == null ? null : Long.parseLong(value); } @Override @@ -67,19 +71,22 @@ protected String flavor(Map request, Map respons @Override protected Long responseContentLength( Map request, Map response) { - return Long.parseLong(response.get("responseContentLength")); + String value = response.get("responseContentLength"); + return value == null ? null : Long.parseLong(value); } @Override protected Long responseContentLengthUncompressed( Map request, Map response) { - return Long.parseLong(response.get("responseContentLengthUncompressed")); + String value = response.get("responseContentLengthUncompressed"); + return value == null ? null : Long.parseLong(value); } @Override protected List responseHeader( Map request, Map response, String name) { - return asList(response.get("header." + name).split(",")); + String value = response.get("header." + name); + return value == null ? emptyList() : asList(value.split(",")); } } @@ -135,4 +142,22 @@ void normal() { AttributeKey.stringArrayKey("http.response.header.custom_response_header"), asList("654", "321"))); } + + @Test + void invalidStatusCode() { + Map request = new HashMap<>(); + + Map response = new HashMap<>(); + response.put("statusCode", "0"); + + TestHttpClientAttributesExtractor extractor = + new TestHttpClientAttributesExtractor(CapturedHttpHeaders.empty()); + + AttributesBuilder attributes = Attributes.builder(); + extractor.onStart(attributes, request); + assertThat(attributes.build()).isEmpty(); + + extractor.onEnd(attributes, request, response, null); + assertThat(attributes.build()).isEmpty(); + } } diff --git a/instrumentation/http-url-connection/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/httpurlconnection/HttpUrlConnectionInstrumentation.java b/instrumentation/http-url-connection/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/httpurlconnection/HttpUrlConnectionInstrumentation.java index 107eadbeb4b2..136fa218d124 100644 --- a/instrumentation/http-url-connection/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/httpurlconnection/HttpUrlConnectionInstrumentation.java +++ b/instrumentation/http-url-connection/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/httpurlconnection/HttpUrlConnectionInstrumentation.java @@ -15,17 +15,12 @@ import static net.bytebuddy.matcher.ElementMatchers.namedOneOf; import static net.bytebuddy.matcher.ElementMatchers.not; -import io.opentelemetry.api.trace.Span; -import io.opentelemetry.api.trace.StatusCode; import io.opentelemetry.context.Context; import io.opentelemetry.context.Scope; import io.opentelemetry.instrumentation.api.field.VirtualField; -import io.opentelemetry.instrumentation.api.tracer.HttpStatusConverter; import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation; import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer; import io.opentelemetry.javaagent.instrumentation.api.CallDepth; -import io.opentelemetry.javaagent.instrumentation.api.Java8BytecodeBridge; -import io.opentelemetry.semconv.trace.attributes.SemanticAttributes; import java.net.HttpURLConnection; import net.bytebuddy.asm.Advice; import net.bytebuddy.description.type.TypeDescription; @@ -137,7 +132,7 @@ public static void methodExit( .end( httpUrlState.context, connection, - responseCode > 0 ? responseCode : null, + responseCode > 0 ? responseCode : httpUrlState.statusCode, throwable); } httpUrlState.finished = true; @@ -167,12 +162,7 @@ public static void methodExit( VirtualField.find(HttpURLConnection.class, HttpUrlState.class); HttpUrlState httpUrlState = storage.get(connection); if (httpUrlState != null) { - Span span = Java8BytecodeBridge.spanFromContext(httpUrlState.context); - span.setAttribute(SemanticAttributes.HTTP_STATUS_CODE, returnValue); - StatusCode statusCode = HttpStatusConverter.CLIENT.statusFromHttpStatus(returnValue); - if (statusCode != StatusCode.UNSET) { - span.setStatus(statusCode); - } + httpUrlState.statusCode = returnValue; } } } diff --git a/instrumentation/http-url-connection/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/httpurlconnection/HttpUrlState.java b/instrumentation/http-url-connection/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/httpurlconnection/HttpUrlState.java index c0e095cff635..8b9f929df9fc 100644 --- a/instrumentation/http-url-connection/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/httpurlconnection/HttpUrlState.java +++ b/instrumentation/http-url-connection/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/httpurlconnection/HttpUrlState.java @@ -12,6 +12,8 @@ public class HttpUrlState { public final Context context; public boolean finished; + // by default 0 is ignored + public int statusCode = 0; public HttpUrlState(Context context) { this.context = context;