-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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(synthetics): add RunConfig parameters to Cloudwatch Synthetics Canary L2 construct #11865
Changes from all commits
214a0f0
7805f75
7cf20a6
4071407
61b059c
7eb7e19
4ec6f25
99870c0
3573348
696b194
42bb7e0
ff7405e
9bc867f
e769a67
7994724
a3c8504
42867b8
cc9db36
0ec44ad
92c60e1
4ad2928
d1ce352
9634d78
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -235,6 +235,46 @@ export interface CanaryProps { | |||||||||||
*/ | ||||||||||||
readonly test: Test; | ||||||||||||
|
||||||||||||
/** | ||||||||||||
* Key-value pairs that the Synthetics caches and makes available for your canary | ||||||||||||
* scripts. Use environment variables to apply configuration changes, such | ||||||||||||
* as test and production environment configurations, without changing your | ||||||||||||
* Canary script source code. | ||||||||||||
* | ||||||||||||
* @default - No environment variables. | ||||||||||||
*/ | ||||||||||||
readonly environment?: { [key: string]: string }; | ||||||||||||
|
||||||||||||
/** | ||||||||||||
* How long the canary is allowed to run before it must stop. | ||||||||||||
* You can't set this time to be longer than the frequency of the runs of this canary. | ||||||||||||
* If you omit this field, the frequency of the canary is used as this value, up to a maximum of 900 seconds. | ||||||||||||
* | ||||||||||||
* @default cdk.Duration.seconds(840) | ||||||||||||
*/ | ||||||||||||
readonly timeout?: cdk.Duration; | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are important caveats mentioned in the cloudformation documentation Please reflect this in your documentation There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It should be in the documentation as well as in the code, if the provided value is not valid we should throw an error. There has been a lot of discussion about this feature in #9300 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added to doc and commented in the code |
||||||||||||
|
||||||||||||
/** | ||||||||||||
* The maximum amount of memory that the canary can use while running. This value | ||||||||||||
* must be a multiple of 64. The range is 960 to 3008. | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||
* | ||||||||||||
* @default 960 | ||||||||||||
*/ | ||||||||||||
readonly memorySize?: number; | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This value must be a multiple of 64. The range is 960 to 3008. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added it to the doc but not sure if I should check the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this should use Size from @aws-cdk/core and also please add checks for the multiple of 64 and the min and max values |
||||||||||||
|
||||||||||||
/** | ||||||||||||
* Specifies whether this canary is to use active AWS X-Ray tracing when it runs. | ||||||||||||
* Active tracing enables this canary run to be displayed in the ServiceLens and X-Ray service maps | ||||||||||||
Comment on lines
+266
to
+267
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||
* even if the canary does not hit an endpoint that has X-ray tracing enabled. | ||||||||||||
* | ||||||||||||
* You can enable active tracing only for canaries that use version syn-nodejs-2.0 or later for their canary runtime. | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you add a check for this? it should be |
||||||||||||
* | ||||||||||||
* Enabling tracing increases canary run time by 2.5% to 7%. | ||||||||||||
* | ||||||||||||
* @default false | ||||||||||||
*/ | ||||||||||||
readonly tracing?: boolean; | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You need to give the canary permissions described here: https://docs.aws.amazon.com/AmazonCloudWatch/latest/monitoring/CloudWatch_Synthetics_Canaries_tracing.html There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please note that if a role is provided, it must also have permissions documented here: https://docs.aws.amazon.com/AmazonCloudWatch/latest/monitoring/CloudWatch_Synthetics_Canaries_tracing.html |
||||||||||||
|
||||||||||||
} | ||||||||||||
|
||||||||||||
/** | ||||||||||||
|
@@ -284,18 +324,21 @@ export class Canary extends cdk.Resource { | |||||||||||
encryption: s3.BucketEncryption.KMS_MANAGED, | ||||||||||||
}); | ||||||||||||
|
||||||||||||
this.role = props.role ?? this.createDefaultRole(props.artifactsBucketLocation?.prefix); | ||||||||||||
this.role = props.role ?? this.createDefaultRole(props.artifactsBucketLocation?.prefix, props.tracing); | ||||||||||||
|
||||||||||||
const schedule = this.createSchedule(props); | ||||||||||||
|
||||||||||||
const resource: CfnCanary = new CfnCanary(this, 'Resource', { | ||||||||||||
artifactS3Location: this.artifactsBucket.s3UrlForObject(props.artifactsBucketLocation?.prefix), | ||||||||||||
executionRoleArn: this.role.roleArn, | ||||||||||||
startCanaryAfterCreation: props.startAfterCreation ?? true, | ||||||||||||
runtimeVersion: props.runtime.name, | ||||||||||||
name: this.physicalName, | ||||||||||||
schedule: this.createSchedule(props), | ||||||||||||
schedule: schedule, | ||||||||||||
failureRetentionPeriod: props.failureRetentionPeriod?.toDays(), | ||||||||||||
successRetentionPeriod: props.successRetentionPeriod?.toDays(), | ||||||||||||
code: this.createCode(props), | ||||||||||||
runConfig: this.createRunConfig(props, schedule), | ||||||||||||
}); | ||||||||||||
|
||||||||||||
this.canaryId = resource.attrId; | ||||||||||||
|
@@ -339,7 +382,7 @@ export class Canary extends cdk.Resource { | |||||||||||
/** | ||||||||||||
* Returns a default role for the canary | ||||||||||||
*/ | ||||||||||||
private createDefaultRole(prefix?: string): iam.IRole { | ||||||||||||
private createDefaultRole(prefix?: string, tracingEnabled?: boolean): iam.IRole { | ||||||||||||
const { partition } = cdk.Stack.of(this); | ||||||||||||
// Created role will need these policies to run the Canary. | ||||||||||||
// https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-synthetics-canary.html#cfn-synthetics-canary-executionrolearn | ||||||||||||
|
@@ -365,6 +408,15 @@ export class Canary extends cdk.Resource { | |||||||||||
], | ||||||||||||
}); | ||||||||||||
|
||||||||||||
if (tracingEnabled) { | ||||||||||||
policy.addStatements( | ||||||||||||
new iam.PolicyStatement({ | ||||||||||||
resources: ['*'], | ||||||||||||
actions: ['xray:PutTraceSegments'], | ||||||||||||
}), | ||||||||||||
); | ||||||||||||
} | ||||||||||||
|
||||||||||||
return new iam.Role(this, 'ServiceRole', { | ||||||||||||
assumedBy: new iam.ServicePrincipal('lambda.amazonaws.com'), | ||||||||||||
inlinePolicies: { | ||||||||||||
|
@@ -400,6 +452,27 @@ export class Canary extends cdk.Resource { | |||||||||||
}; | ||||||||||||
} | ||||||||||||
|
||||||||||||
/** | ||||||||||||
* Retruns a runConfig object | ||||||||||||
*/ | ||||||||||||
private createRunConfig(props:CanaryProps, schedule: CfnCanary.ScheduleProperty): CfnCanary.RunConfigProperty { | ||||||||||||
// Cloudformation implementation made TimeoutInSeconds a required field where it should not (see links below). | ||||||||||||
// So here is a workaround to fix https://github.com/aws/aws-cdk/issues/9300 and still follow documented behavior. | ||||||||||||
// https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-synthetics-canary-runconfig.html#cfn-synthetics-canary-runconfig-timeoutinseconds | ||||||||||||
// https://github.com/aws-cloudformation/aws-cloudformation-resource-providers-synthetics/issues/31 | ||||||||||||
|
||||||||||||
const MAX_CANARY_TIMEOUT = 840; | ||||||||||||
const RATE_IN_SECONDS = Schedule.expressionToRateInSeconds(schedule.expression); | ||||||||||||
const DEFAULT_CANARY_TIMEOUT_IN_SECONDS = RATE_IN_SECONDS <= MAX_CANARY_TIMEOUT ? RATE_IN_SECONDS : MAX_CANARY_TIMEOUT; | ||||||||||||
Comment on lines
+464
to
+466
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no need for all caps here |
||||||||||||
|
||||||||||||
return { | ||||||||||||
timeoutInSeconds: props.timeout?.toSeconds() ?? DEFAULT_CANARY_TIMEOUT_IN_SECONDS, | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. needs to check this is smaller than the rate |
||||||||||||
activeTracing: props.tracing, | ||||||||||||
environmentVariables: props.environment, | ||||||||||||
memoryInMb: props.memorySize, | ||||||||||||
}; | ||||||||||||
} | ||||||||||||
|
||||||||||||
/** | ||||||||||||
* Creates a unique name for the canary. The generated name is the physical ID of the canary. | ||||||||||||
*/ | ||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -42,6 +42,33 @@ export class Schedule { | |
return new Schedule(`rate(${minutes} minutes)`); | ||
} | ||
|
||
/** | ||
* Convert a Schedule expression in to a number of seconds | ||
* | ||
* @param expression Schedule expression such as 'rate(2 minutes)' | ||
*/ | ||
public static expressionToRateInSeconds(expression: string): number { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. convert to a Duration |
||
// extract data isolating the number and units of the rate set | ||
const regularExpression = /^rate\(([0-9]*) ([a-z]*)\)/; | ||
const split = regularExpression.exec(expression); | ||
Comment on lines
+52
to
+53
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. prefer using just string checks instead of a regular expression |
||
const number = Number(split ? split[1] : '0'); | ||
const unit = split ? split[2] : 'minutes'; | ||
|
||
switch (unit) { | ||
case 'second': | ||
case 'seconds': | ||
return number; | ||
case 'minute': | ||
case 'minutes': | ||
return 60 * number; | ||
case 'hour': | ||
case 'hours': | ||
return 3600 * number; | ||
default: | ||
throw new Error('Unit not supported'); | ||
} | ||
} | ||
|
||
private constructor( | ||
/** | ||
* The Schedule expression | ||
|
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.
remove when merging from master