-
Notifications
You must be signed in to change notification settings - Fork 4.4k
feat(synthetics): Update Cloudwatch Synthetics canaries NodeJS runtimes #11866
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
Conversation
|
Title does not follow the guidelines of Conventional Commits. Please adjust title before merge. |
a086868 to
17a7e1d
Compare
17a7e1d to
7c09f34
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.
See my comments about the Python runtime additions. The Canary module assume the runtime is always Nodejs, and enforce the below:
- The Canary code does include
node_moduelsfolder. See synthetics docs: https://docs.aws.amazon.com/AmazonCloudWatch/latest/monitoring/CloudWatch_Synthetics_Canaries_WritingCanary_Nodejs.html - The handler file is named
index.handler
Before we introduce Python as runtime we need to address it. Im happy to accept a PR which adds the new node runtime in the meantime
| code: synthetics.Code.fromInline('/* Synthetics handler code */'), | ||
| }), | ||
| runtime: synthetics.Runtime.SYNTHETICS_NODEJS_2_0, | ||
| runtime: synthetics.Runtime.SYNTHETICS_NODEJS_2_2, |
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 would actually prefer we leave some of the test using other values, as they are valid configuration.
I don't see a reason to change all tests to use the latest version.
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.
It was mainly because the linter was asking for unit test update.
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 was this comment resolve? The linter rule can be exempt. Lets change it back
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.
Sorry about that I didn't know it can be exempt. reverting to 2_0
| * | ||
| * @see https://docs.aws.amazon.com/AmazonCloudWatch/latest/monitoring/CloudWatch_Synthetics_Library_python_selenium.html#CloudWatch_Synthetics_runtimeversion-syn-python-selenium-1.0 | ||
| */ | ||
| public static readonly SYNTHETICS_PYTHON_1_0 = new Runtime('syn-python-selenium-1.0'); |
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.
Does this means that synthetics now support non NodeJs lambda functions? If so we need to change how we handle the assets code, which assumes the code is stored in a node_module folder, per synthetics official docs .
This will require a larger code change, Im not sure what is the right API here, we might need separate class for Python runtimes Canaries and Nodejs Canaries.
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.
Thoughts?
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'll do another PR for Python one if it's ok. I removed python for now.
| handler: 'index.handler', | ||
| }), | ||
| runtime: synthetics.Runtime.SYNTHETICS_NODEJS_2_0, | ||
| runtime: synthetics.Runtime.SYNTHETICS_NODEJS_2_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.
why did you use SYNTHETICS_NODEJS_2_1 and not SYNTHETICS_NODEJS_2_2 in all code samples? If the answer is "It does not matter" then it shouldn't be changed
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.
right. my bad , pushed 2_2 in all examples to guide customer towards latest version.
Pull request has been modified.
|
@NetaNir anything else I missed ? |
|
|
||
| The Canary code will be executed in a lambda function created by Synthetics on your behalf. The Lambda function includes a custom [runtime](https://docs.aws.amazon.com/AmazonCloudWatch/latest/monitoring/CloudWatch_Synthetics_Canaries_Library.html) provided by Synthetics. The provided runtime includes a variety of handy tools such as [Puppeteer](https://www.npmjs.com/package/puppeteer-core) and Chromium. The default runtime is `syn-nodejs-2.0`. | ||
| The Canary code will be executed in a lambda function created by Synthetics on your behalf. The Lambda function includes a custom [runtime](https://docs.aws.amazon.com/AmazonCloudWatch/latest/monitoring/CloudWatch_Synthetics_Canaries_Library.html) provided by Synthetics. The provided runtime includes a variety of handy tools such as [Puppeteer](https://www.npmjs.com/package/puppeteer-core) (for nodejs based one) and Chromium. | ||
|
|
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.
This should have been removed when the runtime property was made a required property. Can you remove it (instead of fixing it)
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 removed it, I just mentioned that pupetter is for nodejs runtime only since python one use selenium. But If you prefer I'll keep that for python PR.
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 master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Cloudwatch Synthetics recently released new NodeJS runtimes (https://docs.aws.amazon.com/AmazonCloudWatch/latest/monitoring/CloudWatch_Synthetics_Library_nodejs_puppeteer.html).
This PR is adding them and update doc links
Fixes #11870
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license