Metric Bug Fix: Strip out Query and Fragment in extractAPIPathValue L…

…ogic (#36)

*Issue #, if available:*
- `httpTarget` can be of the form `/pathname?q=query#fragment`.
- extractAPIPathValue(httpTarget) will include the query or the fragment
parts, which we do not want.
- This isn't caught with the existing unit test
(`/users/1/pet?query#fragment`) because the query+fragment is removed
after the extractAPIPathValue Logic splits the slashes.

*Description of changes:*
- In this PR, we remove the query and fragment.

Testing:
- Unit tests
- Observed removal of query and fragment from Console Span Exporter.

By submitting this pull request, I confirm that you can use, modify,
copy, and redistribute this contribution, under the terms of your
choice.

aws-distro-opentelemetry-node-autoinstrumentation/src/aws-span-processing-util.ts

@@ -76,7 +76,21 @@ export class AwsSpanProcessingUtil {
     if (httpTarget == null || httpTarget === '') {
       return '/';
     }
-    const paths: string[] = httpTarget.split('/');
+    // Divergence from Java/Python
+    // https://github.com/open-telemetry/semantic-conventions/blob/4e7c42ee8e4c3a39a899c4c85c64df28cd543f78/docs/attributes-registry/http.md#deprecated-http-attributes
+    // According to OTel Spec, httpTarget may include query and fragment:
+    // - `/search?q=OpenTelemetry#SemConv`
+    // We do NOT want the `?` or `#` parts, so let us strip it out,
+    // because HTTP (ingress) instrumentation was observed to include the query (`?`) part
+    // - https://github.com/open-telemetry/opentelemetry-js/blob/b418d36609c371d1fcae46898e9ede6278aca917/experimental/packages/opentelemetry-instrumentation-http/src/utils.ts#L502-L504
+    // According to RFC Specification, "The path is terminated by the first question mark ("?") or number sign ("#") character, or by the end of the URI."
+    // - https://datatracker.ietf.org/doc/html/rfc3986#section-3.3
+    //
+    // This is a fix that can be applied here since this is the central location for generating API Path Value
+    // TODO: Possibly contribute fix to upstream for this diff between langauges. However, the current attribute value in JS is according to spec.
+    //
+    // Interestingly, according to Spec, Java/Python should be affected, but they are not.
+    const paths: string[] = httpTarget.split(/[/?#]/);
     if (paths.length > 1) {
       return '/' + paths[1];
     }

aws-distro-opentelemetry-node-autoinstrumentation/test/aws-span-processing-util.test.ts

@@ -171,6 +171,24 @@ describe('AwsSpanProcessingUtilTest', () => {
     expect(pathValue).toEqual('/users');
   });
 
+  it('testExtractAPIPathValidPathSingleSlash', () => {
+    let validTarget: string = '/users?query#fragment';
+    let pathValue: string = AwsSpanProcessingUtil.extractAPIPathValue(validTarget);
+    expect(pathValue).toEqual('/users');
+
+    validTarget = '/users#fragment?fragment_part_2';
+    pathValue = AwsSpanProcessingUtil.extractAPIPathValue(validTarget);
+    expect(pathValue).toEqual('/users');
+
+    validTarget = '/users?query';
+    pathValue = AwsSpanProcessingUtil.extractAPIPathValue(validTarget);
+    expect(pathValue).toEqual('/users');
+
+    validTarget = '/users#fragment';
+    pathValue = AwsSpanProcessingUtil.extractAPIPathValue(validTarget);
+    expect(pathValue).toEqual('/users');
+  });
+
   it('testIsKeyPresentKeyPresent', () => {
     attributesMock[SEMATTRS_HTTP_TARGET] = 'target';
     expect(AwsSpanProcessingUtil.isKeyPresent(spanDataMock, SEMATTRS_HTTP_TARGET)).toBeTruthy();