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.

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 kinesis Consumer support #200

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open

Enhance kinesis Consumer support #200

wants to merge 16 commits into from

Conversation

zzhlogin
Copy link
Contributor

@zzhlogin zzhlogin commented Jun 3, 2024

Add kinesis Consumer Support with the following change:

  1. Modify patch _KinesisExtension where extract ConsumerARN and add into "aws.kinesis.consumer_arn" span attributes. The attributes will be retrieved from both request and response context.

  2. Populate RemoteResourceType and RemoteResourceIdentifier for SNS in ADOT python and update unit test and contract test to verify ADOT python performance.

  3. Contract test - botocore_server.py: As we only want to have one span per test, and the consumerArn contains random prefix, it will be different per test, we pre-compute consumer_arn and pass it into RequestHandler. To avoid exception when creating resource, we check if the resource exist first, and create the resource when it not found. This make sure prepare_aws_server function code can be run to the end without exception and the consumer_arn can be returned.

  4. Contract test - contract_test_base.py: As ConsumerARN can include random prefix, we add _assert_match_attribute function to check if the identifier match with certain pattern.

Testing:
A manually E2E test is performed, and confirmed the expected traces metrics and service maps are generated:

Trace:
Screenshot 2024-06-03 at 5 22 15 PM

Metrics:
Screenshot 2024-06-03 at 5 23 47 PM

Service Map:

Screenshot 2024-06-03 at 5 24 03 PM

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@zzhlogin zzhlogin requested a review from a team as a code owner June 3, 2024 17:38
@zzhlogin zzhlogin changed the title Enhance kinesis Consumer support (NOT READY) Enhance kinesis Consumer support Jun 4, 2024
Comment on lines +229 to +240
bucket_names: List[str] = [bucket["Name"] for bucket in s3_client.list_buckets()["Buckets"]]
put_bucket_name: str = "test-put-object-bucket-name"
if put_bucket_name not in bucket_names:
s3_client.create_bucket(
Bucket=put_bucket_name, CreateBucketConfiguration={"LocationConstraint": _AWS_REGION}
)

get_bucket_name: str = "test-get-object-bucket-name"
if get_bucket_name not in bucket_names:
s3_client.create_bucket(
Bucket=get_bucket_name, CreateBucketConfiguration={"LocationConstraint": _AWS_REGION}
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this needed? Same with changes to DDB, SQS sections? Is it because we are not refreshing the AWS container between tests?

Copy link
Contributor Author

@zzhlogin zzhlogin Jun 4, 2024

Choose a reason for hiding this comment

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

For each test, we will pull the "aws-application-signals-tests-botocore-app", and start a new container. Only the first container will succeed, all the following containers are fail with CreateBucket, exception:
[INFO] Unexpected exception occurred An error occurred (BucketAlreadyOwnedByYou) when calling the CreateBucket operation: Your previous request to create the named bucket succeeded and you already own it. Then the function directly return [None] without proceed with following code.

Same for other resources. So, for all the services, only the first container succeed with creating resources. All the following container fails. This was fine as long as we already created the resource before. But After adding kinesis_client consumer, we will have trouble, because we want to have different consumer_arn returned for each container.

Copy link
Contributor

@thpierce thpierce Jun 5, 2024

Choose a reason for hiding this comment

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

Interesting, thanks for investigating and improving!

Not blocking: Consider adding documentation to this logic that explains it really only runs once, to avoid confusion in future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants