From b9b2fc38814a8afd85c6ab009e55484f541b47bc Mon Sep 17 00:00:00 2001 From: Atharva Sharma <60044988+atharvasharma61@users.noreply.github.com> Date: Wed, 7 Feb 2024 10:01:54 +0530 Subject: [PATCH] [BugFix] Fixing Span operation names generated from RestActions (#12005) * bug fix to have RestActions rawPath as operation names, query params as tags and avoid using exact URI (#10791) Signed-off-by: Atharva Sharma * Added UTs Signed-off-by: Atharva Sharma * Added parameterized tests Signed-off-by: Atharva Sharma * improvised parameterized tests using annotations Signed-off-by: Atharva Sharma * removed unused annotations Signed-off-by: Atharva Sharma * Added details in CHANGELOG.md Signed-off-by: Atharva Sharma * Addressed spotlessJavaCheck failures Signed-off-by: Atharva Sharma --------- Signed-off-by: Atharva Sharma Signed-off-by: Atharva Sharma <60044988+atharvasharma61@users.noreply.github.com> Co-authored-by: Atharva Sharma (cherry picked from commit c0fca74709e96ec7c8cf295cdca6110946b592b8) --- CHANGELOG.md | 1 + .../telemetry/tracing/AttributeNames.java | 5 ++ .../telemetry/tracing/SpanBuilder.java | 36 +++++++++++-- .../telemetry/tracing/SpanBuilderTests.java | 53 ++++++++++++++++--- 4 files changed, 82 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index db815f63b5541..7126f6076b584 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -176,6 +176,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - [BUG] Fix remote shards balancer when filtering throttled nodes ([#11724](https://github.com/opensearch-project/OpenSearch/pull/11724)) - [Bug] Check phase name before SearchRequestOperationsListener onPhaseStart ([#12094](https://github.com/opensearch-project/OpenSearch/pull/12094)) - Add advance(int) for numeric values in order to allow point based optimization to kick in ([#12089](https://github.com/opensearch-project/OpenSearch/pull/12089)) +- Fix Span operation names generated from RestActions ([#12005](https://github.com/opensearch-project/OpenSearch/pull/12005)) ### Security diff --git a/server/src/main/java/org/opensearch/telemetry/tracing/AttributeNames.java b/server/src/main/java/org/opensearch/telemetry/tracing/AttributeNames.java index b6b2cf360d1c5..6a97914b04ebc 100644 --- a/server/src/main/java/org/opensearch/telemetry/tracing/AttributeNames.java +++ b/server/src/main/java/org/opensearch/telemetry/tracing/AttributeNames.java @@ -40,6 +40,11 @@ private AttributeNames() { */ public static final String HTTP_URI = "http.uri"; + /** + * Http Request Query Parameters. + */ + public static final String HTTP_REQ_QUERY_PARAMS = "url.query"; + /** * Rest Request ID. */ diff --git a/server/src/main/java/org/opensearch/telemetry/tracing/SpanBuilder.java b/server/src/main/java/org/opensearch/telemetry/tracing/SpanBuilder.java index 1dce422943b7a..70658c5d71bf3 100644 --- a/server/src/main/java/org/opensearch/telemetry/tracing/SpanBuilder.java +++ b/server/src/main/java/org/opensearch/telemetry/tracing/SpanBuilder.java @@ -11,6 +11,7 @@ import org.opensearch.action.bulk.BulkShardRequest; import org.opensearch.action.support.replication.ReplicatedWriteRequest; import org.opensearch.common.annotation.InternalApi; +import org.opensearch.common.collect.Tuple; import org.opensearch.core.common.Strings; import org.opensearch.http.HttpRequest; import org.opensearch.rest.RestRequest; @@ -75,7 +76,9 @@ public static SpanCreationContext from(String spanName, String nodeId, Replicate } private static String createSpanName(HttpRequest httpRequest) { - return httpRequest.method().name() + SEPARATOR + httpRequest.uri(); + Tuple uriParts = splitUri(httpRequest.uri()); + String path = uriParts.v1(); + return httpRequest.method().name() + SEPARATOR + path; } private static Attributes buildSpanAttributes(HttpRequest httpRequest) { @@ -84,9 +87,26 @@ private static Attributes buildSpanAttributes(HttpRequest httpRequest) { .addAttribute(AttributeNames.HTTP_METHOD, httpRequest.method().name()) .addAttribute(AttributeNames.HTTP_PROTOCOL_VERSION, httpRequest.protocolVersion().name()); populateHeader(httpRequest, attributes); + + Tuple uriParts = splitUri(httpRequest.uri()); + String query = uriParts.v2(); + if (query.isBlank() == false) { + attributes.addAttribute(AttributeNames.HTTP_REQ_QUERY_PARAMS, query); + } + return attributes; } + private static Tuple splitUri(String uri) { + int index = uri.indexOf('?'); + if (index >= 0 && index < uri.length() - 1) { + String path = uri.substring(0, index); + String query = uri.substring(index + 1); + return new Tuple<>(path, query); + } + return new Tuple<>(uri, ""); + } + private static void populateHeader(HttpRequest httpRequest, Attributes attributes) { HEADERS_TO_BE_ADDED_AS_ATTRIBUTES.forEach(x -> { if (httpRequest.getHeaders() != null @@ -102,9 +122,8 @@ private static String createSpanName(RestRequest restRequest) { if (restRequest != null) { try { String methodName = restRequest.method().name(); - // path() does the decoding, which may give error - String path = restRequest.path(); - spanName = methodName + SEPARATOR + path; + String rawPath = restRequest.rawPath(); + spanName = methodName + SEPARATOR + rawPath; } catch (Exception e) { // swallow the exception and keep the default name. } @@ -114,9 +133,16 @@ private static String createSpanName(RestRequest restRequest) { private static Attributes buildSpanAttributes(RestRequest restRequest) { if (restRequest != null) { - return Attributes.create() + Attributes attributes = Attributes.create() .addAttribute(AttributeNames.REST_REQ_ID, restRequest.getRequestId()) .addAttribute(AttributeNames.REST_REQ_RAW_PATH, restRequest.rawPath()); + + Tuple uriParts = splitUri(restRequest.uri()); + String query = uriParts.v2(); + if (query.isBlank() == false) { + attributes.addAttribute(AttributeNames.HTTP_REQ_QUERY_PARAMS, query); + } + return attributes; } else { return Attributes.EMPTY; } diff --git a/server/src/test/java/org/opensearch/telemetry/tracing/SpanBuilderTests.java b/server/src/test/java/org/opensearch/telemetry/tracing/SpanBuilderTests.java index b4183412cdf02..75fc6761a60ef 100644 --- a/server/src/test/java/org/opensearch/telemetry/tracing/SpanBuilderTests.java +++ b/server/src/test/java/org/opensearch/telemetry/tracing/SpanBuilderTests.java @@ -8,6 +8,8 @@ package org.opensearch.telemetry.tracing; +import com.carrotsearch.randomizedtesting.annotations.ParametersFactory; + import org.opensearch.Version; import org.opensearch.cluster.node.DiscoveryNode; import org.opensearch.common.network.NetworkAddress; @@ -27,29 +29,64 @@ import java.io.IOException; import java.util.Arrays; +import java.util.Collection; import java.util.List; import java.util.Map; public class SpanBuilderTests extends OpenSearchTestCase { + public String uri; + + public String expectedSpanName; + + public String expectedQueryParams; + + public String expectedReqRawPath; + + @ParametersFactory + public static Collection data() { + return Arrays.asList( + new Object[][] { + { "/_test/resource?name=John&age=25", "GET /_test/resource", "name=John&age=25", "/_test/resource" }, + { "/_test/", "GET /_test/", "", "/_test/" }, } + ); + } + + public SpanBuilderTests(String uri, String expectedSpanName, String expectedQueryParams, String expectedReqRawPath) { + this.uri = uri; + this.expectedSpanName = expectedSpanName; + this.expectedQueryParams = expectedQueryParams; + this.expectedReqRawPath = expectedReqRawPath; + } + public void testHttpRequestContext() { - HttpRequest httpRequest = createHttpRequest(); + HttpRequest httpRequest = createHttpRequest(uri); SpanCreationContext context = SpanBuilder.from(httpRequest); Attributes attributes = context.getAttributes(); - assertEquals("GET /_test", context.getSpanName()); + assertEquals(expectedSpanName, context.getSpanName()); assertEquals("true", attributes.getAttributesMap().get(AttributeNames.TRACE)); assertEquals("GET", attributes.getAttributesMap().get(AttributeNames.HTTP_METHOD)); assertEquals("HTTP_1_0", attributes.getAttributesMap().get(AttributeNames.HTTP_PROTOCOL_VERSION)); - assertEquals("/_test", attributes.getAttributesMap().get(AttributeNames.HTTP_URI)); + assertEquals(uri, attributes.getAttributesMap().get(AttributeNames.HTTP_URI)); + if (expectedQueryParams.isBlank()) { + assertNull(attributes.getAttributesMap().get(AttributeNames.HTTP_REQ_QUERY_PARAMS)); + } else { + assertEquals(expectedQueryParams, attributes.getAttributesMap().get(AttributeNames.HTTP_REQ_QUERY_PARAMS)); + } } public void testRestRequestContext() { - RestRequest restRequest = RestRequest.request(null, createHttpRequest(), null); + RestRequest restRequest = RestRequest.request(null, createHttpRequest(uri), null); SpanCreationContext context = SpanBuilder.from(restRequest); Attributes attributes = context.getAttributes(); - assertEquals("GET /_test", context.getSpanName()); - assertEquals("/_test", attributes.getAttributesMap().get(AttributeNames.REST_REQ_RAW_PATH)); + assertEquals(expectedSpanName, context.getSpanName()); + assertEquals(expectedReqRawPath, attributes.getAttributesMap().get(AttributeNames.REST_REQ_RAW_PATH)); assertNotNull(attributes.getAttributesMap().get(AttributeNames.REST_REQ_ID)); + if (expectedQueryParams.isBlank()) { + assertNull(attributes.getAttributesMap().get(AttributeNames.HTTP_REQ_QUERY_PARAMS)); + } else { + assertEquals(expectedQueryParams, attributes.getAttributesMap().get(AttributeNames.HTTP_REQ_QUERY_PARAMS)); + } } public void testRestRequestContextForNull() { @@ -97,7 +134,7 @@ public void close() { }; } - private static HttpRequest createHttpRequest() { + private static HttpRequest createHttpRequest(String uri) { return new HttpRequest() { @Override public RestRequest.Method method() { @@ -106,7 +143,7 @@ public RestRequest.Method method() { @Override public String uri() { - return "/_test"; + return uri; } @Override