-
Notifications
You must be signed in to change notification settings - Fork 4k
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(lambda): add support for auto-instrumentation with ADOT Lambda layer #23027
Conversation
packages/@aws-cdk/core/lib/stack.ts
Outdated
const lookupMap = partitions ? RegionInfo.limitedRegionMap(factName, partitions) : RegionInfo.regionMap(factName); | ||
const lookupMap = | ||
partitions !== undefined && partitions !== 'undefined' | ||
? RegionInfo.limitedRegionMap(factName, partitions) | ||
: RegionInfo.regionMap(factName); |
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.
These changes belong to a commit that was submitted in a separate PR: #23023
I have to rebase-on-top/include this commit in this PR because without it, the added integration test will fail.
I may have missed it but how does the lookup resolve the layer version. For example |
Ahh, the answer lies in |
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.
Thanks for the PR! It looks like there are a couple of things that we need to
add.
-
From the docs it looks like enabling the layer requires you to also:
- Enable active tracing
- Set the
AWS_LAMBDA_EXEC_WRAPPER
env variable - Go & .NET have some special requirements. We should handle what we can
(e.g. check that Go lambdas use provided.al2) and point users to the docs
for info on how to configure the rest
-
Version support. I know we use
LATEST
as defaults elsewhere, but we are
trying to get away from that. What if we turned theAdotLambdaLayerType
into an enum-like class
and allowed the user to specify the version? Something like:
export class AdotJavaVersion {
/**
* Will always use the latest layer version. New versions could
* introduce incompatable changes. Make sure to test new versions.
*/
public static LATEST = new AdotJavaVersion('LATEST');
public static 1_19_0_1 = new AdotJavaVersion('1-19-0:1');
constructor(public readonly version: string) {}
}
export class AdotLambdaLayerType {
public static fromJavaVersion(version: AdotJavaVersion, architecture?: Architecture): AdotLambdaLayerType {
...
}
}
The user can still use LATEST
if they want to, but they need to opt-in.
@@ -500,3 +500,218 @@ export const FIREHOSE_CIDR_BLOCKS: { [region: string]: string } = { | |||
'us-west-1': '13.57.135.192', | |||
'us-west-2': '52.89.255.224', | |||
}; | |||
|
|||
const ADOT_LAMBDA_LAYER_JAVA_SDK_ARNS: { [version: string]: { [arch: string]: { [region: string]: string } } } = { |
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.
Do we need to hard code the account id multiple times? Can't we generate all the permutations of region/accountid/language programmatically?
I know that you are following what was in the repo before, but I'm curious if we cannot consider this. It is very easy for a typo to happen here.
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.
I did try to work out a way to avoid dumping a bunch of hard-coded ARNs like this. But the ARN patterns are not consistent across three dimensions (i.e., type, region and architectures). For example, see the note about arm64
and ca-central-1
for Java SDK lambda type. We can shorten these code with some code logic, but that code looks equally bad to me.
I talked with ADOT folks internally and the plan is to generate this list automatically from their Lambda layer's CI/CD.
|
@corymhall thanks for the feedback!
I'd describe that in a slightly different way: enabling auto-instrumentation requires you to add the layer and those extra steps. I emphasized that slight difference because one can legitimately acquire an ADOT layer just to consume the libraries that it offers and not to enable instrumentation (thus does not need to perform those extra steps). I can add support for those steps if we prefer coupling this way. I would need to change the API from
I saw that we did this with CloudWatch Insights and I tried to follow the pattern. But because we have 5 different Lambda layer types and each of them has their own version line, the code quickly got messy. The fact that ADOT has inconsistent layer revisions for the same layer version makes the problem worse. For example:
|
Pull request has been modified.
Ok I didn't realize that was a valid use case. Could we do something similar to
That is unfortunate for the user experience, but we need to make the user |
@corymhall the latest revision should address your comments. Few notes:
|
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.
Looking good! Couple more comments.
readonly version: | ||
| AdotLambdaLayerJavaSdkVersion | ||
| AdotLambdaLayerJavaAutoInstrumentationVersion | ||
| AdotLambdaLayerJavaScriptSdkVersion | ||
| AdotLambdaLayerPythonSdkVersion | ||
| AdotLambdaLayerGenericVersion; |
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.
jsii doesn't allow union types (I'm not sure why the build is passing actually).
What if we combined the type with the version?
export abstract class AdotLambdaLayerType {
public static fromJavaSdkVersion(version: AdotLambdaLayerJavaSdkVersion, execWrapper: string): AdotLambdaLayerType {
return new JavaSdkVersionLayer({
version: version.version,
type: AdotLayerType.JAVA_SDK,
});
}
public abstract bind(scope: Construct, handler: IFunction, options?: LayerBindOptions): AdotLambdaLayerConfig;
}
class JavaSdkVersionLayer extends AdotLambdaLayerType {
constructor(private readonly options: JavaSdkVersionLayerOptions) {
super();
}
public bind(scope: Construct, handler: IFunction, options: LayerBindOptions = {}): AdotLambdaLayerConfig {
// can do the individual runtime checks here, just an example
if (handler.runtime === Runtime.JAVA_8) {
throw new Error(...);
}
return {
version: this.options.version,
architecture: handler.architecture,
type: this.options.type,
};
}
}
Then in the addAdotLayer method you can just do
const config = props.type.bind(this, this);
AdotLambdaLayer.fromLayerAttributes(this, 'Layer', config);
The user would do something like
handler.addAdotLayer({
type: AdotLambdaLayerType.fromJavaSdkVersion(AdotLambdaLayerJavaSdkVersion.V1_9_1),
})
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.
JSII does allow union types, it just does not work in other languages the same way as it does in TypeScript.
Since all those *Version
classes inherits a base class, how about we just use that base class instead? My latest revision demonstrates the idea. But if you still prefer to have a specific function for each ADOT layer type (which I honestly want to avoid), I can do that.
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.
Why do you want to avoid the specific functions per type? I think you end up
with a better user (and contributor) experience. With the currently
implementation we need to input the same type of information multiple places,
and make sure that the user has input the correct combination.
Pull request has been modified.
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
readonly version: | ||
| AdotLambdaLayerJavaSdkVersion | ||
| AdotLambdaLayerJavaAutoInstrumentationVersion | ||
| AdotLambdaLayerJavaScriptSdkVersion | ||
| AdotLambdaLayerPythonSdkVersion | ||
| AdotLambdaLayerGenericVersion; |
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.
Why do you want to avoid the specific functions per type? I think you end up
with a better user (and contributor) experience. With the currently
implementation we need to input the same type of information multiple places,
and make sure that the user has input the correct combination.
Too much boilerplate code and extra interfaces are the main reason. For each lambda layer type there will be one new I agree that customer experience will be better with that. Let me give that a try anyway. |
Note that the implementation you suggested does not stop users from inputting the wrong combination of type and version. For example, with these declarations: class JavaSdkVersion {
public static readonly V1_19_0 = new JavaSdkVersion('1.19.0');
protected constructor(public readonly version: string) {}
}
class PythonSdkVersion {
public static readonly V1_13_0 = new PythonSdkVersion('1.13.0');
protected constructor(public readonly version: string) {}
}
function createJavaLayer(version: JavaSdkVersion) {
console.log("Run just fine");
} You would think that this function invocation won't be allowed, but it's actually accepted thanks to TypeScript being createJavaLayer(PythonSdkVersion.V1_13_0); |
Pull request has been modified.
The latest revision is ready for review now. It should be very close to what you suggested in term of customer experience. Also, I use the "branding" trick to work around the issue of structural typing. I'm a bit unsure about the usage though: fn.addAdotInstrumentation({
layer: AdotLambdaLayer.forJavaSdk(fn, 'LambdaLayer', AdotLambdaLayerJavaSdkVersion.LATEST);
execWrapper: AdotLambdaExecWrapper.REGULAR_HANDLER,
}); Users will have to reference the Lambda function |
The module fn.addAdotInstrumentation({
layerVersion: AdotLambdaLayerJavaSdkVersion.V1_19_0,
execWrapper: AdotLambdaExecWrapper.REGULAR_HANDLER,
}); I'm going to call it a day now. Let me know if you have any question there. |
runtime: lambda.Runtime.NODEJS_18_X, | ||
handler: 'index.handler', | ||
code: lambda.Code.fromInline('exports.handler = function(event, ctx, cb) { return cb(null, "hi"); }'), | ||
adotInstrumentationConfig: { |
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.
Can we model this differently?
const fn = new lambda.Function(this, 'MyFunction', {
adotLayer: AdotLambdaLayer.forJavaScriptSdk(
AdotLambdaLayerJavaScriptSdkVersion.V1_7_0,
AdotLambdaExecWrapper.REGULAR_HANDLER, // could we have a default for this?
});
This way my editor will show me that I need to provide a AdotLambdaLayer
and I can type that in and use code completion to show me all the options.
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.
We should avoid confusing users about two slightly different concepts here: ADOT instrumentation and ADOT Lambda layer. Enabling ADOT instrumentation means two independent activities:
- Adding an ADOT Lambda layer to the Lambda function.
- Configuring the Lambda function to use an exec wrapper that's compatible with ADOT.
So setting a value for the exec wrapper means configuring the instrumentation, not configuring the layer. I'm afraid that the proposed usage suggests the opposite.
What do you think about this?
const fn = new lambda.Function(this, 'MyFunction', {
...
adotInstrumentationConfig: AdotInstrumentationConfig.forJavaScriptSdk(
AdotLambdaLayerJavaScriptSdkVersion.V1_7_0,
AdotLambdaExecWrapper.REGULAR_HANDLER, // optional, defaulted to REGULAR_HANDLER
),
...
});
One thing I don't like about this approach though, is that we will have to introduce 5 more helper functions under AdotLambdaConfig
. But I'm happy to go with it if it's right for users.
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.
Ok sure lets go with adotInstrumentation
then (drop the Config
).
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.
Sure! I've added a new commit to address this.
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.
@huyphan I created a
gist of
what I think a good user experience would be. You would consume it like
new lambda.Function(stack, 'MyLambda', {
code: new lambda.InlineCode('foo'),
handler: 'index.handler',
runtime: lambda.Runtime.NODEJS_14_X,
adotInstrumentation: {
layerVersion: lambda.AdotLayerVersion.fromJavaSdkLayerVersion(lambda.AdotLambdaLayerJavaSdkVersion.LATEST),
execWrapper: lambda.AdotLambdaExecWrapper.REGULAR_HANDLER,
},
});
What do you think?
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.
I'm a bit skeptical with that experience. When I tried that new change in the code and wrote this line as a user:
layerVersion: lambda.AdotLayerVersion.fromJavaSdkLayerVersion(lambda.AdotLambdaLayerJavaSdkVersion.LATEST),
... two things stood out for me:
- When I typed up to
layerVersion: lambda.
, the auto-complete feature suggested me a bunch of options. The SDK-specific options are showed up at the top andAdotLayerVersion
, which is the only one I can use, is near to the bottom. Most of the times, users know what SDK they need, so they would tend to choose those SDK-specific option first and get stuck. - I have to type the same Layer type (i.e.,
JavaSdk
) twice on that line.
Given those concerns, if you think they are minor enough, I'm happy to update the code to align with that experience.
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.
But my editor will show me that layerVersion
takes an AdotLayerVersion
type so I can just type that in and when I get to the .
it will show me all the options. The alternative that you propose is that it would show me that it takes a type of AdotLambdaLayerVersion
and I have to just know which other classes implement that. Nothing in the editor will help me out.
I have to type the same Layer type (i.e., JavaSdk ) twice on that line.
Yeah I get that, but that's just the way the versions work right? If every language had the same versions then this would be easier. We use this model elsewhere, for example the rds.DatabaseCluster.engine
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.
I've updated the code (the diff on top of old commits) to make the UX aligned with what you proposed. The code is a bit different from your gist since I want to avoid exposing irrelevant props to users. Let me know if you need me to change anything else.
Pull request has been modified.
runtime: lambda.Runtime.NODEJS_18_X, | ||
handler: 'index.handler', | ||
code: lambda.Code.fromInline('exports.handler = function(event, ctx, cb) { return cb(null, "hi"); }'), | ||
adotInstrumentationConfig: { |
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.
@huyphan I created a
gist of
what I think a good user experience would be. You would consume it like
new lambda.Function(stack, 'MyLambda', {
code: new lambda.InlineCode('foo'),
handler: 'index.handler',
runtime: lambda.Runtime.NODEJS_14_X,
adotInstrumentation: {
layerVersion: lambda.AdotLayerVersion.fromJavaSdkLayerVersion(lambda.AdotLambdaLayerJavaSdkVersion.LATEST),
execWrapper: lambda.AdotLambdaExecWrapper.REGULAR_HANDLER,
},
});
What do you think?
Pull request has been modified.
There was a small issue with the README change in my last commit. But it's all sorted now. |
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.
@huyphan looks great! thanks for being patient and sticking with me on this one! 🚀
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
…ayer (aws#23027) AWS Distro for OpenTelemetry (ADOT) layers provide OpenTelemetry agent with an out-of-the-box configuration for AWS Lambda and AWS X-Ray. For the popular runtimes (Java, Python, Node), it provides automatic instrumentation without code change. This commit allows CDK users to easily add an ADOT Lambda layer to their Lambda function with the new prop `adotInstrumentationConfig`. For example: ```ts const fn = new lambda.Function(this, 'MyFunction', { ... adotInstrumentation: { layerVersion: AdotLayerVersion.fromJavaScriptSdkLayerVersion(AdotLambdaLayerJavaScriptSdkVersion.V1_7_0), execWrapper: AdotLambdaExecWrapper.REGULAR_HANDLER, } ... }); ``` To support this feature, we provides the following a new classes, enum, and helper functions to users: * `AdotLambdaExecWrapper` is the enum of all possible wrapper script that's provided by ADOT Lambda layer and must be set to the `AWS_LAMBDA_EXEC_WRAPPER` environment variable * `AdotLambdaLayer*Version` are five classes representing five different Lambda layer types offered by ADOT. * `AdotLayerVersion.from*LayerVersion` are five helper functions that return the layer ARN for the Lambda function during synthesis time. If users want to retrieve the ARN independently from enabling ADOT in the Lambda function, they can use the following pattern: ```ts declare const fn: lambda.Function; const layerArn = lambda.AdotLambdaLayerJavaSdkVersion.V1_19_0.layerArn(fn.stack, fn.architecture); ``` ---- ### All Submissions: * [X] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [X] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [X] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
…ayer (aws#23027) AWS Distro for OpenTelemetry (ADOT) layers provide OpenTelemetry agent with an out-of-the-box configuration for AWS Lambda and AWS X-Ray. For the popular runtimes (Java, Python, Node), it provides automatic instrumentation without code change. This commit allows CDK users to easily add an ADOT Lambda layer to their Lambda function with the new prop `adotInstrumentationConfig`. For example: ```ts const fn = new lambda.Function(this, 'MyFunction', { ... adotInstrumentation: { layerVersion: AdotLayerVersion.fromJavaScriptSdkLayerVersion(AdotLambdaLayerJavaScriptSdkVersion.V1_7_0), execWrapper: AdotLambdaExecWrapper.REGULAR_HANDLER, } ... }); ``` To support this feature, we provides the following a new classes, enum, and helper functions to users: * `AdotLambdaExecWrapper` is the enum of all possible wrapper script that's provided by ADOT Lambda layer and must be set to the `AWS_LAMBDA_EXEC_WRAPPER` environment variable * `AdotLambdaLayer*Version` are five classes representing five different Lambda layer types offered by ADOT. * `AdotLayerVersion.from*LayerVersion` are five helper functions that return the layer ARN for the Lambda function during synthesis time. If users want to retrieve the ARN independently from enabling ADOT in the Lambda function, they can use the following pattern: ```ts declare const fn: lambda.Function; const layerArn = lambda.AdotLambdaLayerJavaSdkVersion.V1_19_0.layerArn(fn.stack, fn.architecture); ``` ---- ### All Submissions: * [X] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [X] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [X] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
AWS Distro for OpenTelemetry (ADOT) layers provide OpenTelemetry agent with an out-of-the-box configuration for AWS Lambda and AWS X-Ray. For the popular runtimes (Java, Python, Node), it provides automatic instrumentation without code change. This commit allows CDK users to easily add an ADOT Lambda layer to their Lambda function with the new prop
adotInstrumentationConfig
. For example:To support this feature, we provides the following a new classes, enum, and helper functions to users:
AdotLambdaExecWrapper
is the enum of all possible wrapper script that's provided by ADOT Lambda layer and must be set to theAWS_LAMBDA_EXEC_WRAPPER
environment variableAdotLambdaLayer*Version
are five classes representing five different Lambda layer types offered by ADOT.AdotLayerVersion.from*LayerVersion
are five helper functions that return the layer ARN for the Lambda function during synthesis time.If users want to retrieve the ARN independently from enabling ADOT in the Lambda function, they can use the following pattern:
All Submissions:
Adding new Unconventional Dependencies:
New Features
yarn integ
to deploy the infrastructure and generate the snapshot (i.e.yarn integ
without--dry-run
)?By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license