Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhance AWS APM metrics mapping implementation #906

Merged
merged 2 commits into from
Jun 16, 2023
Merged

Enhance AWS APM metrics mapping implementation #906

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d45c953
Enhance AWS APM metrics mapping implementation to resolve a few unkno…
mxiamxia Jun 5, 2023
144b6cb
Updated the comments for the new changes
mxiamxia Jun 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions aws-xray/src/main/java/io/opentelemetry/contrib/awsxray/AwsAttributeKeys.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,4 +24,12 @@ private AwsAttributeKeys() {}

static final AttributeKey<String> AWS_REMOTE_OPERATION =
AttributeKey.stringKey("aws.remote.operation");

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link

@carolabadeer carolabadeer Jun 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

static final AttributeKey<String> AWS_QUEUE_NAME = AttributeKey.stringKey("aws.queue.name");
static final AttributeKey<String> AWS_STREAM_NAME = AttributeKey.stringKey("aws.stream.name");
static final AttributeKey<String> AWS_TABLE_NAME = AttributeKey.stringKey("aws.table.name");
}
217 changes: 185 additions & 32 deletions aws-xray/src/main/java/io/opentelemetry/contrib/awsxray/AwsMetricAttributeGenerator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,19 +5,31 @@

package io.opentelemetry.contrib.awsxray;

import static io.opentelemetry.contrib.awsxray.AwsAttributeKeys.AWS_BUCKET_NAME;
import static io.opentelemetry.contrib.awsxray.AwsAttributeKeys.AWS_LOCAL_OPERATION;
import static io.opentelemetry.contrib.awsxray.AwsAttributeKeys.AWS_LOCAL_SERVICE;
import static io.opentelemetry.contrib.awsxray.AwsAttributeKeys.AWS_QUEUE_NAME;
import static io.opentelemetry.contrib.awsxray.AwsAttributeKeys.AWS_REMOTE_OPERATION;
import static io.opentelemetry.contrib.awsxray.AwsAttributeKeys.AWS_REMOTE_SERVICE;
import static io.opentelemetry.contrib.awsxray.AwsAttributeKeys.AWS_REMOTE_TARGET;
import static io.opentelemetry.contrib.awsxray.AwsAttributeKeys.AWS_SPAN_KIND;
import static io.opentelemetry.contrib.awsxray.AwsAttributeKeys.AWS_STREAM_NAME;
import static io.opentelemetry.contrib.awsxray.AwsAttributeKeys.AWS_TABLE_NAME;
import static io.opentelemetry.semconv.resource.attributes.ResourceAttributes.SERVICE_NAME;
import static io.opentelemetry.semconv.trace.attributes.SemanticAttributes.DB_OPERATION;
import static io.opentelemetry.semconv.trace.attributes.SemanticAttributes.DB_SYSTEM;
import static io.opentelemetry.semconv.trace.attributes.SemanticAttributes.FAAS_INVOKED_NAME;
import static io.opentelemetry.semconv.trace.attributes.SemanticAttributes.FAAS_INVOKED_PROVIDER;
import static io.opentelemetry.semconv.trace.attributes.SemanticAttributes.FAAS_TRIGGER;
import static io.opentelemetry.semconv.trace.attributes.SemanticAttributes.GRAPHQL_OPERATION_TYPE;
import static io.opentelemetry.semconv.trace.attributes.SemanticAttributes.HTTP_METHOD;
import static io.opentelemetry.semconv.trace.attributes.SemanticAttributes.HTTP_TARGET;
import static io.opentelemetry.semconv.trace.attributes.SemanticAttributes.HTTP_URL;
import static io.opentelemetry.semconv.trace.attributes.SemanticAttributes.MESSAGING_OPERATION;
import static io.opentelemetry.semconv.trace.attributes.SemanticAttributes.MESSAGING_SYSTEM;
import static io.opentelemetry.semconv.trace.attributes.SemanticAttributes.NET_PEER_NAME;
import static io.opentelemetry.semconv.trace.attributes.SemanticAttributes.NET_PEER_PORT;
import static io.opentelemetry.semconv.trace.attributes.SemanticAttributes.NET_SOCK_PEER_ADDR;
import static io.opentelemetry.semconv.trace.attributes.SemanticAttributes.NET_SOCK_PEER_PORT;
import static io.opentelemetry.semconv.trace.attributes.SemanticAttributes.PEER_SERVICE;
import static io.opentelemetry.semconv.trace.attributes.SemanticAttributes.RPC_METHOD;
import static io.opentelemetry.semconv.trace.attributes.SemanticAttributes.RPC_SERVICE;
Expand All @@ -30,6 +42,9 @@
import io.opentelemetry.sdk.trace.data.SpanData;
import io.opentelemetry.semconv.resource.attributes.ResourceAttributes;
import io.opentelemetry.semconv.trace.attributes.SemanticAttributes;
import java.net.MalformedURLException;
import java.net.URL;
import java.util.Optional;
import java.util.logging.Level;
import java.util.logging.Logger;

