-
Notifications
You must be signed in to change notification settings - Fork 15
feat: Extract resource arn and remote resource access key for cross-account support #229
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
feat: Extract resource arn and remote resource access key for cross-account support #229
Conversation
|
Re-implement AWS SDK v3 Cross-Account Support with Recursion Fix This PR reintroduces the AWS SDK v3 cross-account support (previously reverted in #192) with fixes for the EKS E2E test failures. Background Issues Fixed
Implementation Details
Testing
Related |
462b89c to
7f1a447
Compare
aws-distro-opentelemetry-node-autoinstrumentation/src/patches/instrumentation-patch.ts
Outdated
Show resolved
Hide resolved
aws-distro-opentelemetry-node-autoinstrumentation/src/patches/instrumentation-patch.ts
Show resolved
Hide resolved
aws-distro-opentelemetry-node-autoinstrumentation/src/patches/instrumentation-patch.ts
Show resolved
Hide resolved
aws-distro-opentelemetry-node-autoinstrumentation/src/patches/instrumentation-patch.ts
Show resolved
Hide resolved
a6fb5f0 to
9cfb85c
Compare
aws-distro-opentelemetry-node-autoinstrumentation/test/patches/instrumentation-patch.test.ts
Outdated
Show resolved
Hide resolved
28cbc16 to
d979fe5
Compare
aws-distro-opentelemetry-node-autoinstrumentation/test/patches/instrumentation-patch.test.ts
Outdated
Show resolved
Hide resolved
aws-distro-opentelemetry-node-autoinstrumentation/test/patches/instrumentation-patch.test.ts
Outdated
Show resolved
Hide resolved
aws-distro-opentelemetry-node-autoinstrumentation/test/patches/instrumentation-patch.test.ts
Outdated
Show resolved
Hide resolved
63da336 to
c964852
Compare
aws-distro-opentelemetry-node-autoinstrumentation/test/patches/instrumentation-patch.test.ts
Outdated
Show resolved
Hide resolved
c964852 to
a490ce5
Compare
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.
Approving, but I'm not 100% sure about the pure reliance on OTel's Context Management to prevent this infinite recursion issue.
We've got a good unit test for this use case but I'm still a bit wary about not having a fallback mitigation for infinite recursion. Will rely on the @aws-observability/aws-application-signals-maintainers approver on whether or not we want to proceed with only this mitigation (please see this comment thread).
…ccount support (aws-observability#192) **Description of changes:** 1. Add auto-instrumentation support for the following AWS resources. - Populate `aws.stream.arn` in Span by extracting StreamArn from the request body. - Populate `aws.table.arn` in Span by extracting TableArn from the response body. 2. Add auto-instrumentation support for remote resource access key. - Populate `aws.auth.account.access_key` and `aws.auth.region` in Span from STS credentials in client config 3. Generate cross-account metrics attributes when remote resource identifier is present - If remote resource arn is available, extract account id and region from arn. - Otherwise, pass account access key id and region from span to metric if available. 4. Add unit tests, contract tests. Done E2E tests with CW agent. By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
d19f5d9 to
be3f78f
Compare
be3f78f to
9e036ee
Compare
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.
Re-approving since last review was stale
Description of changes:
aws.stream.arnin Span by extracting StreamArn from the request body.aws.table.arnin Span by extracting TableArn from the response body.aws.auth.account.access_keyandaws.auth.regionin Span from STS credentials in client configBy submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.