Skip to content

Commit da3c1a5

Browse files
atharvasharma61atharvasharma61
authored
[BugFix] Fixing Span operation names generated from RestActions (opensearch-project#12005) (opensearch-project#12222)
* 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 <athrv@amazon.com> * Added UTs Signed-off-by: Atharva Sharma <athrv@amazon.com> * Added parameterized tests Signed-off-by: Atharva Sharma <athrv@amazon.com> * improvised parameterized tests using annotations Signed-off-by: Atharva Sharma <athrv@amazon.com> * removed unused annotations Signed-off-by: Atharva Sharma <athrv@amazon.com> * Added details in CHANGELOG.md Signed-off-by: Atharva Sharma <athrv@amazon.com> * Addressed spotlessJavaCheck failures Signed-off-by: Atharva Sharma <athrv@amazon.com> --------- Signed-off-by: Atharva Sharma <athrv@amazon.com> Signed-off-by: Atharva Sharma <60044988+atharvasharma61@users.noreply.github.com> Co-authored-by: Atharva Sharma <athrv@amazon.com> (cherry picked from commit c0fca74)
1 parent 5be018e commit da3c1a5

File tree

4 files changed

+82
-13
lines changed

4 files changed

+82
-13
lines changed

‎CHANGELOG.md

+1
Original file line numberDiff line numberDiff line change
@@ -178,6 +178,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
178178
- [BUG] Fix remote shards balancer when filtering throttled nodes ([#11724](https://github.com/opensearch-project/OpenSearch/pull/11724))
179179
- [Bug] Check phase name before SearchRequestOperationsListener onPhaseStart ([#12094](https://github.com/opensearch-project/OpenSearch/pull/12094))
180180
- 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))
181+
- Fix Span operation names generated from RestActions ([#12005](https://github.com/opensearch-project/OpenSearch/pull/12005))
181182

182183
### Security
183184

‎server/src/main/java/org/opensearch/telemetry/tracing/AttributeNames.java

+5
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,11 @@ private AttributeNames() {
4040
*/
4141
public static final String HTTP_URI = "http.uri";
4242

43+
/**
44+
* Http Request Query Parameters.
45+
*/
46+
public static final String HTTP_REQ_QUERY_PARAMS = "url.query";
47+
4348
/**
4449
* Rest Request ID.
4550
*/

‎server/src/main/java/org/opensearch/telemetry/tracing/SpanBuilder.java

+31-5
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
import org.opensearch.action.bulk.BulkShardRequest;
1212
import org.opensearch.action.support.replication.ReplicatedWriteRequest;
1313
import org.opensearch.common.annotation.InternalApi;
14+
import org.opensearch.common.collect.Tuple;
1415
import org.opensearch.core.common.Strings;
1516
import org.opensearch.http.HttpRequest;
1617
import org.opensearch.rest.RestRequest;
@@ -75,7 +76,9 @@ public static SpanCreationContext from(String spanName, String nodeId, Replicate
7576
}
7677

7778
private static String createSpanName(HttpRequest httpRequest) {
78-
return httpRequest.method().name() + SEPARATOR + httpRequest.uri();
79+
Tuple<String, String> uriParts = splitUri(httpRequest.uri());
80+
String path = uriParts.v1();
81+
return httpRequest.method().name() + SEPARATOR + path;
7982
}
8083

8184
private static Attributes buildSpanAttributes(HttpRequest httpRequest) {
@@ -84,9 +87,26 @@ private static Attributes buildSpanAttributes(HttpRequest httpRequest) {
8487
.addAttribute(AttributeNames.HTTP_METHOD, httpRequest.method().name())
8588
.addAttribute(AttributeNames.HTTP_PROTOCOL_VERSION, httpRequest.protocolVersion().name());
8689
populateHeader(httpRequest, attributes);
90+
91+
Tuple<String, String> uriParts = splitUri(httpRequest.uri());
92+
String query = uriParts.v2();
93+
if (query.isBlank() == false) {
94+
attributes.addAttribute(AttributeNames.HTTP_REQ_QUERY_PARAMS, query);
95+
}
96+
8797
return attributes;
8898
}
8999

100+
private static Tuple<String, String> splitUri(String uri) {
101+
int index = uri.indexOf('?');
102+
if (index >= 0 && index < uri.length() - 1) {
103+
String path = uri.substring(0, index);
104+
String query = uri.substring(index + 1);
105+
return new Tuple<>(path, query);
106+
}
107+
return new Tuple<>(uri, "");
108+
}
109+
90110
private static void populateHeader(HttpRequest httpRequest, Attributes attributes) {
91111
HEADERS_TO_BE_ADDED_AS_ATTRIBUTES.forEach(x -> {
92112
if (httpRequest.getHeaders() != null
@@ -102,9 +122,8 @@ private static String createSpanName(RestRequest restRequest) {
102122
if (restRequest != null) {
103123
try {
104124
String methodName = restRequest.method().name();
105-
// path() does the decoding, which may give error
106-
String path = restRequest.path();
107-
spanName = methodName + SEPARATOR + path;
125+
String rawPath = restRequest.rawPath();
126+
spanName = methodName + SEPARATOR + rawPath;
108127
} catch (Exception e) {
109128
// swallow the exception and keep the default name.
110129
}
@@ -114,9 +133,16 @@ private static String createSpanName(RestRequest restRequest) {
114133

115134
private static Attributes buildSpanAttributes(RestRequest restRequest) {
116135
if (restRequest != null) {
117-
return Attributes.create()
136+
Attributes attributes = Attributes.create()
118137
.addAttribute(AttributeNames.REST_REQ_ID, restRequest.getRequestId())
119138
.addAttribute(AttributeNames.REST_REQ_RAW_PATH, restRequest.rawPath());
139+
140+
Tuple<String, String> uriParts = splitUri(restRequest.uri());
141+
String query = uriParts.v2();
142+
if (query.isBlank() == false) {
143+
attributes.addAttribute(AttributeNames.HTTP_REQ_QUERY_PARAMS, query);
144+
}
145+
return attributes;
120146
} else {
121147
return Attributes.EMPTY;
122148
}

‎server/src/test/java/org/opensearch/telemetry/tracing/SpanBuilderTests.java

+45-8
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,8 @@
88

99
package org.opensearch.telemetry.tracing;
1010

11+
import com.carrotsearch.randomizedtesting.annotations.ParametersFactory;
12+
1113
import org.opensearch.Version;
1214
import org.opensearch.cluster.node.DiscoveryNode;
1315
import org.opensearch.common.network.NetworkAddress;
@@ -27,29 +29,64 @@
2729

2830
import java.io.IOException;
2931
import java.util.Arrays;
32+
import java.util.Collection;
3033
import java.util.List;
3134
import java.util.Map;
3235

3336
public class SpanBuilderTests extends OpenSearchTestCase {
3437

38+
public String uri;
39+
40+
public String expectedSpanName;
41+
42+
public String expectedQueryParams;
43+
44+
public String expectedReqRawPath;
45+
46+
@ParametersFactory
47+
public static Collection<Object[]> data() {
48+
return Arrays.asList(
49+
new Object[][] {
50+
{ "/_test/resource?name=John&age=25", "GET /_test/resource", "name=John&age=25", "/_test/resource" },
51+
{ "/_test/", "GET /_test/", "", "/_test/" }, }
52+
);
53+
}
54+
55+
public SpanBuilderTests(String uri, String expectedSpanName, String expectedQueryParams, String expectedReqRawPath) {
56+
this.uri = uri;
57+
this.expectedSpanName = expectedSpanName;
58+
this.expectedQueryParams = expectedQueryParams;
59+
this.expectedReqRawPath = expectedReqRawPath;
60+
}
61+
3562
public void testHttpRequestContext() {
36-
HttpRequest httpRequest = createHttpRequest();
63+
HttpRequest httpRequest = createHttpRequest(uri);
3764
SpanCreationContext context = SpanBuilder.from(httpRequest);
3865
Attributes attributes = context.getAttributes();
39-
assertEquals("GET /_test", context.getSpanName());
66+
assertEquals(expectedSpanName, context.getSpanName());
4067
assertEquals("true", attributes.getAttributesMap().get(AttributeNames.TRACE));
4168
assertEquals("GET", attributes.getAttributesMap().get(AttributeNames.HTTP_METHOD));
4269
assertEquals("HTTP_1_0", attributes.getAttributesMap().get(AttributeNames.HTTP_PROTOCOL_VERSION));
43-
assertEquals("/_test", attributes.getAttributesMap().get(AttributeNames.HTTP_URI));
70+
assertEquals(uri, attributes.getAttributesMap().get(AttributeNames.HTTP_URI));
71+
if (expectedQueryParams.isBlank()) {
72+
assertNull(attributes.getAttributesMap().get(AttributeNames.HTTP_REQ_QUERY_PARAMS));
73+
} else {
74+
assertEquals(expectedQueryParams, attributes.getAttributesMap().get(AttributeNames.HTTP_REQ_QUERY_PARAMS));
75+
}
4476
}
4577

4678
public void testRestRequestContext() {
47-
RestRequest restRequest = RestRequest.request(null, createHttpRequest(), null);
79+
RestRequest restRequest = RestRequest.request(null, createHttpRequest(uri), null);
4880
SpanCreationContext context = SpanBuilder.from(restRequest);
4981
Attributes attributes = context.getAttributes();
50-
assertEquals("GET /_test", context.getSpanName());
51-
assertEquals("/_test", attributes.getAttributesMap().get(AttributeNames.REST_REQ_RAW_PATH));
82+
assertEquals(expectedSpanName, context.getSpanName());
83+
assertEquals(expectedReqRawPath, attributes.getAttributesMap().get(AttributeNames.REST_REQ_RAW_PATH));
5284
assertNotNull(attributes.getAttributesMap().get(AttributeNames.REST_REQ_ID));
85+
if (expectedQueryParams.isBlank()) {
86+
assertNull(attributes.getAttributesMap().get(AttributeNames.HTTP_REQ_QUERY_PARAMS));
87+
} else {
88+
assertEquals(expectedQueryParams, attributes.getAttributesMap().get(AttributeNames.HTTP_REQ_QUERY_PARAMS));
89+
}
5390
}
5491

5592
public void testRestRequestContextForNull() {
@@ -97,7 +134,7 @@ public void close() {
97134
};
98135
}
99136

100-
private static HttpRequest createHttpRequest() {
137+
private static HttpRequest createHttpRequest(String uri) {
101138
return new HttpRequest() {
102139
@Override
103140
public RestRequest.Method method() {
@@ -106,7 +143,7 @@ public RestRequest.Method method() {
106143

107144
@Override
108145
public String uri() {
109-
return "/_test";
146+
return uri;
110147
}
111148

112149
@Override

0 commit comments

Comments
 (0)