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

…search-project#12005) * bug fix to have RestActions rawPath as operation names, query params as tags and avoid using exact URI (opensearch-project#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)
atharvasharma61 · Feb 7, 2024 · 22f05cb · 22f05cb
1 parent 34dafac
commit 22f05cb
Show file tree

Hide file tree

Showing 4 changed files with 171 additions and 13 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -3,6 +3,96 @@ All notable changes to this project are documented in this file.
 
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). See the [CONTRIBUTING guide](./CONTRIBUTING.md#Changelog) for instructions on how to add changelog entries.
 
+<<<<<<< HEAD
+=======
+## [Unreleased 3.0]
+### Added
+- Support for HTTP/2 (server-side) ([#3847](https://github.com/opensearch-project/OpenSearch/pull/3847))
+- Add getter for path field in NestedQueryBuilder ([#4636](https://github.com/opensearch-project/OpenSearch/pull/4636))
+- Allow mmap to use new JDK-19 preview APIs in Apache Lucene 9.4+ ([#5151](https://github.com/opensearch-project/OpenSearch/pull/5151))
+- Add events correlation engine plugin ([#6854](https://github.com/opensearch-project/OpenSearch/issues/6854))
+- Implement on behalf of token passing for extensions ([#8679](https://github.com/opensearch-project/OpenSearch/pull/8679), [#10664](https://github.com/opensearch-project/OpenSearch/pull/10664))
+- Provide service accounts tokens to extensions ([#9618](https://github.com/opensearch-project/OpenSearch/pull/9618))
+- [AdmissionControl] Added changes for AdmissionControl Interceptor and AdmissionControlService for RateLimiting ([#9286](https://github.com/opensearch-project/OpenSearch/pull/9286))
+- GHA to verify checklist items completion in PR descriptions ([#10800](https://github.com/opensearch-project/OpenSearch/pull/10800))
+- Allow to pass the list settings through environment variables (like [], ["a", "b", "c"], ...) ([#10625](https://github.com/opensearch-project/OpenSearch/pull/10625))
+- [Admission Control] Integrate CPU AC with ResourceUsageCollector and add CPU AC stats to nodes/stats ([#10887](https://github.com/opensearch-project/OpenSearch/pull/10887))
+- [S3 Repository] Add setting to control connection count for sync client ([#12028](https://github.com/opensearch-project/OpenSearch/pull/12028))
+
+### Dependencies
+- Bump `log4j-core` from 2.18.0 to 2.19.0
+- Bump `forbiddenapis` from 3.3 to 3.4
+- Bump `avro` from 1.11.1 to 1.11.2
+- Bump `woodstox-core` from 6.3.0 to 6.3.1
+- Bump `xmlbeans` from 5.1.0 to 5.1.1 ([#4354](https://github.com/opensearch-project/OpenSearch/pull/4354))
+- Bump `reactor-netty-core` from 1.0.19 to 1.0.22 ([#4447](https://github.com/opensearch-project/OpenSearch/pull/4447))
+- Bump `reactive-streams` from 1.0.3 to 1.0.4 ([#4488](https://github.com/opensearch-project/OpenSearch/pull/4488))
+- Bump `jempbox` from 1.8.16 to 1.8.17 ([#4550](https://github.com/opensearch-project/OpenSearch/pull/4550))
+- Update to Gradle 7.6 and JDK-19 ([#4973](https://github.com/opensearch-project/OpenSearch/pull/4973))
+- Update Apache Lucene to 9.5.0-snapshot-d5cef1c ([#5570](https://github.com/opensearch-project/OpenSearch/pull/5570))
+- Bump `maven-model` from 3.6.2 to 3.8.6 ([#5599](https://github.com/opensearch-project/OpenSearch/pull/5599))
+- Bump `maxmind-db` from 2.1.0 to 3.0.0 ([#5601](https://github.com/opensearch-project/OpenSearch/pull/5601))
+- Bump `wiremock-jre8-standalone` from 2.33.2 to 2.35.0
+- Bump `gson` from 2.10 to 2.10.1
+- Bump `com.google.code.gson:gson` from 2.10 to 2.10.1
+- Bump `com.maxmind.geoip2:geoip2` from 4.0.0 to 4.0.1
+- Bump `com.avast.gradle:gradle-docker-compose-plugin` from 0.16.11 to 0.16.12
+- Bump `org.apache.commons:commons-configuration2` from 2.8.0 to 2.9.0
+- Bump `com.netflix.nebula:nebula-publishing-plugin` from 19.2.0 to 20.3.0
+- Bump `io.opencensus:opencensus-api` from 0.18.0 to 0.31.1 ([#7291](https://github.com/opensearch-project/OpenSearch/pull/7291))
+- OpenJDK Update (April 2023 Patch releases) ([#7344](https://github.com/opensearch-project/OpenSearch/pull/7344)
+- Bump `com.google.http-client:google-http-client:1.43.2` from 1.42.0 to 1.43.2 ([7928](https://github.com/opensearch-project/OpenSearch/pull/7928)))
+- Add Opentelemetry dependencies ([#7543](https://github.com/opensearch-project/OpenSearch/issues/7543))
+- Bump `org.bouncycastle:bcprov-jdk15on` to `org.bouncycastle:bcprov-jdk15to18` version 1.75 ([#8247](https://github.com/opensearch-project/OpenSearch/pull/8247))
+- Bump `org.bouncycastle:bcmail-jdk15on` to `org.bouncycastle:bcmail-jdk15to18` version 1.75 ([#8247](https://github.com/opensearch-project/OpenSearch/pull/8247))
+- Bump `org.bouncycastle:bcpkix-jdk15on` to `org.bouncycastle:bcpkix-jdk15to18` version 1.75 ([#8247](https://github.com/opensearch-project/OpenSearch/pull/8247))
+- Bump JNA version from 5.5 to 5.13 ([#9963](https://github.com/opensearch-project/OpenSearch/pull/9963))
+- Bump `org.eclipse.jgit` from 6.5.0 to 6.7.0 ([#10147](https://github.com/opensearch-project/OpenSearch/pull/10147))
+- Bump OpenTelemetry from 1.30.1 to 1.31.0 ([#10617](https://github.com/opensearch-project/OpenSearch/pull/10617))
+- Bump OpenTelemetry from 1.31.0 to 1.32.0 and OpenTelemetry Semconv from 1.21.0-alpha to 1.23.1-alpha ([#11305](https://github.com/opensearch-project/OpenSearch/pull/11305))
+
+### Changed
+- [CCR] Add getHistoryOperationsFromTranslog method to fetch the history snapshot from translogs ([#3948](https://github.com/opensearch-project/OpenSearch/pull/3948))
+- Relax visibility of the HTTP_CHANNEL_KEY and HTTP_SERVER_CHANNEL_KEY to make it possible for the plugins to access associated Netty4HttpChannel / Netty4HttpServerChannel instance ([#4638](https://github.com/opensearch-project/OpenSearch/pull/4638))
+- Migrate client transports to Apache HttpClient / Core 5.x ([#4459](https://github.com/opensearch-project/OpenSearch/pull/4459))
+- Change http code on create index API with bad input raising NotXContentException from 500 to 400 ([#4773](https://github.com/opensearch-project/OpenSearch/pull/4773))
+- Improve summary error message for invalid setting updates ([#4792](https://github.com/opensearch-project/OpenSearch/pull/4792))
+- Return 409 Conflict HTTP status instead of 503 on failure to concurrently execute snapshots ([#8986](https://github.com/opensearch-project/OpenSearch/pull/5855))
+- Add task completion count in search backpressure stats API ([#10028](https://github.com/opensearch-project/OpenSearch/pull/10028/))
+- Deprecate CamelCase `PathHierarchy` tokenizer name in favor to lowercase `path_hierarchy` ([#10894](https://github.com/opensearch-project/OpenSearch/pull/10894))
+- Switched to more reliable OpenSearch Lucene snapshot location([#11728](https://github.com/opensearch-project/OpenSearch/pull/11728))
+
+### Deprecated
+
+### Removed
+- Remove deprecated code to add node name into log pattern of log4j property file ([#4568](https://github.com/opensearch-project/OpenSearch/pull/4568))
+- Unused object and import within TransportClusterAllocationExplainAction ([#4639](https://github.com/opensearch-project/OpenSearch/pull/4639))
+- Remove LegacyESVersion.V_7_0_* and V_7_1_* Constants ([#2768](https://https://github.com/opensearch-project/OpenSearch/pull/2768))
+- Remove LegacyESVersion.V_7_2_ and V_7_3_ Constants ([#4702](https://github.com/opensearch-project/OpenSearch/pull/4702))
+- Always auto release the flood stage block ([#4703](https://github.com/opensearch-project/OpenSearch/pull/4703))
+- Remove LegacyESVersion.V_7_4_ and V_7_5_ Constants ([#4704](https://github.com/opensearch-project/OpenSearch/pull/4704))
+- Remove Legacy Version support from Snapshot/Restore Service ([#4728](https://github.com/opensearch-project/OpenSearch/pull/4728))
+- Remove deprecated serialization logic from pipeline aggs ([#4847](https://github.com/opensearch-project/OpenSearch/pull/4847))
+- Remove unused private methods ([#4926](https://github.com/opensearch-project/OpenSearch/pull/4926))
+- Remove LegacyESVersion.V_7_8_ and V_7_9_ Constants ([#4855](https://github.com/opensearch-project/OpenSearch/pull/4855))
+- Remove LegacyESVersion.V_7_6_ and V_7_7_ Constants ([#4837](https://github.com/opensearch-project/OpenSearch/pull/4837))
+- Remove LegacyESVersion.V_7_10_ Constants ([#5018](https://github.com/opensearch-project/OpenSearch/pull/5018))
+- Remove Version.V_1_ Constants ([#5021](https://github.com/opensearch-project/OpenSearch/pull/5021))
+- Remove custom Map, List and Set collection classes ([#6871](https://github.com/opensearch-project/OpenSearch/pull/6871))
+
+### Fixed
+- Fix 'org.apache.hc.core5.http.ParseException: Invalid protocol version' under JDK 16+ ([#4827](https://github.com/opensearch-project/OpenSearch/pull/4827))
+- Fix compression support for h2c protocol ([#4944](https://github.com/opensearch-project/OpenSearch/pull/4944))
+- Don't over-allocate in HeapBufferedAsyncEntityConsumer in order to consume the response ([#9993](https://github.com/opensearch-project/OpenSearch/pull/9993))
+- Update supported version for max_shard_size parameter in Shrink API ([#11439](https://github.com/opensearch-project/OpenSearch/pull/11439))
+- Fix typo in API annotation check message ([11836](https://github.com/opensearch-project/OpenSearch/pull/11836))
+- Update supported version for must_exist parameter in update aliases API ([#11872](https://github.com/opensearch-project/OpenSearch/pull/11872))
+- [Bug] Check phase name before SearchRequestOperationsListener onPhaseStart ([#12035](https://github.com/opensearch-project/OpenSearch/pull/12035))
+- Fix Span operation names generated from RestActions ([#12005](https://github.com/opensearch-project/OpenSearch/pull/12005))
+
+### Security
+
+>>>>>>> c0fca74709e ([BugFix] Fixing Span operation names generated from RestActions (#12005))
 ## [Unreleased 2.x]
 ### Added
 - [Admission control] Add Resource usage collector service and resource usage tracker ([#10695](https://github.com/opensearch-project/OpenSearch/pull/10695))

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