From ac63dd099eaff57b0f4b0bd4997bfd7c3217c8e8 Mon Sep 17 00:00:00 2001 From: Rishabh Singh Date: Fri, 8 Jan 2021 16:48:39 +0530 Subject: [PATCH] 1. Update HttpFieldsGenerator#maybeSetHttpUrlForOtelFormat to overwrite relative url 2. Update the logic to fetch http url in SpanEventViewGenerator --- .../generators/SpanEventViewGenerator.java | 7 +++- .../SpanEventViewGeneratorTest.java | 25 +++++++++++ .../fieldgenerators/HttpFieldsGenerator.java | 20 +++++++-- .../HttpFieldsGeneratorTest.java | 41 ++++++++++++++++++- 4 files changed, 88 insertions(+), 5 deletions(-) diff --git a/hypertrace-view-generator/hypertrace-view-generator/src/main/java/org/hypertrace/viewgenerator/generators/SpanEventViewGenerator.java b/hypertrace-view-generator/hypertrace-view-generator/src/main/java/org/hypertrace/viewgenerator/generators/SpanEventViewGenerator.java index d1e6fa40f..1e78944fe 100644 --- a/hypertrace-view-generator/hypertrace-view-generator/src/main/java/org/hypertrace/viewgenerator/generators/SpanEventViewGenerator.java +++ b/hypertrace-view-generator/hypertrace-view-generator/src/main/java/org/hypertrace/viewgenerator/generators/SpanEventViewGenerator.java @@ -6,12 +6,15 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.Optional; import java.util.stream.Collectors; import org.apache.avro.Schema; import org.hypertrace.core.datamodel.Entity; import org.hypertrace.core.datamodel.Event; import org.hypertrace.core.datamodel.MetricValue; import org.hypertrace.core.datamodel.StructuredTrace; +import org.hypertrace.core.datamodel.eventfields.http.Http; +import org.hypertrace.core.datamodel.eventfields.http.Request; import org.hypertrace.core.datamodel.shared.SpanAttributeUtils; import org.hypertrace.traceenricher.enrichedspan.constants.EnrichedSpanConstants; import org.hypertrace.traceenricher.enrichedspan.constants.utils.EnrichedSpanUtils; @@ -292,7 +295,9 @@ String getRequestUrl(Event event, Protocol protocol) { switch (protocol) { case PROTOCOL_HTTP: case PROTOCOL_HTTPS: - return EnrichedSpanUtils.getFullHttpUrl(event).orElse(event.getHttp().getRequest().getPath()); + return EnrichedSpanUtils.getFullHttpUrl(event).orElse( + Optional.ofNullable(event.getHttp()).map(Http::getRequest).map(Request::getPath) + .orElse(null)); case PROTOCOL_GRPC: return event.getEventName(); } diff --git a/hypertrace-view-generator/hypertrace-view-generator/src/test/java/org/hypertrace/viewgenerator/generators/SpanEventViewGeneratorTest.java b/hypertrace-view-generator/hypertrace-view-generator/src/test/java/org/hypertrace/viewgenerator/generators/SpanEventViewGeneratorTest.java index 8e89ce59d..1f01c47de 100644 --- a/hypertrace-view-generator/hypertrace-view-generator/src/test/java/org/hypertrace/viewgenerator/generators/SpanEventViewGeneratorTest.java +++ b/hypertrace-view-generator/hypertrace-view-generator/src/test/java/org/hypertrace/viewgenerator/generators/SpanEventViewGeneratorTest.java @@ -64,4 +64,29 @@ public void test_getRequestUrl_grpcProctol_shouldReturnEventName() { spanEventViewGenerator.getRequestUrl(event, Protocol.PROTOCOL_GRPC) ); } + + @Test + public void testGetRequestUrl_fullUrlIsAbsent() { + Event event = mock(Event.class); + when(event.getHttp()).thenReturn(Http.newBuilder() + .setRequest(Request.newBuilder() + .setPath("/api/v1/gatekeeper/check") + .build() + ).build() + ); + Assertions.assertEquals( + "/api/v1/gatekeeper/check", + spanEventViewGenerator.getRequestUrl(event, Protocol.PROTOCOL_HTTP)); + } + + @Test + public void testGetRequestUrl_urlAndPathIsAbsent() { + Event event = mock(Event.class); + when(event.getHttp()).thenReturn(Http.newBuilder() + .setRequest(Request.newBuilder() + .build() + ).build() + ); + Assertions.assertNull(spanEventViewGenerator.getRequestUrl(event, Protocol.PROTOCOL_HTTP)); + } } diff --git a/span-normalizer/span-normalizer/src/main/java/org/hypertrace/core/spannormalizer/fieldgenerators/HttpFieldsGenerator.java b/span-normalizer/span-normalizer/src/main/java/org/hypertrace/core/spannormalizer/fieldgenerators/HttpFieldsGenerator.java index f4bc56b4b..1b4ab5112 100644 --- a/span-normalizer/span-normalizer/src/main/java/org/hypertrace/core/spannormalizer/fieldgenerators/HttpFieldsGenerator.java +++ b/span-normalizer/span-normalizer/src/main/java/org/hypertrace/core/spannormalizer/fieldgenerators/HttpFieldsGenerator.java @@ -446,7 +446,7 @@ private static void setUserAgent( .ifPresent(userAgent -> httpBuilder.getRequestBuilder().setUserAgent(userAgent)); } - private static void setPath( + static void setPath( Http.Builder httpBuilder, Map tagsMap) { if (httpBuilder.getRequestBuilder().hasPath()) { return; @@ -469,7 +469,7 @@ private static String removeTrailingSlash(String s) { return s.endsWith(SLASH) && s.length() > 1 ? s.substring(0, s.length() - 1) : s; } - private static Optional getPathFromUrlObject(String urlPath) { + static Optional getPathFromUrlObject(String urlPath) { try { URL url = getNormalizedUrl(urlPath); return Optional.of(url.getPath()); @@ -569,13 +569,27 @@ private static URL getNormalizedUrl(String url) throws MalformedURLException { return new URL(new URL(RELATIVE_URL_CONTEXT), url); } + /** + * If the requestBuilder already has absolute url, do nothing + * if not, try building url based on otel attributes and overwrite + */ private void maybeSetHttpUrlForOtelFormat( Request.Builder requestBuilder, final Map attributeValueMap) { - if (requestBuilder.hasUrl()) { + if (requestBuilder.hasUrl() && isAbsoluteUrl(requestBuilder.getUrl())) { return; } Optional url = HttpSemanticConventionUtils.getHttpUrlForOTelFormat(attributeValueMap); url.ifPresent(requestBuilder::setUrl); } + + static boolean isAbsoluteUrl(String urlStr) { + try { + URL url = getNormalizedUrl(urlStr); + return url.toString().equals(urlStr); + } catch (MalformedURLException e) { + // ignore + } + return false; + } } diff --git a/span-normalizer/span-normalizer/src/test/java/org/hypertrace/core/spannormalizer/fieldgenerators/HttpFieldsGeneratorTest.java b/span-normalizer/span-normalizer/src/test/java/org/hypertrace/core/spannormalizer/fieldgenerators/HttpFieldsGeneratorTest.java index 259238fe4..e25aeaa2d 100644 --- a/span-normalizer/span-normalizer/src/test/java/org/hypertrace/core/spannormalizer/fieldgenerators/HttpFieldsGeneratorTest.java +++ b/span-normalizer/span-normalizer/src/test/java/org/hypertrace/core/spannormalizer/fieldgenerators/HttpFieldsGeneratorTest.java @@ -38,6 +38,8 @@ import static org.hypertrace.core.span.constants.v1.OTSpanTag.OT_SPAN_TAG_HTTP_URL; import static org.hypertrace.core.spannormalizer.utils.TestUtils.createKeyValue; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; import com.google.common.collect.Maps; import io.jaegertracing.api_v2.JaegerSpanInternalModel; @@ -46,9 +48,12 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.Optional; import org.hypertrace.core.datamodel.AttributeValue; import org.hypertrace.core.datamodel.Event; import org.hypertrace.core.datamodel.eventfields.http.Http; +import org.hypertrace.core.datamodel.eventfields.http.Request; +import org.hypertrace.core.semantic.convention.constants.span.OTelSpanSemanticConventions; import org.hypertrace.core.span.constants.RawSpanConstants; import org.hypertrace.core.semantic.convention.constants.http.OTelHttpSemanticConventions; import org.junit.jupiter.api.Assertions; @@ -836,7 +841,6 @@ public void testPopulateOtherFieldsOTelSpan() { eventBuilder = Event.newBuilder(); eventBuilder.getHttpBuilder().getRequestBuilder().setUrl("/apis/5673/events?a1=v1&a2=v2"); map = Maps.newHashMap(); - map.put(OTelHttpSemanticConventions.HTTP_URL.getValue(), buildAttributeValue(url)); httpFieldsGenerator.populateOtherFields(eventBuilder, map); Assertions.assertNull(eventBuilder.getHttpBuilder().getRequestBuilder().getUrl()); @@ -876,6 +880,41 @@ public void testPopulateOtherFieldsOTelSpan() { "/some-test-path", eventBuilder.getHttpBuilder().getRequestBuilder().getPath()); assertEquals( "some-query-str=v1", eventBuilder.getHttpBuilder().getRequestBuilder().getQueryString()); + + // originally set url is a relative url, should be overridden + eventBuilder = Event.newBuilder(); + eventBuilder.getHttpBuilder().getRequestBuilder().setUrl("/api/v1/gatekeeper/check?url=%2Fpixel%2Factivities%3Fadvertisable%3DTRHRT&method=GET&service=pixel"); + map = Maps.newHashMap(); + map.put(OTelHttpSemanticConventions.HTTP_SCHEME.getValue(), buildAttributeValue("http")); + map.put(OTelSpanSemanticConventions.SPAN_KIND.getValue(), buildAttributeValue("server")); + map.put(OTelHttpSemanticConventions.HTTP_NET_HOST_NAME.getValue(), buildAttributeValue("example.internal.com")); + map.put(OTelHttpSemanticConventions.HTTP_NET_HOST_PORT.getValue(), buildAttributeValue("50850")); + map.put(OTelHttpSemanticConventions.HTTP_TARGET.getValue(), buildAttributeValue("/api/v1/gatekeeper/check?url=%2Fpixel%2Factivities%3Fadvertisable%3DTRHRT&method=GET&service=pixel")); + httpFieldsGenerator.populateOtherFields(eventBuilder, map); + assertEquals("http://example.internal.com:50850/api/v1/gatekeeper/check?url=%2Fpixel%2Factivities%3Fadvertisable%3DTRHRT&method=GET&service=pixel", eventBuilder.getHttpBuilder().getRequestBuilder().getUrl()); + } + + @Test + public void testIsAbsoluteUrl() { + assertTrue(HttpFieldsGenerator.isAbsoluteUrl("http://example.com/abc/xyz")); + assertFalse(HttpFieldsGenerator.isAbsoluteUrl("/abc/xyz")); + } + + @Test + public void testGetPathFromUrl() { + Optional path = HttpFieldsGenerator.getPathFromUrlObject( + "/api/v1/gatekeeper/check?url=%2Fpixel%2Factivities%3Fadvertisable%3DTRHRT&method=GET&service=pixel"); + assertEquals(path.get(), "/api/v1/gatekeeper/check"); + } + + @Test + public void testSetPath() { + Map tagsMap = new HashMap<>(); + tagsMap.put(OTelHttpSemanticConventions.HTTP_TARGET.getValue(), + createKeyValue("/api/v1/gatekeeper/check?url=%2Fpixel%2Factivities%3Fadvertisable%3DTRHRT&method=GET&service=pixel")); + Http.Builder builder = Http.newBuilder().setRequestBuilder(Request.newBuilder()); + HttpFieldsGenerator.setPath(builder, tagsMap); + assertEquals(builder.getRequestBuilder().getPath(), "/api/v1/gatekeeper/check"); } private static AttributeValue buildAttributeValue(String value) {