feat: Add auto-instrumentation support for AWS resources in AWS SDK v1 #13980
Conversation
| import org.junit.jupiter.params.provider.Arguments; | ||
| import org.junit.jupiter.params.provider.MethodSource; | ||
|
|
||
| public abstract class AbstractLambdaClientTest extends AbstractBaseAwsClientTest { |
There was a problem hiding this comment.
Since there isn't a class that extends this abstract class these tests won't be run. Perhaps you forgot to add some classes to the PR?
There was a problem hiding this comment.
You are correct. I need add tests.
| getStateMachineArn = findAccessorOrNull(clz, "getStateMachineArn"); | ||
| getStepFunctionsActivityArn = findAccessorOrNull(clz, "getActivityArn"); | ||
| getSnsTopicArn = findAccessorOrNull(clz, "getTopicArn"); | ||
| getSecretArn = findAccessorOrNull(clz, "getARN"); |
There was a problem hiding this comment.
getARN is also present in https://github.com/aws/aws-sdk-java/blob/76838885247a94fc6b0acc49c46f718473d38e63/aws-java-sdk-wafv2/src/main/java/com/amazonaws/services/wafv2/model/GetRuleGroupRequest.java#L377 and in some other classes. Perhaps you need to find a way to only read this from some specific types? Or perhaps there should be a generic arn attribute?
There was a problem hiding this comment.
Good feedback. I will add conditional check with the class name.
| @@ -90,6 +150,12 @@ private RequestAccess(Class<?> clz) { | |||
| getTableName = findAccessorOrNull(clz, "getTableName"); | |||
| getTopicArn = findAccessorOrNull(clz, "getTopicArn"); | |||
| getTargetArn = findAccessorOrNull(clz, "getTargetArn"); | |||
| getStateMachineArn = findAccessorOrNull(clz, "getStateMachineArn"); | |||
| getStepFunctionsActivityArn = findAccessorOrNull(clz, "getActivityArn"); | |||
| getSnsTopicArn = findAccessorOrNull(clz, "getTopicArn"); | |||
There was a problem hiding this comment.
there is already getTopicArn that is used to fill messaging.destination.name attribute for sns messages
There was a problem hiding this comment.
I will use the existing one.
| getStepFunctionsActivityArn = findAccessorOrNull(clz, "getActivityArn"); | ||
| getSnsTopicArn = findAccessorOrNull(clz, "getTopicArn"); | ||
| getSecretArn = findAccessorOrNull(clz, "getARN"); | ||
| getLambdaName = findAccessorOrNull(clz, "getFunctionName"); |
There was a problem hiding this comment.
Also used in https://github.com/aws/aws-sdk-java/blob/76838885247a94fc6b0acc49c46f718473d38e63/aws-java-sdk-glue/src/main/java/com/amazonaws/services/glue/model/GetUserDefinedFunctionRequest.java#L155 that doesn't seem to be related to lambdas.
There was a problem hiding this comment.
Will address this.
| getSnsTopicArn = findAccessorOrNull(clz, "getTopicArn"); | ||
| getSecretArn = findAccessorOrNull(clz, "getARN"); | ||
| getLambdaName = findAccessorOrNull(clz, "getFunctionName"); | ||
| getLambdaResourceId = findAccessorOrNull(clz, "getUUID"); |
There was a problem hiding this comment.
I couldn't find it being used outside of lambda sdk, but given its generic name it could benefit from an extra check to ensure that it is not added to some random request
There was a problem hiding this comment.
I will address this with getFunctionName().
This PR adds auto-instrumentation support for the following AWS resources: Lambda Secrets Manager SNS Step Functions Tests Run: ./gradlew spotlessCheck ./gradlew clean assemble ./gradlew instrumentation:test ./gradlew :smoke-tests:test No regression issues found. All newly added tests pass. Backward Compatibility: There is no risk of breaking existing functionality. This change only adds instrumentation for additional AWS resources without modifying the existing behavior of the auto-instrumentation library.
|
@laurit I addressed all the review comments. Would you please review the new iteration? Sorry I made a rebase from main branch, which makes the diff between 1st and 2nd iterations a bit out of sync. |
This PR adds auto-instrumentation support for the following AWS resources:
Lambda
Secrets Manager
SNS
Step Functions
Tests Run:
./gradlew spotlessCheck
./gradlew clean assemble
./gradlew instrumentation:test
./gradlew :smoke-tests:test
No regression issues found. All newly added tests pass.
Backward Compatibility:
There is no risk of breaking existing functionality. This change only adds instrumentation for additional AWS resources without modifying the existing behavior of the auto-instrumentation library.