From 57b94a8e2eb8871a10fcfd53682cea33b13ca0a6 Mon Sep 17 00:00:00 2001 From: Liudmila Molkova Date: Thu, 21 Oct 2021 23:17:21 -0700 Subject: [PATCH] http-span-minor-fixes --- .../OpenTelemetryHttpPolicy.java | 14 ++--- .../OpenTelemetryHttpPolicyTests.java | 55 +++++++++++++++++-- 2 files changed, 58 insertions(+), 11 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 d08fd7c5c08c9..3a235fa341939 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; @@ -94,8 +93,11 @@ public Mono process(HttpPipelineCallContext context, HttpPipelineN Span parentSpan = (Span) context.getData(PARENT_SPAN_KEY).orElse(Span.current()); HttpRequest request = context.getHttpRequest(); - // Build new child span representing this outgoing request. + // Build new child span representing this outgoing request + // provide sampling-relevant attributes (users make sampling decisions based on this) SpanBuilder spanBuilder = tracer.spanBuilder("HTTP " + request.getHttpMethod().toString()) + .setAttribute(HTTP_METHOD, request.getHttpMethod().toString()) + .setAttribute(HTTP_URL, request.getUrl().toString()) .setParent(currentContext.with(parentSpan)); // A span's kind can be SERVER (incoming request) or CLIENT (outgoing request); @@ -104,9 +106,9 @@ public Mono process(HttpPipelineCallContext context, HttpPipelineN // Starting the span makes the sampling decision (nothing is logged at this time) Span span = spanBuilder.startSpan(); - // If span is sampled in, add additional TRACING attributes + // If span is sampled in, add additional attributes if (span.isRecording()) { - addSpanRequestAttributes(span, request, context); // Adds HTTP method, URL, & user-agent + addPostSamplingAttributes(span, request, context); } // For no-op tracer, SpanContext is INVALID; inject valid span headers onto outgoing request @@ -121,12 +123,10 @@ public Mono process(HttpPipelineCallContext context, HttpPipelineN .contextWrite(Context.of("TRACING_SPAN", span, "REQUEST", request)); } - private static void addSpanRequestAttributes(Span span, HttpRequest request, + private static void addPostSamplingAttributes(Span span, HttpRequest request, HttpPipelineCallContext context) { putAttributeIfNotEmptyOrNull(span, HTTP_USER_AGENT, request.getHeaders().getValue("User-Agent")); - putAttributeIfNotEmptyOrNull(span, HTTP_METHOD, request.getHttpMethod().toString()); - putAttributeIfNotEmptyOrNull(span, HTTP_URL, request.getUrl().toString()); Optional tracingNamespace = context.getData(AZ_TRACING_NAMESPACE_KEY); tracingNamespace.ifPresent(o -> putAttributeIfNotEmptyOrNull(span, OpenTelemetryTracer.AZ_NAMESPACE_KEY, o.toString())); 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 4925cf1710019..0b1e681249489 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 @@ -16,20 +16,24 @@ import com.azure.core.http.policy.RetryPolicy; import com.azure.core.test.http.MockHttpResponse; import com.azure.core.util.Context; +import io.opentelemetry.api.common.Attributes; import io.opentelemetry.api.trace.Span; +import io.opentelemetry.api.trace.SpanKind; import io.opentelemetry.api.trace.Tracer; import io.opentelemetry.sdk.OpenTelemetrySdk; import io.opentelemetry.sdk.common.CompletableResultCode; import io.opentelemetry.sdk.trace.ReadableSpan; import io.opentelemetry.sdk.trace.SdkTracerProvider; +import io.opentelemetry.sdk.trace.data.LinkData; import io.opentelemetry.sdk.trace.data.SpanData; import io.opentelemetry.sdk.trace.export.SimpleSpanProcessor; import io.opentelemetry.sdk.trace.export.SpanExporter; +import io.opentelemetry.sdk.trace.samplers.Sampler; +import io.opentelemetry.sdk.trace.samplers.SamplingDecision; +import io.opentelemetry.sdk.trace.samplers.SamplingResult; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.TestInfo; -import org.junit.jupiter.params.ParameterizedTest; -import org.junit.jupiter.params.provider.ValueSource; import reactor.core.publisher.Mono; import reactor.test.StepVerifier; @@ -38,12 +42,14 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.concurrent.atomic.AtomicBoolean; 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; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; /** * Unit tests for {@link OpenTelemetryHttpPolicy}. @@ -112,6 +118,48 @@ public void openTelemetryHttpPolicyTest() { assertEquals(X_MS_REQUEST_ID_1, httpAttributes.get("serviceRequestId")); } + @Test + public void presamplingAttributesArePopulatedBeforeSpanStarts() { + AtomicBoolean samplerCalled = new AtomicBoolean(); + SdkTracerProvider providerWithSampler = SdkTracerProvider.builder() + .setSampler(new Sampler() { + @Override + public SamplingResult shouldSample(io.opentelemetry.context.Context parentContext, String traceId, String name, SpanKind spanKind, Attributes attributes, List parentLinks) { + samplerCalled.set(true); + assertEquals(2, attributes.size()); + assertEquals("HTTP DELETE", name); + attributes.forEach((k, v) -> { + if (k.getKey() == "http.url") { + assertEquals("https://httpbin.org/hello?there#otel", v); + } else { + assertEquals("http.method", k.getKey()); + assertEquals("DELETE", v); + } + }); + + return SamplingResult.create(SamplingDecision.DROP); + } + + @Override + public String getDescription() { + return "test"; + } + }) + .addSpanProcessor(SimpleSpanProcessor.create(exporter)).build(); + + tracer = OpenTelemetrySdk.builder().setTracerProvider(providerWithSampler).build().getTracer("presampling-test"); + + // Act + HttpRequest request = new HttpRequest(HttpMethod.DELETE, "https://httpbin.org/hello?there#otel"); + HttpResponse response = createHttpPipeline(tracer).send(request).block(); + + // Assert + List exportedSpans = exporter.getSpans(); + // rest proxy span is not exported as global otel is not configured + assertEquals(0, exportedSpans.size()); + assertTrue(samplerCalled.get()); + } + @Test public void clientRequestIdIsStamped() { HttpRequest request = new HttpRequest(HttpMethod.PUT, "https://httpbin.org/hello?there#otel"); @@ -135,7 +183,7 @@ public void clientRequestIdIsStamped() { } @Test - public void everyTryIsTraced(){ + public void everyTryIsTraced() { AtomicInteger attemptCount = new AtomicInteger(); AtomicReference traceparentTry503 = new AtomicReference<>(); AtomicReference traceparentTry200 = new AtomicReference<>(); @@ -244,5 +292,4 @@ public List getSpans() { return exportedSpans; } } - }