From 7a5d4ab4d6c00eee0ad2e201869c377a4f053f70 Mon Sep 17 00:00:00 2001 From: Liudmila Molkova Date: Fri, 22 Oct 2021 00:07:08 -0700 Subject: [PATCH] Http spans minor fixes (#24954) * Add OpenTelemtery HTTP policy test for retries * Fix minor inconsistencies in span attribute and name * fix build --- .../OpenTelemetryHttpPolicy.java | 19 ++-- .../OpenTelemetryHttpPolicyTests.java | 105 ++++++++++++++++-- 2 files changed, 109 insertions(+), 15 deletions(-) diff --git a/sdk/core/azure-core-tracing-opentelemetry/src/main/java/com/azure/core/tracing/opentelemetry/OpenTelemetryHttpPolicy.java b/sdk/core/azure-core-tracing-opentelemetry/src/main/java/com/azure/core/tracing/opentelemetry/OpenTelemetryHttpPolicy.java index 1ca4ed9138e05..6dc0f3aeae5cd 100644 --- a/sdk/core/azure-core-tracing-opentelemetry/src/main/java/com/azure/core/tracing/opentelemetry/OpenTelemetryHttpPolicy.java +++ b/sdk/core/azure-core-tracing-opentelemetry/src/main/java/com/azure/core/tracing/opentelemetry/OpenTelemetryHttpPolicy.java @@ -12,7 +12,6 @@ import com.azure.core.http.policy.HttpPipelinePolicy; import com.azure.core.tracing.opentelemetry.implementation.HttpTraceUtil; import com.azure.core.util.CoreUtils; -import com.azure.core.util.UrlBuilder; import io.opentelemetry.api.GlobalOpenTelemetry; import io.opentelemetry.api.common.AttributeKey; import io.opentelemetry.api.trace.Span; @@ -73,7 +72,12 @@ public OpenTelemetryHttpPolicy() { private static final String HTTP_METHOD = "http.method"; private static final String HTTP_URL = "http.url"; private static final String HTTP_STATUS_CODE = "http.status_code"; - private static final String REQUEST_ID = "x-ms-request-id"; + private static final String SERVICE_REQUEST_ID_HEADER = "x-ms-request-id"; + private static final String SERVICE_REQUEST_ID_ATTRIBUTE = "serviceRequestId"; + + private static final String CLIENT_REQUEST_ID_HEADER = "x-ms-client-request-id"; + private static final String CLIENT_REQUEST_ID_ATTRIBUTE = "requestId"; + // This helper class implements W3C distributed tracing protocol and injects SpanContext into the outgoing http // request @@ -90,9 +94,7 @@ public Mono process(HttpPipelineCallContext context, HttpPipelineN HttpRequest request = context.getHttpRequest(); // Build new child span representing this outgoing request. - final UrlBuilder urlBuilder = UrlBuilder.parse(context.getHttpRequest().getUrl()); - - SpanBuilder spanBuilder = tracer.spanBuilder(urlBuilder.getPath()) + SpanBuilder spanBuilder = tracer.spanBuilder("HTTP " + request.getHttpMethod().toString()) .setParent(currentContext.with(parentSpan)); // A span's kind can be SERVER (incoming request) or CLIENT (outgoing request); @@ -127,6 +129,9 @@ private static void addSpanRequestAttributes(Span span, HttpRequest request, Optional tracingNamespace = context.getData(AZ_TRACING_NAMESPACE_KEY); tracingNamespace.ifPresent(o -> putAttributeIfNotEmptyOrNull(span, OpenTelemetryTracer.AZ_NAMESPACE_KEY, o.toString())); + + String requestId = request.getHeaders().getValue(CLIENT_REQUEST_ID_HEADER); + putAttributeIfNotEmptyOrNull(span, CLIENT_REQUEST_ID_ATTRIBUTE, requestId); } private static void putAttributeIfNotEmptyOrNull(Span span, String key, String value) { @@ -183,10 +188,10 @@ private static void spanEnd(Span span, HttpResponse response, Throwable error) { String requestId = null; if (response != null) { statusCode = response.getStatusCode(); - requestId = response.getHeaderValue(REQUEST_ID); + requestId = response.getHeaderValue(SERVICE_REQUEST_ID_HEADER); } - putAttributeIfNotEmptyOrNull(span, REQUEST_ID, requestId); + putAttributeIfNotEmptyOrNull(span, SERVICE_REQUEST_ID_ATTRIBUTE, requestId); span.setAttribute(HTTP_STATUS_CODE, statusCode); span = HttpTraceUtil.setSpanStatus(span, statusCode, error); } diff --git a/sdk/core/azure-core-tracing-opentelemetry/src/test/java/com/azure/core/tracing/opentelemetry/OpenTelemetryHttpPolicyTests.java b/sdk/core/azure-core-tracing-opentelemetry/src/test/java/com/azure/core/tracing/opentelemetry/OpenTelemetryHttpPolicyTests.java index d40efde969d46..6daa5ff465d25 100644 --- a/sdk/core/azure-core-tracing-opentelemetry/src/test/java/com/azure/core/tracing/opentelemetry/OpenTelemetryHttpPolicyTests.java +++ b/sdk/core/azure-core-tracing-opentelemetry/src/test/java/com/azure/core/tracing/opentelemetry/OpenTelemetryHttpPolicyTests.java @@ -12,6 +12,8 @@ import com.azure.core.http.HttpResponse; import com.azure.core.http.policy.HttpPipelinePolicy; import com.azure.core.http.policy.HttpPolicyProviders; +import com.azure.core.http.policy.RequestIdPolicy; +import com.azure.core.http.policy.RetryPolicy; import com.azure.core.test.http.MockHttpResponse; import com.azure.core.util.Context; import io.opentelemetry.api.trace.Span; @@ -27,12 +29,15 @@ import org.junit.jupiter.api.Test; import org.junit.jupiter.api.TestInfo; import reactor.core.publisher.Mono; +import reactor.test.StepVerifier; import java.util.ArrayList; import java.util.Collection; import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicReference; import static com.azure.core.util.tracing.Tracer.AZ_TRACING_NAMESPACE_KEY; import static com.azure.core.util.tracing.Tracer.PARENT_SPAN_KEY; @@ -43,7 +48,8 @@ */ public class OpenTelemetryHttpPolicyTests { - private static final String X_MS_REQUEST_ID = "response id"; + private static final String X_MS_REQUEST_ID_1 = "response id 1"; + private static final String X_MS_REQUEST_ID_2 = "response id 2"; private static final int RESPONSE_STATUS_CODE = 201; private TestExporter exporter; private Tracer tracer; @@ -78,7 +84,7 @@ public void openTelemetryHttpPolicyTest() { .addData(AZ_TRACING_NAMESPACE_KEY, "foo"); // Act - HttpRequest request = new HttpRequest(HttpMethod.GET, "https://httpbin.org/hello?there#otel"); + HttpRequest request = new HttpRequest(HttpMethod.POST, "https://httpbin.org/hello?there#otel"); request.setHeader("User-Agent", "user-agent"); HttpResponse response = createHttpPipeline(tracer).send(request, tracingContext).block(); @@ -91,17 +97,99 @@ public void openTelemetryHttpPolicyTest() { assertEquals(request.getHeaders().getValue("Traceparent"), String.format("00-%s-%s-01", httpSpan.getTraceId(), httpSpan.getSpanId())); assertEquals(((ReadableSpan) parentSpan).getSpanContext().getSpanId(), httpSpan.getParentSpanId()); - assertEquals("/hello", httpSpan.getName()); + assertEquals("HTTP POST", httpSpan.getName()); Map httpAttributes = getAttributes(httpSpan); assertEquals(6, httpAttributes.size()); assertEquals("https://httpbin.org/hello?there#otel", httpAttributes.get("http.url")); - assertEquals("GET", httpAttributes.get("http.method")); + assertEquals("POST", httpAttributes.get("http.method")); assertEquals("user-agent", httpAttributes.get("http.user_agent")); assertEquals("foo", httpAttributes.get(AZ_TRACING_NAMESPACE_KEY)); assertEquals(Long.valueOf(RESPONSE_STATUS_CODE), httpAttributes.get("http.status_code")); - assertEquals(X_MS_REQUEST_ID, httpAttributes.get("x-ms-request-id")); + assertEquals(X_MS_REQUEST_ID_1, httpAttributes.get("serviceRequestId")); + } + + @Test + public void clientRequestIdIsStamped() { + HttpRequest request = new HttpRequest(HttpMethod.PUT, "https://httpbin.org/hello?there#otel"); + HttpResponse response = createHttpPipeline(tracer, new RequestIdPolicy()).send(request).block(); + + // Assert + List exportedSpans = exporter.getSpans(); + assertEquals(1, exportedSpans.size()); + + assertEquals("HTTP PUT", exportedSpans.get(0).getName()); + + Map httpAttributes = getAttributes(exportedSpans.get(0)); + assertEquals(5, httpAttributes.size()); + + assertEquals(response.getRequest().getHeaders().getValue("x-ms-client-request-id"), httpAttributes.get("requestId")); + assertEquals(X_MS_REQUEST_ID_1, httpAttributes.get("serviceRequestId")); + + assertEquals("https://httpbin.org/hello?there#otel", httpAttributes.get("http.url")); + assertEquals("PUT", httpAttributes.get("http.method")); + assertEquals(Long.valueOf(RESPONSE_STATUS_CODE), httpAttributes.get("http.status_code")); + } + + @Test + public void everyTryIsTraced() { + AtomicInteger attemptCount = new AtomicInteger(); + AtomicReference traceparentTry503 = new AtomicReference<>(); + AtomicReference traceparentTry200 = new AtomicReference<>(); + + HttpPipeline pipeline = new HttpPipelineBuilder() + .policies(new RetryPolicy()) + .policies(new OpenTelemetryHttpPolicy(tracer)) + .httpClient(request -> { + HttpHeaders headers = new HttpHeaders(); + + int count = attemptCount.getAndIncrement(); + if (count == 0) { + traceparentTry503.set(request.getHeaders().getValue("traceparent")); + headers.set("x-ms-request-id", X_MS_REQUEST_ID_1); + return Mono.just(new MockHttpResponse(request, 503, headers)); + } else if (count == 1) { + traceparentTry200.set(request.getHeaders().getValue("traceparent")); + headers.set("x-ms-request-id", X_MS_REQUEST_ID_2); + return Mono.just(new MockHttpResponse(request, 200, headers)); + } else { + // Too many requests have been made. + return Mono.just(new MockHttpResponse(request, 400, headers)); + } + }) + .build(); + + // Start user parent span and populate context. + Span parentSpan = tracer.spanBuilder(PARENT_SPAN_KEY).startSpan(); + + Context tracingContext = new Context(PARENT_SPAN_KEY, parentSpan) + .addData(AZ_TRACING_NAMESPACE_KEY, "foo"); + + StepVerifier.create(pipeline.send(new HttpRequest(HttpMethod.GET, "http://localhost/hello"), tracingContext)) + .assertNext(response -> assertEquals(200, response.getStatusCode())) + .verifyComplete(); + + List exportedSpans = exporter.getSpans(); + assertEquals(2, exportedSpans.size()); + + SpanData try503 = exportedSpans.get(0); + SpanData try200 = exportedSpans.get(1); + + assertEquals(traceparentTry503.get(), String.format("00-%s-%s-01", try503.getTraceId(), try503.getSpanId())); + assertEquals(traceparentTry200.get(), String.format("00-%s-%s-01", try200.getTraceId(), try200.getSpanId())); + + assertEquals("HTTP GET", try503.getName()); + Map httpAttributes503 = getAttributes(try503); + assertEquals(5, httpAttributes503.size()); + assertEquals(Long.valueOf(503), httpAttributes503.get("http.status_code")); + assertEquals(X_MS_REQUEST_ID_1, httpAttributes503.get("serviceRequestId")); + + assertEquals("HTTP GET", try503.getName()); + Map httpAttributes200 = getAttributes(try200); + assertEquals(5, httpAttributes200.size()); + assertEquals(Long.valueOf(200), httpAttributes200.get("http.status_code")); + assertEquals(X_MS_REQUEST_ID_2, httpAttributes200.get("serviceRequestId")); } private Map getAttributes(SpanData span) { @@ -111,10 +199,11 @@ private Map getAttributes(SpanData span) { return attributes; } - private static HttpPipeline createHttpPipeline(Tracer tracer) { + private static HttpPipeline createHttpPipeline(Tracer tracer, HttpPipelinePolicy... beforeRetryPolicies) { final HttpPipeline httpPipeline = new HttpPipelineBuilder() - .httpClient(new SimpleMockHttpClient()) + .policies(beforeRetryPolicies) .policies(new OpenTelemetryHttpPolicy(tracer)) + .httpClient(new SimpleMockHttpClient()) .build(); return httpPipeline; } @@ -124,7 +213,7 @@ private static class SimpleMockHttpClient implements HttpClient { @Override public Mono send(HttpRequest request) { HttpHeaders headers = new HttpHeaders() - .set("x-ms-request-id", X_MS_REQUEST_ID); + .set("x-ms-request-id", X_MS_REQUEST_ID_1); return Mono.just(new MockHttpResponse(request, RESPONSE_STATUS_CODE, headers)); }