Skip to content
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): Synthetics L2 Support #8824

Merged
merged 117 commits into from
Aug 14, 2020
Merged

feat(synthetics): Synthetics L2 Support #8824

merged 117 commits into from
Aug 14, 2020

Conversation

kaizencc
Copy link
Contributor

@kaizencc kaizencc commented Jul 1, 2020

This PR specifies an implementation for a CloudWatch Canary. It will be implemented in milestones specified below.

Closes #7687.

Currently, this PR implements up to Milestone 2.

Milestone 1: This implementation allows customers to specify a canary with these properties:

  • artifactBucket: IBucket
  • role: IRole
  • timeToLive: Duration
  • schedule: Schedule
  • startAfterCreation: boolean
  • successRetentionPeriod: Duration
  • failureRetentionPeriod: Duration
  • canaryName: string

And:

  • generate name for users if not specified
  • metric methods that reference those emitted by CloudWatch

Milestone 2: This is the next step needed to allow customers to specify their own canary scripts:

  • test:Test
    • Test class with static method Test.custom()
    • Code class with static method Code.inline()
  • Code.fromAsset()
    • validate correct folder structure for assets nodejs/node_modules
    • validate correct file name
  • Code.fromBucket()

Milestone 3: These are features that make a more robust Canary API:

  • static method Test.apiEndpoint()
  • additional static method templates for Test
  • support runConfig properties - memorySize and timeout

README Rendered version


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@kaizencc kaizencc requested a review from NetaNir July 1, 2020 00:27
@kaizencc kaizencc added the @aws-cdk/aws-synthetics Related to Amazon CloudWatch Synthetics label Jul 1, 2020
@kaizencc kaizencc changed the title feat(synthetics): added readme chore(synthetics): added readme Jul 1, 2020
@NetaNir NetaNir self-assigned this Jul 1, 2020
@kaizencc kaizencc marked this pull request as draft July 1, 2020 15:03
Copy link
Contributor

@NetaNir NetaNir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have pushed a few changes, mainly to the structure, and added a few comments inline.
A few general general comments:

  1. Add a short section in the end which lists all of the Canary Cfn resource properties with a short description, this will give the reader a place to comment on their implementation.
  2. Under future work you should add features/capabilities that might be implemented in the future. For example the idea of having a set of blueprints scripts for the canary code. This is a good opportunity to get feedback on ideas you might have, even if they will not be implemented in the initial version.
  3. If there are any non trivial things you have encountered while researching the Canary resource add them under a "Note" section, readers might have ideas to how you can tackle them. Of the top pf my head - when deleting a canary it's resources, like the lambda function are not deleted.

@mergify mergify bot dismissed NetaNir’s stale review July 13, 2020 17:35

Pull request has been modified.

@@ -0,0 +1,69 @@
import '@aws-cdk/assert/jest';
import { App, Stack } from '@aws-cdk/core';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a test that verifies a user can specify a custom runtime

@@ -0,0 +1,121 @@
import * as s3 from '@aws-cdk/aws-s3';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd actually move this entire file to canary.ts - its rather simple and avoid the clunky test-prop file name. We'll see what to do later if it grows too large.

@kaizencc kaizencc marked this pull request as ready for review August 13, 2020 20:16
//Wait for page to render.
//Increase or decrease wait time based on endpoint being monitored.
await page.waitFor(15000);
await synthetics.takeScreenshot('loaded', 'loaded');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
await synthetics.takeScreenshot('loaded', 'loaded');
await synthetics.takeScreenshot('loaded', 'loaded'); // This will take a snapshot that will be included in test output artifacts

});
```

The following is an example of an `index.js` file which exports the `handler` function, note that the function **must** be called `handler`:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
The following is an example of an `index.js` file which exports the `handler` function, note that the function **must** be called `handler`:
The following is an example of an `index.js` file which exports the `handler` function:

exports.handler = async () => {
return await pageLoadBlueprint();
};
```
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here you can add a note section

> the function must be named `handler` blah blah

Copy link
Contributor

@NetaNir NetaNir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few comments on the README, also I'd prefer we bring back the "Future work" sections which includes the a link to the blueprint issues. Lets ship it!

Comment on lines +21 to +29
To illustrate how to use a canary, assume your application defines the following endpoint:

```bash
% curl "https://api.example.com/user/books/topbook/"
The Hitchhikers Guide to the Galaxy

```

The below code defines a canary that will hit the `books/topbook` endpoint every 5 minutes:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The example has been adapted to show how to take a screenshot. The use case is not an API anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to show off the power of the synthetics library, as you mentioned in a previous comment. The heartbeat blueprint works for APIs as well, and takes a screenshot of the rendered HTML (in this case, would capture a screenshot of a white page with The Hitchhikers Guide to the Galaxy). I tested this on my own API endpoint to make sure.

While the screenshot is likely more powerful for websites, it can still be a valuable addition to APIs.

Copy link
Contributor

@NetaNir NetaNir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦄🎉🥳

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: ab1b4e0
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@NetaNir NetaNir dismissed stale reviews from iliapolo and RomainMuller August 14, 2020 20:48

stale

@mergify
Copy link
Contributor

mergify bot commented Aug 14, 2020

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).

@mergify mergify bot merged commit 691b349 into aws:master Aug 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-synthetics Related to Amazon CloudWatch Synthetics contribution/core This is a PR that came from AWS.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stabilize Amazon CloudWatch Synthetics module L2 construct for Synthetics Canary
7 participants