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): add activeTracing, memory and timeout property to Canary class #30556

Merged
merged 22 commits into from
Jul 31, 2024

Conversation

mazyu36
Copy link
Contributor

@mazyu36 mazyu36 commented Jun 15, 2024

Issue # (if applicable)

Closes #9300, #14086, #28152

Reason for this change

In order to make the properties in the runConfig of the L1 Construct configurable in the L2 Construct.

At the moment, the L2 Canary Class only supports environmentVariables in the runConfig.

Description of changes

Add missing properties.

  • activeTracing
  • memory
  • timeout

Note: The following is stated in #9300, but when tested, it was possible to set only memoryInMb (memory).

The difficulty is that timeoutInSeconds is required if runConfig is set, so one cannot only specify memoryInMb.

Test code is here.

class TestStack extends Stack {
  constructor(scope: Construct, id: string, props?: StackProps) {
    super(scope, id, props);

    new synthetics.Canary(this, 'Canary', {
      canaryName: 'next',
      runtime: synthetics.Runtime.SYNTHETICS_NODEJS_PUPPETEER_7_0,
      test: synthetics.Test.custom({
        handler: 'index.handler',
        code: synthetics.Code.fromInline(`
          exports.handler = async () => {
            console.log(\'hello world\');
          };`),
      }),
      cleanup: synthetics.Cleanup.LAMBDA,
      memory: Size.mebibytes(2048),
    });
  }
}

Description of how you validated changes

Add unit tests and integ tests.

Checklist


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

