[BugFix] Fixing Span operation names generated from RestActions (#12005…

…) (#12221) * bug fix to have RestActions rawPath as operation names, query params as tags and avoid using exact URI (#10791) Signed-off-by: Atharva Sharma <[email protected]> * Added UTs Signed-off-by: Atharva Sharma <[email protected]> * Added parameterized tests Signed-off-by: Atharva Sharma <[email protected]> * improvised parameterized tests using annotations Signed-off-by: Atharva Sharma <[email protected]> * removed unused annotations Signed-off-by: Atharva Sharma <[email protected]> * Added details in CHANGELOG.md Signed-off-by: Atharva Sharma <[email protected]> * Addressed spotlessJavaCheck failures Signed-off-by: Atharva Sharma <[email protected]> --------- Signed-off-by: Atharva Sharma <[email protected]> Signed-off-by: Atharva Sharma <[email protected]> Co-authored-by: Atharva Sharma <[email protected]> (cherry picked from commit c0fca74)
opensearch-project · Feb 7, 2024 · 7fef055 · 7fef055
1 parent e519ac2
commit 7fef055
Show file tree

Hide file tree

Showing 4 changed files with 82 additions and 13 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -178,6 +178,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
@@ -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
@@ -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<String, String> 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<String, String> 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<String, String> 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<String, String> 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
@@ -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<Object[]> 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