chore(cdk): Update ADOT Lambda Layer ARNS - v0.117.0 #34630
Conversation
There was a problem hiding this comment.
The pull request linter fails with the following errors:
❌ The first word of the pull request title should not be capitalized. If the title starts with a CDK construct, it should be in backticks "``".
If you believe this pull request should receive an exemption, please comment and provide a justification. A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed, add Clarification Request to a comment.
| @@ -1249,6 +1249,44 @@ export const PARAMS_AND_SECRETS_LAMBDA_LAYER_ARNS: { [version: string]: { [arch: | |||
| }; | |||
|
|
|||
| const ADOT_LAMBDA_LAYER_JAVA_SDK_ARNS: { [version: string]: { [arch: string]: { [region: string]: string } } } = { | |||
| '1.32.0-2': { | |||
| x86_64: { | |||
| 'ap-northeast-1': 'arn:aws:lambda:ap-northeast-1:901920570463:layer:aws-otel-java-wrapper-amd64-ver-1-32-0:6', | |||
There was a problem hiding this comment.
Instead of hardcoding each one, it would be better to write a function which prepares the ARN based on the region, platform, and version. This will significantly reduce the risk of typos/copy-paste errors and make that URL preparation function testable as well.
It is not necessary to change all the existing URLs to use the function, but for anything we add going forward, I would recommend this approach.
There was a problem hiding this comment.
got it, we would need invest some time, but certainly will take this consideration for future.
There was a problem hiding this comment.
It would be great if the change can be made in the current PR. The function should be quite straightforward to write and test.
The current change leaves a lot of scope for errors, both from the contributor and the reviewer, since it is not feasible to verify that every single ARN is correct.
| 'ap-south-1': 'arn:aws:lambda:ap-south-1:901920570463:layer:aws-otel-java-wrapper-amd64-ver-1-32-0:6', | ||
| 'ap-southeast-1': 'arn:aws:lambda:ap-southeast-1:901920570463:layer:aws-otel-java-wrapper-amd64-ver-1-32-0:6', | ||
| 'ap-southeast-2': 'arn:aws:lambda:ap-southeast-2:901920570463:layer:aws-otel-java-wrapper-amd64-ver-1-32-0:6', | ||
| 'ca-central-1': 'arn:aws:lambda:ca-central-1:901920570463:layer:aws-otel-java-wrapper-amd64-ver-1-32-0:6', |
There was a problem hiding this comment.
How is the :6 part of this ARN determined? I see the ARNs on aws-observability/aws-otel-lambda#1111, but it's not clear if there is a deterministic way to determine this part.
There was a problem hiding this comment.
that is because of the layer published with the same name aws-otel-java-wrapper-amd64-ver-1-32-0 for the past new releases but not necessarily a defined to determine, since we haven't bumped our sdk instrumentation version. we publish the layer arn's that are actively tested in our canaries.
There was a problem hiding this comment.
since we haven't bumped our sdk instrumentation version. we publish the layer arn's that are actively tested in our canaries.
Once the SDK instrumentation version is bumped, will the ARNs be updated? Just trying to understand if the current setup is temporary or long-term.
There was a problem hiding this comment.
Yes the sdk instrumentation version will be bumped along with the ARNs.
739e5dc to
d574e1d
Compare
This reverts commit 08ffaa46d93fc896e7293c964b4f7c9381f3d25a.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. Note that PRs with failing linting check or builds are not reviewed, please ensure your build is passing To prevent automatic closure:
This PR will automatically close in 14 days if no action is taken. |
Reason for this change
Update the fact tables and tests for lambda layers v0.115.0
aws-observability/aws-otel-lambda#1111
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license