@github-actions github-actions bot added feature-request A feature should be added or improved. p2 labels Jun 15, 2024
@aws-cdk-automation aws-cdk-automation requested a review from a team June 15, 2024 12:46
@github-actions github-actions bot added the star-contributor [Pilot] contributed between 25-49 PRs to the CDK label Jun 15, 2024
return undefined;
}
const activeTracingNotSupportedRuntime = [
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The error messages are taken directly from the documentation.
However, since syn-nodejs-2.0 and earlier versions are no longer available, I am only setting it for the currently available runtimes that do not support active tracing (see also docs).

I was unsure about including this validation, so if you have any thoughts or feedback, please let me know.

Copy link
Contributor

Choose a reason for hiding this comment

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

What about checking props.runtime.family !== RuntimeFamily.NODEJS like in the other PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you. I made the correction since checking the runtime is better.
(Even though I did it myself in another PR...lol)

@mazyu36 mazyu36 marked this pull request as draft June 15, 2024 13:09
@mazyu36 mazyu36 marked this pull request as ready for review June 15, 2024 13:47
@aws-cdk-automation aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jun 15, 2024
Copy link
Contributor

@lpizzinidev lpizzinidev left a comment

Choose a reason for hiding this comment

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

Thanks 👍 Left some comments for adjustments

packages/aws-cdk-lib/aws-synthetics/lib/canary.ts Outdated Show resolved Hide resolved
packages/aws-cdk-lib/aws-synthetics/lib/canary.ts Outdated Show resolved Hide resolved
packages/aws-cdk-lib/aws-synthetics/lib/canary.ts Outdated Show resolved Hide resolved
* The maximum amount of memory that the canary can use while running.
* This value must be a multiple of 64 Mib.
*
* @default Size.mebibytes(5)
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure about this default? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you. I made the correction since 1024MB is the default.

return undefined;
}
const activeTracingNotSupportedRuntime = [
Copy link
Contributor

Choose a reason for hiding this comment

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

What about checking props.runtime.family !== RuntimeFamily.NODEJS like in the other PR?

packages/aws-cdk-lib/aws-synthetics/README.md Outdated Show resolved Hide resolved
packages/aws-cdk-lib/aws-synthetics/README.md Outdated Show resolved Hide resolved
packages/aws-cdk-lib/aws-synthetics/README.md Outdated Show resolved Hide resolved
@aws-cdk-automation aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jun 16, 2024
mazyu36 and others added 12 commits June 17, 2024 16:45
Co-authored-by: Luca Pizzini <lpizzini7@gmail.com>
Co-authored-by: Luca Pizzini <lpizzini7@gmail.com>
Co-authored-by: Luca Pizzini <lpizzini7@gmail.com>
Co-authored-by: Luca Pizzini <lpizzini7@gmail.com>
Co-authored-by: Luca Pizzini <lpizzini7@gmail.com>
Co-authored-by: Luca Pizzini <lpizzini7@gmail.com>
Co-authored-by: Luca Pizzini <lpizzini7@gmail.com>
Co-authored-by: Luca Pizzini <lpizzini7@gmail.com>
Co-authored-by: Luca Pizzini <lpizzini7@gmail.com>
Co-authored-by: Luca Pizzini <lpizzini7@gmail.com>
Co-authored-by: Luca Pizzini <lpizzini7@gmail.com>
Co-authored-by: Luca Pizzini <lpizzini7@gmail.com>
@mazyu36 mazyu36 force-pushed the synthetics-runconfig branch 3 times, most recently from 6ec1bdc to ae0b174 Compare June 17, 2024 08:57
@mazyu36
Copy link
Contributor Author

mazyu36 commented Jun 17, 2024

@lpizzinidev
Thank you for always reviewing. I have addressed all comments.

@lpizzinidev
Copy link
Contributor

@mazyu36 Thanks for the changes 👍 Answered https://github.com/aws/aws-cdk/pull/30556/files#r1643336431

@github-actions github-actions bot added the effort/medium Medium work item – several days of effort label Jun 18, 2024
@mazyu36
Copy link
Contributor Author

mazyu36 commented Jun 18, 2024

@lpizzinidev
Thanks.
I've added a validation, and answered https://github.com/aws/aws-cdk/pull/30556/files#r1643641842

Copy link
Contributor

@lpizzinidev lpizzinidev left a comment

Choose a reason for hiding this comment

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

Nice 👍

packages/aws-cdk-lib/aws-synthetics/lib/canary.ts Outdated Show resolved Hide resolved
@aws-cdk-automation aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Jun 18, 2024
@paulhcsun paulhcsun self-assigned this Jul 29, 2024
Copy link
Contributor

@paulhcsun paulhcsun left a comment

Choose a reason for hiding this comment

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

Nice work @mazyu36! Thanks for the contribution!

Just have one minor wording change for the error message and I'd be happy to approve.


// Only check runtime family is nodejs because versions prior to syn-nodejs-2.0 are deprecated and can no longer be configured.
if (props.activeTracing && !cdk.Token.isUnresolved(props.runtime.family) && props.runtime.family !== RuntimeFamily.NODEJS) {
throw new Error('You can enable active tracing only for canaries that use version `syn-nodejs-2.0` or later for their canary runtime.');
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
throw new Error('You can enable active tracing only for canaries that use version `syn-nodejs-2.0` or later for their canary runtime.');
throw new Error('You can only enable active tracing for canaries that use canary runtime version `syn-nodejs-2.0` or later.');

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@paulhcsun
Thanks.
I've updated error message and unit tests.

@mazyu36 mazyu36 requested a review from paulhcsun July 31, 2024 00:02
@paulhcsun
Copy link
Contributor

@Mergifyio update

Copy link
Contributor

mergify bot commented Jul 31, 2024

update

☑️ Nothing to do

  • #commits-behind>0 [📌 update requirement]
  • -closed [📌 update requirement]
  • -conflict [📌 update requirement]
  • queue-position=-1 [📌 update requirement]

Copy link
Contributor

mergify bot commented Jul 31, 2024

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-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: ee9f60d
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@mergify mergify bot merged commit 9295a85 into aws:main Jul 31, 2024
9 checks passed
Copy link
Contributor

mergify bot commented Jul 31, 2024

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

Copy link

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 31, 2024
@mazyu36 mazyu36 deleted the synthetics-runconfig branch July 31, 2024 22:03
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2 pr/needs-maintainer-review This PR needs a review from a Core Team Member star-contributor [Pilot] contributed between 25-49 PRs to the CDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[synthetics] support runConfig properties
4 participants