Enhance AWS APM metrics mapping implementation #906

mxiamxia · 2023-06-06T00:09:20Z

Description:
Enhance AWS APM metrics mapping implementation to resolve a few of the following "Unknown" metric attributes use cases.

if span name used in ingress operation is not valid with values of 'null', 'UnknownOperation' and 'HttpMethod valuefor HTTP request use cases. We will try to extract the ingress operation fromhttp.methodandhttp.target` attributes.
if the remote service attribute can not be directly derived from the attributes such rpc.name, db.system. We will try to fill it with the net.peer.name and net.sock.peer.addr attribute values and have Collector to do the further resolution on the populated values.
if the remote operation attribute is Unknown, We will try to extract the remote operation from http.method and http.url attributes.
Add a new aws.remote.target attribute for supporting the following 4 AWS resources when invoking it with Client Spans
- AWS S3 Bucket Name
- AWS Queue Name
- AWS Stream Name
- AWS DynamoDB table name

Testing:
unit test and adhoc

Documentation:
N/A

Outstanding items:
N/A

bjrara · 2023-06-06T23:51:48Z

aws-xray/src/main/java/io/opentelemetry/contrib/awsxray/AwsAttributeKeys.java

+
+  static final AttributeKey<String> AWS_REMOTE_TARGET = AttributeKey.stringKey("aws.remote.target");
+
+  static final AttributeKey<String> AWS_BUCKET_NAME = AttributeKey.stringKey("aws.bucket.name");


Can we add comments to link to the source of these SDK managed fields or split them into a separate file?

sdk 1-1
sdk 2-2

bjrara · 2023-06-07T00:06:29Z

aws-xray/src/main/java/io/opentelemetry/contrib/awsxray/AwsMetricAttributeGenerator.java

-      logUnknownAttribute(AWS_REMOTE_OPERATION, span);
-      builder.put(AWS_REMOTE_OPERATION, UNKNOWN_REMOTE_OPERATION);
+      // try to derive RemoteService and RemoteOperation from the other un-directive attributes
+      remoteService = generateRemoteService(span);


It's hard to tell the difference between generateRemoteService/Operation and getRemoteService/Operation, only that generate* does some specific to HTTP/NET spans which makes me feel the method name is ambiguous.

I'm thinking if it will be better to assign UNKNOWN* as the initial values to remoteService and remoteOperation, and only do HTTP/NET handling using function like getRemote*FromNETSpan in the else block.

Make some updates to split the RemoteService and RemoteOperation generating logic separately on unknown checking.
The method name is general as I thought we could do more to handle unknown besides the http/net span attributes in the future(even I don't have any idea right now what the other attribute we could use)

Thanks! It's much clear now.

…wn dimension use cases

bjrara

LGTM

mxiamxia · 2023-06-08T22:26:39Z

Hi @trask , could you please take a look this PR as code approver when you get the chance? Thanks!

wangzlei · 2023-06-12T17:25:00Z

LGTM!

thpierce · 2023-06-12T17:53:22Z

aws-xray/src/main/java/io/opentelemetry/contrib/awsxray/AwsAttributeKeys.java

+
+  static final AttributeKey<String> AWS_REMOTE_TARGET = AttributeKey.stringKey("aws.remote.target");
+
+  // use the same AWS Resource attribute name defined by OTel java auto-instr for aws_sdk_v_1_1


If we want to use the same name, why not just depend on OTel java auto-instr directly? Why define them separately here? This adds risk to the implementation because if the names change in OTel java auto-instr, we will not know to change them here. This seems possible given that these strings do not seem to align with the OTEL Spec (e.g. aws.s3.bucket)

+1. I was going to recommend adding a TODO note here to update these attributes to match the semantic attributes, but I prefer @thpierce's suggestion

The listed AWS resource attributes are defined in opentelemetry-java-instrumentation#AwsExperimentalAttributes which is not visible to SDK. I think the long term solution is to move these resource name definitions into semconv#SemanticAttributes.

I think we have to define it again here before we can add it into semconv package.

Create an issue for better way to manage AWS specific attributes open-telemetry/opentelemetry-java-instrumentation#8710

thpierce · 2023-06-12T18:31:54Z

aws-xray/src/main/java/io/opentelemetry/contrib/awsxray/AwsMetricAttributeGenerator.java

-      setRemoteService(span, builder, MESSAGING_SYSTEM);
-      setRemoteOperation(span, builder, MESSAGING_OPERATION);
+      remoteService = getRemoteService(span, MESSAGING_SYSTEM);
+      remoteOperation = getRemoteOperation(span, MESSAGING_OPERATION);


I thought this would be: {messaging.destination.name} {messaging.operation}

thanks for calling it out. I am thinking moving {messaging.destination.name} to aws.remote.target attribute later and keep remote.operation with messaging.operation.

Synced up with @thpierce offline, we'll keep it with the current impl

thpierce · 2023-06-12T18:32:56Z

aws-xray/src/main/java/io/opentelemetry/contrib/awsxray/AwsMetricAttributeGenerator.java

    }
