Skip to content

Commit

Permalink
[BugFix] Fixing Span operation names generated from RestActions (#12005…
Browse files Browse the repository at this point in the history 
…) (#12222)

* 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)
  • Loading branch information
@atharvasharma61
atharvasharma61 authored Feb 7, 2024
1 parent 5be018e commit da3c1a5
Show file tree
Hide file tree
Showing 4 changed files with 82 additions and 13 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
5 changes: 5 additions & 0 deletions server/src/main/java/org/opensearch/telemetry/tracing/AttributeNames.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*/
Expand Down
36 changes: 31 additions & 5 deletions server/src/main/java/org/opensearch/telemetry/tracing/SpanBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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) {
Expand All @@ -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
Expand All @@ -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.
}
Expand All @@ -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;
}
Expand Down
53 changes: 45 additions & 8 deletions server/src/test/java/org/opensearch/telemetry/tracing/SpanBuilderTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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() {
Expand Down Expand Up @@ -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() {
Expand All @@ -106,7 +143,7 @@ public RestRequest.Method method() {

@Override
public String uri() {
return "/_test";
return uri;
}

@Override
Expand Down

0 comments on commit da3c1a5

Please sign in to comment.