Added dynamodb instrumenter for aws v1_11 sdk #12756
Conversation
There was a problem hiding this comment.
There are some CI failings, you can refer to related contribution docs to fix them:
...brary/src/main/java/io/opentelemetry/instrumentation/awssdk/v1_11/TracingRequestHandler.java
Outdated
Show resolved
Hide resolved
|
The tests were recently rewritten so I need to update them |
...y/src/main/java/io/opentelemetry/instrumentation/awssdk/v1_11/DynamoAttributesExtractor.java
Outdated
Show resolved
Hide resolved
...esting/src/main/java/io/opentelemetry/instrumentation/awssdk/v1_11/AbstractS3ClientTest.java
Outdated
Show resolved
Hide resolved
...11/testing/src/main/java/io/opentelemetry/instrumentation/awssdk/v1_11/AttributeKeyPair.java
Outdated
Show resolved
Hide resolved
...brary/src/main/java/io/opentelemetry/instrumentation/awssdk/v1_11/TracingRequestHandler.java
Outdated
Show resolved
Hide resolved
...brary/src/main/java/io/opentelemetry/instrumentation/awssdk/v1_11/TracingRequestHandler.java
Outdated
Show resolved
Hide resolved
.../src/main/java/io/opentelemetry/instrumentation/awssdk/v1_11/AbstractDynamoDbClientTest.java
Outdated
Show resolved
Hide resolved
...src/main/java/io/opentelemetry/instrumentation/awssdk/v1_11/DynamoDbAttributesExtractor.java
Outdated
Show resolved
Hide resolved
| equalTo(stringKey("aws.table.name"), "sometable"), | ||
| equalTo(stringKey("db.system"), "dynamodb"), | ||
| equalTo(stringArrayKey("aws.dynamodb.table_names"), singletonList("sometable"))); |
There was a problem hiding this comment.
it's a bit confusing to have both aws.dynamodb.table_names and aws.table.name at the same time, but I don't have a better idea (and aws.table.name requires experimental flag to get anyways)
There was a problem hiding this comment.
Yep I had the same qualm but I don't want to introduce a breaking change and also wanted to align to the sem convs
...sting/src/main/java/io/opentelemetry/instrumentation/awssdk/v1_11/AbstractSnsClientTest.java
Outdated
Show resolved
Hide resolved
| String tableName = RequestAccess.getTableName(request.getOriginalRequest()); | ||
| AttributesExtractorUtil.internalSet( | ||
| attributes, AWS_DYNAMODB_TABLE_NAMES, Collections.singletonList(tableName)); |
There was a problem hiding this comment.
this is just an observation more than a request for a change - I understand that this attribute is a List<String> based on the spec, but it doesn't look like this code supports multiple table names, so it feels a little strange for it be defined that way
| private static final String SEND_MESSAGE_REQUEST = | ||
| "com.amazonaws.services.sqs.model.SendMessageRequest"; | ||
| private static final String DYNAMODBV2 = "com.amazonaws.services.dynamodbv2.model."; |
There was a problem hiding this comment.
slight nitpick: perhaps the usage of these variables provides enough context, but it might be a little more intuitive if the names indicated that they reference the names of classes
| private static final String SEND_MESSAGE_REQUEST = | |
| "com.amazonaws.services.sqs.model.SendMessageRequest"; | |
| private static final String DYNAMODBV2 = "com.amazonaws.services.dynamodbv2.model."; | |
| private static final String SEND_MESSAGE_REQUEST_CLASS = | |
| "com.amazonaws.services.sqs.model.SendMessageRequest"; | |
| private static final String DYNAMODBV2_CLASS = "com.amazonaws.services.dynamodbv2.model."; |
Resolves #12493
Added a dynamo instrumenter for the aws v1_11 sdk. The most important thing this provides is the db.system attribute which was previously missing and is required according to the sem conv for dynamo. Table name has also been added as an attribute, the instrumenter can continue to be enhanced to add additional 'recommended' attributed from the semantic conventions.