-
Notifications
You must be signed in to change notification settings - Fork 865
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.
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
Added dynamodb instrumenter for aws v1_11 sdk #12756
base: main
Are you sure you want to change the base?
Changes from all commits
fceec97
647d433
fd2f694
9857868
3c0acac
70daf4e
61c014c
1b0a198
09e823a
d2b7fed
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
/* | ||
* Copyright The OpenTelemetry Authors | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
package io.opentelemetry.instrumentation.awssdk.v1_11; | ||
|
||
import com.amazonaws.Request; | ||
import com.amazonaws.Response; | ||
import io.opentelemetry.api.common.AttributeKey; | ||
import io.opentelemetry.api.common.AttributesBuilder; | ||
import io.opentelemetry.context.Context; | ||
import io.opentelemetry.instrumentation.api.instrumenter.AttributesExtractor; | ||
import io.opentelemetry.instrumentation.api.internal.AttributesExtractorUtil; | ||
import java.util.Collections; | ||
import java.util.List; | ||
import javax.annotation.Nullable; | ||
|
||
public class DynamoDbAttributesExtractor implements AttributesExtractor<Request<?>, Response<?>> { | ||
|
||
private static final AttributeKey<String> DB_SYSTEM = AttributeKey.stringKey("db.system"); | ||
private static final AttributeKey<List<String>> AWS_DYNAMODB_TABLE_NAMES = | ||
AttributeKey.stringArrayKey("aws.dynamodb.table_names"); | ||
|
||
private static final String DYNAMODB = "dynamodb"; | ||
|
||
@Override | ||
public void onStart(AttributesBuilder attributes, Context parentContext, Request<?> request) { | ||
AttributesExtractorUtil.internalSet(attributes, DB_SYSTEM, DYNAMODB); | ||
String tableName = RequestAccess.getTableName(request.getOriginalRequest()); | ||
AttributesExtractorUtil.internalSet( | ||
attributes, AWS_DYNAMODB_TABLE_NAMES, Collections.singletonList(tableName)); | ||
} | ||
|
||
@Override | ||
public void onEnd( | ||
AttributesBuilder attributes, | ||
Context context, | ||
Request<?> request, | ||
@Nullable Response<?> response, | ||
@Nullable Throwable error) {} | ||
} |
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -31,21 +31,27 @@ final class TracingRequestHandler extends RequestHandler2 { | |||||||||||||
ContextKey.named(TracingRequestHandler.class.getName() + ".Timer"); | ||||||||||||||
private static final ContextKey<Boolean> REQUEST_SPAN_SUPPRESSED_KEY = | ||||||||||||||
ContextKey.named(TracingRequestHandler.class.getName() + ".RequestSpanSuppressed"); | ||||||||||||||
private static final String SEND_MESSAGE_REQUEST = | ||||||||||||||
"com.amazonaws.services.sqs.model.SendMessageRequest"; | ||||||||||||||
private static final String DYNAMODBV2 = "com.amazonaws.services.dynamodbv2.model."; | ||||||||||||||
Comment on lines
+34
to
+36
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Suggested change
|
||||||||||||||
|
||||||||||||||
private final Instrumenter<Request<?>, Response<?>> requestInstrumenter; | ||||||||||||||
private final Instrumenter<SqsReceiveRequest, Response<?>> consumerReceiveInstrumenter; | ||||||||||||||
private final Instrumenter<SqsProcessRequest, Response<?>> consumerProcessInstrumenter; | ||||||||||||||
private final Instrumenter<Request<?>, Response<?>> producerInstrumenter; | ||||||||||||||
private final Instrumenter<Request<?>, Response<?>> dynamoDbInstrumenter; | ||||||||||||||
|
||||||||||||||
TracingRequestHandler( | ||||||||||||||
Instrumenter<Request<?>, Response<?>> requestInstrumenter, | ||||||||||||||
Instrumenter<SqsReceiveRequest, Response<?>> consumerReceiveInstrumenter, | ||||||||||||||
Instrumenter<SqsProcessRequest, Response<?>> consumerProcessInstrumenter, | ||||||||||||||
Instrumenter<Request<?>, Response<?>> producerInstrumenter) { | ||||||||||||||
Instrumenter<Request<?>, Response<?>> producerInstrumenter, | ||||||||||||||
Instrumenter<Request<?>, Response<?>> dynamoDbInstrumenter) { | ||||||||||||||
this.requestInstrumenter = requestInstrumenter; | ||||||||||||||
this.consumerReceiveInstrumenter = consumerReceiveInstrumenter; | ||||||||||||||
this.consumerProcessInstrumenter = consumerProcessInstrumenter; | ||||||||||||||
this.producerInstrumenter = producerInstrumenter; | ||||||||||||||
this.dynamoDbInstrumenter = dynamoDbInstrumenter; | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
@Override | ||||||||||||||
|
@@ -151,14 +157,17 @@ private void finish(Request<?> request, Response<?> response, @Nullable Throwabl | |||||||||||||
} | ||||||||||||||
return; | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
instrumenter.end(context, request, response, error); | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
private Instrumenter<Request<?>, Response<?>> getInstrumenter(Request<?> request) { | ||||||||||||||
boolean isSqsProducer = | ||||||||||||||
"com.amazonaws.services.sqs.model.SendMessageRequest" | ||||||||||||||
.equals(request.getOriginalRequest().getClass().getName()); | ||||||||||||||
return isSqsProducer ? producerInstrumenter : requestInstrumenter; | ||||||||||||||
String className = request.getOriginalRequest().getClass().getName(); | ||||||||||||||
if (className.startsWith(DYNAMODBV2)) { | ||||||||||||||
return dynamoDbInstrumenter; | ||||||||||||||
} | ||||||||||||||
if (className.equals(SEND_MESSAGE_REQUEST)) { | ||||||||||||||
return producerInstrumenter; | ||||||||||||||
} | ||||||||||||||
return requestInstrumenter; | ||||||||||||||
} | ||||||||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,13 +5,20 @@ | |
|
||
package io.opentelemetry.instrumentation.awssdk.v1_11; | ||
|
||
import static io.opentelemetry.api.common.AttributeKey.stringArrayKey; | ||
import static io.opentelemetry.api.common.AttributeKey.stringKey; | ||
import static io.opentelemetry.sdk.testing.assertj.OpenTelemetryAssertions.equalTo; | ||
import static java.util.Collections.singletonList; | ||
|
||
import com.amazonaws.services.dynamodbv2.AmazonDynamoDB; | ||
import com.amazonaws.services.dynamodbv2.AmazonDynamoDBClientBuilder; | ||
import com.amazonaws.services.dynamodbv2.model.CreateTableRequest; | ||
import com.google.common.collect.ImmutableMap; | ||
import io.opentelemetry.sdk.testing.assertj.AttributeAssertion; | ||
import io.opentelemetry.testing.internal.armeria.common.HttpResponse; | ||
import io.opentelemetry.testing.internal.armeria.common.HttpStatus; | ||
import io.opentelemetry.testing.internal.armeria.common.MediaType; | ||
import java.util.Arrays; | ||
import java.util.List; | ||
import org.junit.jupiter.api.Test; | ||
|
||
public abstract class AbstractDynamoDbClientTest extends AbstractBaseAwsClientTest { | ||
|
@@ -34,13 +41,14 @@ public void sendRequestWithMockedResponse() throws Exception { | |
|
||
server.enqueue(HttpResponse.of(HttpStatus.OK, MediaType.PLAIN_TEXT_UTF_8, "")); | ||
|
||
List<AttributeAssertion> additionalAttributes = | ||
Arrays.asList( | ||
equalTo(stringKey("aws.table.name"), "sometable"), | ||
equalTo(stringKey("db.system"), "dynamodb"), | ||
equalTo(stringArrayKey("aws.dynamodb.table_names"), singletonList("sometable"))); | ||
Comment on lines
+46
to
+48
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it's a bit confusing to have both There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
|
||
Object response = client.createTable(new CreateTableRequest("sometable", null)); | ||
assertRequestWithMockedResponse( | ||
response, | ||
client, | ||
"DynamoDBv2", | ||
"CreateTable", | ||
"POST", | ||
ImmutableMap.of("aws.table.name", "sometable")); | ||
response, client, "DynamoDBv2", "CreateTable", "POST", additionalAttributes); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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