+
+    // try to derive RemoteService and RemoteOperation from the other un-directive attributes


What is un-directive?

removed it.

thpierce · 2023-06-12T18:34:31Z

aws-xray/src/main/java/io/opentelemetry/contrib/awsxray/AwsMetricAttributeGenerator.java

   * name.
   */


Please update comment, behaviour has substantially changed. Please review other comments in this code and update accordingly.

thpierce · 2023-06-12T18:37:19Z

aws-xray/src/main/java/io/opentelemetry/contrib/awsxray/AwsMetricAttributeGenerator.java

+      if (httpTarget != null) {
+        operation = extractAPIPathValue(httpTarget);
+      }
+      if (isKeyPresent(span, HTTP_METHOD)) {


if HTTP_METHOD is not found, are we sure we want to keep just the first part of the target?

Also this code is confusing because in isValidOperation, we are saying that HTTP_METHOD alone is invalid, but here we consider it valid if target is null. A cleaner solution would be to update the if block on line 158 to attempt to append target if only HTTP_METHOD is being used as span name.

changed it to: only add http.method as prefix for RemoteOperation when http.target is not null.

thpierce · 2023-06-12T18:41:54Z

aws-xray/src/main/java/io/opentelemetry/contrib/awsxray/AwsMetricAttributeGenerator.java

+    if (remoteOperation.equals(UNKNOWN_REMOTE_OPERATION)) {
+      logUnknownAttribute(AWS_REMOTE_OPERATION, span);
+    }


Not blocking: This doesn't seem appropriate for this method - I would probably keep it in setRemoteServiceAndOperation. Ditto to generateRemoteService

thpierce · 2023-06-12T18:42:44Z

aws-xray/src/main/java/io/opentelemetry/contrib/awsxray/AwsMetricAttributeGenerator.java

+   * @return the first part from the http target. Eg, /payment
+   */
+  private static String extractAPIPathValue(String httpTarget) {
+    if (httpTarget.isEmpty()) {


Should we do null as well?

thpierce · 2023-06-12T18:44:31Z

aws-xray/src/test/java/io/opentelemetry/contrib/awsxray/AwsMetricAttributeGeneratorTest.java

@@ -5,19 +5,32 @@

 package io.opentelemetry.contrib.awsxray;


Did not have time to review tests, will rely on reviews from others to cover these.

thpierce · 2023-06-12T18:45:43Z

aws-xray/src/main/java/io/opentelemetry/contrib/awsxray/AwsMetricAttributeGenerator.java

@@ -152,35 +212,132 @@ private static void setEgressOperation(SpanData span, AttributesBuilder builder)
   * and RPC attributes to use here, but this is a sufficient starting point.
   */
  private static void setRemoteServiceAndOperation(SpanData span, AttributesBuilder builder) {


I thought we said that remote service/remote operation would be done independently (e.g. could have rpc service and db operation)

it could happen in the following blocks which is not restricted by pair of values.

// try to derive RemoteService and RemoteOperation from the other related attributes if (remoteService.equals(UNKNOWN_REMOTE_SERVICE)) { remoteService = generateRemoteService(span); } if (remoteOperation.equals(UNKNOWN_REMOTE_OPERATION)) { remoteOperation = generateRemoteOperation(span); }

Discussed offline - not exactly what I was talking about, but we decided this is desirable behaviour.

mxiamxia · 2023-06-14T18:20:10Z

Hi @trask, kindly follow up if you can help to review this PR and it has been reviewed by AWS reviewers. Thanks!

github-actions bot assigned srprash and wangzlei Jun 6, 2023

github-actions bot requested review from srprash and wangzlei June 6, 2023 00:09

mxiamxia force-pushed the apm_metric branch 2 times, most recently from 7b35c2c to b0837a3 Compare June 6, 2023 22:23

bjrara reviewed Jun 7, 2023

View reviewed changes

mxiamxia force-pushed the apm_metric branch from b0837a3 to 2fc6de3 Compare June 7, 2023 18:30

Enhance AWS APM metrics mapping implementation to resolve a few unkno…

d45c953

…wn dimension use cases

mxiamxia force-pushed the apm_metric branch from 2fc6de3 to d45c953 Compare June 7, 2023 18:38

mxiamxia requested a review from bjrara June 7, 2023 18:41

bjrara approved these changes Jun 7, 2023

View reviewed changes

mxiamxia marked this pull request as ready for review June 8, 2023 16:08

mxiamxia requested review from a team and bjrara June 8, 2023 16:08

srprash approved these changes Jun 8, 2023

View reviewed changes

trask added this to the v1.27.0 milestone Jun 9, 2023

bjrara approved these changes Jun 12, 2023

View reviewed changes

wangzlei approved these changes Jun 12, 2023

View reviewed changes

thpierce reviewed Jun 12, 2023

View reviewed changes

mxiamxia mentioned this pull request Jun 13, 2023

Move AWS specific attributes definitions in AWS SDK instrumentation modules into semconv package open-telemetry/opentelemetry-java-instrumentation#8710

Closed

Updated the comments for the new changes

144b6cb

mxiamxia requested review from carolabadeer and thpierce June 13, 2023 21:12

carolabadeer approved these changes Jun 14, 2023

View reviewed changes

thpierce approved these changes Jun 14, 2023

View reviewed changes

trask approved these changes Jun 16, 2023

View reviewed changes

trask merged commit 047bd89 into open-telemetry:main Jun 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhance AWS APM metrics mapping implementation #906

Enhance AWS APM metrics mapping implementation #906

mxiamxia commented Jun 6, 2023 •

edited

Loading

bjrara Jun 6, 2023

bjrara Jun 7, 2023

mxiamxia Jun 7, 2023

bjrara Jun 7, 2023

bjrara left a comment

mxiamxia commented Jun 8, 2023

wangzlei commented Jun 12, 2023

thpierce Jun 12, 2023

carolabadeer Jun 12, 2023 •

edited

Loading

mxiamxia Jun 13, 2023

mxiamxia Jun 13, 2023

thpierce Jun 12, 2023

mxiamxia Jun 13, 2023 •

edited

Loading

thpierce Jun 12, 2023

mxiamxia Jun 13, 2023

thpierce Jun 12, 2023

thpierce Jun 12, 2023

mxiamxia Jun 13, 2023

thpierce Jun 12, 2023

thpierce Jun 12, 2023

thpierce Jun 12, 2023

thpierce Jun 12, 2023

mxiamxia Jun 13, 2023

thpierce Jun 14, 2023

mxiamxia commented Jun 14, 2023


		static final AttributeKey<String> AWS_REMOTE_TARGET = AttributeKey.stringKey("aws.remote.target");

		static final AttributeKey<String> AWS_BUCKET_NAME = AttributeKey.stringKey("aws.bucket.name");


		static final AttributeKey<String> AWS_REMOTE_TARGET = AttributeKey.stringKey("aws.remote.target");

		// use the same AWS Resource attribute name defined by OTel java auto-instr for aws_sdk_v_1_1

		}

		// try to derive RemoteService and RemoteOperation from the other un-directive attributes

Enhance AWS APM metrics mapping implementation #906

Enhance AWS APM metrics mapping implementation #906

Conversation

mxiamxia commented Jun 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjrara left a comment

Choose a reason for hiding this comment

mxiamxia commented Jun 8, 2023

wangzlei commented Jun 12, 2023

Choose a reason for hiding this comment

carolabadeer Jun 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mxiamxia Jun 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mxiamxia commented Jun 14, 2023

mxiamxia commented Jun 6, 2023 •

edited

Loading

carolabadeer Jun 12, 2023 •

edited

Loading

mxiamxia Jun 13, 2023 •

edited

Loading