Expand Down Expand Up @@ -71,6 +86,7 @@ public Attributes generateMetricAttributesFromSpan(SpanData span, Resource resou
setService(resource, span, builder);
setEgressOperation(span, builder);
setRemoteServiceAndOperation(span, builder);
setRemoteTarget(span, builder);
setSpanKind(span, builder);
break;
default:
Expand All @@ -79,6 +95,30 @@ public Attributes generateMetricAttributesFromSpan(SpanData span, Resource resou
return builder.build();
}

private static void setRemoteTarget(SpanData span, AttributesBuilder builder) {
Optional<String> remoteTarget = getRemoteTarget(span);
remoteTarget.ifPresent(s -> builder.put(AWS_REMOTE_TARGET, s));
}

/**
* RemoteTarget attribute {@link AwsAttributeKeys#AWS_REMOTE_TARGET} is used to store the resource
* name of the remote invokes, such as S3 bucket name, mysql table name, etc. TODO: currently only
* support AWS resource name, will be extended to support the general remote targets, such as
* ActiveMQ name, etc.
*/
private static Optional<String> getRemoteTarget(SpanData span) {
if (isKeyPresent(span, AWS_BUCKET_NAME)) {
return Optional.ofNullable(span.getAttributes().get(AWS_BUCKET_NAME));
} else if (isKeyPresent(span, AWS_QUEUE_NAME)) {
return Optional.ofNullable(span.getAttributes().get(AWS_QUEUE_NAME));
} else if (isKeyPresent(span, AWS_STREAM_NAME)) {
return Optional.ofNullable(span.getAttributes().get(AWS_STREAM_NAME));
} else if (isKeyPresent(span, AWS_TABLE_NAME)) {
return Optional.ofNullable(span.getAttributes().get(AWS_TABLE_NAME));
}
return Optional.empty();
}

/** Service is always derived from {@link ResourceAttributes#SERVICE_NAME} */
private static void setService(Resource resource, SpanData span, AttributesBuilder builder) {
String service = resource.getAttribute(SERVICE_NAME);
Expand All @@ -94,14 +134,34 @@ private static void setService(Resource resource, SpanData span, AttributesBuild
* name.
*/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

private static void setIngressOperation(SpanData span, AttributesBuilder builder) {
String operation = span.getName();
if (operation == null) {
String operation;
if (!isValidOperation(span)) {
operation = generateIngressOperation(span);
} else {
operation = span.getName();
}
if (operation.equals(UNKNOWN_OPERATION)) {
logUnknownAttribute(AWS_LOCAL_OPERATION, span);
operation = UNKNOWN_OPERATION;
}
builder.put(AWS_LOCAL_OPERATION, operation);
}

/**
* When Span name is null, UnknownOperation or HttpMethod value, it will be treated as invalid
* local operation value that needs to be further processed
*/
private static boolean isValidOperation(SpanData span) {
String operation = span.getName();
if (operation == null || operation.equals(UNKNOWN_OPERATION)) {
return false;
}
if (isKeyPresent(span, HTTP_METHOD)) {
String httpMethod = span.getAttributes().get(HTTP_METHOD);
return !operation.equals(httpMethod);
}
return true;
}

/**
* Egress operation (i.e. operation for Client and Producer spans) is always derived from a
* special span attribute, {@link AwsAttributeKeys#AWS_LOCAL_OPERATION}. This attribute is
Expand Down Expand Up @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
    }

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

String remoteService = UNKNOWN_REMOTE_SERVICE;
String remoteOperation = UNKNOWN_REMOTE_OPERATION;
if (isKeyPresent(span, AWS_REMOTE_SERVICE) || isKeyPresent(span, AWS_REMOTE_OPERATION)) {
setRemoteService(span, builder, AWS_REMOTE_SERVICE);
setRemoteOperation(span, builder, AWS_REMOTE_OPERATION);
remoteService = getRemoteService(span, AWS_REMOTE_SERVICE);
remoteOperation = getRemoteOperation(span, AWS_REMOTE_OPERATION);
} else if (isKeyPresent(span, RPC_SERVICE) || isKeyPresent(span, RPC_METHOD)) {
setRemoteService(span, builder, RPC_SERVICE);
setRemoteOperation(span, builder, RPC_METHOD);
remoteService = getRemoteService(span, RPC_SERVICE);
remoteOperation = getRemoteOperation(span, RPC_METHOD);
} else if (isKeyPresent(span, DB_SYSTEM) || isKeyPresent(span, DB_OPERATION)) {
setRemoteService(span, builder, DB_SYSTEM);
setRemoteOperation(span, builder, DB_OPERATION);
} else if (isKeyPresent(span, FAAS_INVOKED_PROVIDER) || isKeyPresent(span, FAAS_INVOKED_NAME)) {
setRemoteService(span, builder, FAAS_INVOKED_PROVIDER);
setRemoteOperation(span, builder, FAAS_INVOKED_NAME);
remoteService = getRemoteService(span, DB_SYSTEM);
remoteOperation = getRemoteOperation(span, DB_OPERATION);
} else if (isKeyPresent(span, FAAS_INVOKED_NAME) || isKeyPresent(span, FAAS_TRIGGER)) {
remoteService = getRemoteService(span, FAAS_INVOKED_NAME);
remoteOperation = getRemoteOperation(span, FAAS_TRIGGER);
} else if (isKeyPresent(span, MESSAGING_SYSTEM) || isKeyPresent(span, MESSAGING_OPERATION)) {
setRemoteService(span, builder, MESSAGING_SYSTEM);
setRemoteOperation(span, builder, MESSAGING_OPERATION);
remoteService = getRemoteService(span, MESSAGING_SYSTEM);
remoteOperation = getRemoteOperation(span, MESSAGING_OPERATION);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@mxiamxia mxiamxia Jun 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

} else if (isKeyPresent(span, GRAPHQL_OPERATION_TYPE)) {
builder.put(AWS_REMOTE_SERVICE, GRAPHQL);
setRemoteOperation(span, builder, GRAPHQL_OPERATION_TYPE);
} else {
logUnknownAttribute(AWS_REMOTE_SERVICE, span);
builder.put(AWS_REMOTE_SERVICE, UNKNOWN_REMOTE_SERVICE);
logUnknownAttribute(AWS_REMOTE_OPERATION, span);
builder.put(AWS_REMOTE_OPERATION, UNKNOWN_REMOTE_OPERATION);
remoteService = GRAPHQL;
remoteOperation = getRemoteOperation(span, GRAPHQL_OPERATION_TYPE);
}

// Peer service takes priority as RemoteService over everything but AWS Remote.
if (isKeyPresent(span, PEER_SERVICE) && !isKeyPresent(span, AWS_REMOTE_SERVICE)) {
setRemoteService(span, builder, PEER_SERVICE);
remoteService = getRemoteService(span, PEER_SERVICE);
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is un-directive?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed it.

if (remoteService.equals(UNKNOWN_REMOTE_SERVICE)) {
remoteService = generateRemoteService(span);
}
if (remoteOperation.equals(UNKNOWN_REMOTE_OPERATION)) {
remoteOperation = generateRemoteOperation(span);
}

builder.put(AWS_REMOTE_SERVICE, remoteService);
builder.put(AWS_REMOTE_OPERATION, remoteOperation);
}

/**
* When span name is not meaningful(null, unknown or http_method value) as operation name for http
* use cases. Will try to extract the operation name from http target string
*/
private static String generateIngressOperation(SpanData span) {
String operation = UNKNOWN_OPERATION;
if (isKeyPresent(span, HTTP_TARGET)) {
String httpTarget = span.getAttributes().get(HTTP_TARGET);
// get the first part from API path string as operation value
// the more levels/parts we get from API path the higher chance for getting high cardinality
// data
if (httpTarget != null) {
operation = extractAPIPathValue(httpTarget);
}
if (isKeyPresent(span, HTTP_METHOD)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

String httpMethod = span.getAttributes().get(HTTP_METHOD);
if (httpMethod != null) {
operation = httpMethod + " " + operation;
}
}
}
return operation;
}

/**
* When the remote call operation is undetermined for http use cases, will try to extract the
* remote operation name from http url string
*/
private static String generateRemoteOperation(SpanData span) {
String remoteOperation = UNKNOWN_REMOTE_OPERATION;
if (isKeyPresent(span, HTTP_URL)) {
String httpUrl = span.getAttributes().get(HTTP_URL);
try {
URL url;
if (httpUrl != null) {
url = new URL(httpUrl);
remoteOperation = extractAPIPathValue(url.getPath());
}
} catch (MalformedURLException e) {
logger.log(Level.FINEST, "invalid http.url attribute: ", httpUrl);
}
}
if (isKeyPresent(span, HTTP_METHOD)) {
String httpMethod = span.getAttributes().get(HTTP_METHOD);
remoteOperation = httpMethod + " " + remoteOperation;
}
if (remoteOperation.equals(UNKNOWN_REMOTE_OPERATION)) {
logUnknownAttribute(AWS_REMOTE_OPERATION, span);
}
Comment on lines +303 to +305
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return remoteOperation;
}

/**
* Extract the first part from API http target if it exists
*
* @param httpTarget http request target string value. Eg, /payment/1234
* @return the first part from the http target. Eg, /payment
*/
private static String extractAPIPathValue(String httpTarget) {
if (httpTarget.isEmpty()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we do null as well?

return "/";
}
String[] paths = httpTarget.split("/");
if (paths.length > 1) {
return "/" + paths[1];
}
return "/";
}

private static String generateRemoteService(SpanData span) {
String remoteService = UNKNOWN_REMOTE_SERVICE;
if (isKeyPresent(span, NET_PEER_NAME)) {
remoteService = getRemoteService(span, NET_PEER_NAME);
if (isKeyPresent(span, NET_PEER_PORT)) {
Long port = span.getAttributes().get(NET_PEER_PORT);
remoteService += ":" + port;
}
} else if (isKeyPresent(span, NET_SOCK_PEER_ADDR)) {
remoteService = getRemoteService(span, NET_SOCK_PEER_ADDR);
if (isKeyPresent(span, NET_SOCK_PEER_PORT)) {
Long port = span.getAttributes().get(NET_SOCK_PEER_PORT);
remoteService += ":" + port;
}
} else {
logUnknownAttribute(AWS_REMOTE_SERVICE, span);
}
return remoteService;
}

/** Span kind is needed for differentiating metrics in the EMF exporter */
Expand All @@ -189,28 +346,24 @@ private static void setSpanKind(SpanData span, AttributesBuilder builder) {
builder.put(AWS_SPAN_KIND, spanKind);
}

private static boolean isKeyPresent(SpanData span, AttributeKey<String> key) {
private static boolean isKeyPresent(SpanData span, AttributeKey<?> key) {
return span.getAttributes().get(key) != null;
}

private static void setRemoteService(
SpanData span, AttributesBuilder builder, AttributeKey<String> remoteServiceKey) {
private static String getRemoteService(SpanData span, AttributeKey<String> remoteServiceKey) {
String remoteService = span.getAttributes().get(remoteServiceKey);
if (remoteService == null) {
logUnknownAttribute(AWS_REMOTE_SERVICE, span);
remoteService = UNKNOWN_REMOTE_SERVICE;
}
builder.put(AWS_REMOTE_SERVICE, remoteService);
return remoteService;
}

private static void setRemoteOperation(
SpanData span, AttributesBuilder builder, AttributeKey<String> remoteOperationKey) {
private static String getRemoteOperation(SpanData span, AttributeKey<String> remoteOperationKey) {
String remoteOperation = span.getAttributes().get(remoteOperationKey);
if (remoteOperation == null) {
logUnknownAttribute(AWS_REMOTE_OPERATION, span);
remoteOperation = UNKNOWN_REMOTE_OPERATION;
}
builder.put(AWS_REMOTE_OPERATION, remoteOperation);
return remoteOperation;
}

private static void logUnknownAttribute(AttributeKey<String> attributeKey, SpanData span) {
Expand Down
Loading