Enhance AWS APM metrics mapping implementation by mxiamxia · Pull Request #906 · open-telemetry/opentelemetry-java-contrib

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

+
+  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

-      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

+
+  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

-      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

+      remoteService = getRemoteService(span, PEER_SERVICE);
    }
+
+    // try to derive RemoteService and RemoteOperation from the other un-directive attributes


What is un-directive?

removed it.

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

   * name.
   */


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

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

+      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

+    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

+   * @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

@@ -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

@@ -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


		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

Conversation

mxiamxia commented Jun 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bjrara left a comment

Choose a reason for hiding this comment

Uh oh!

mxiamxia commented Jun 8, 2023

Uh oh!

wangzlei commented Jun 12, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carolabadeer Jun 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mxiamxia Jun 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mxiamxia commented Jun 14, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

mxiamxia commented Jun 6, 2023 •

edited

Loading

carolabadeer Jun 12, 2023 •

edited

Loading

mxiamxia Jun 13, 2023 •

edited

Loading