-
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(codeguruprofiler): ProfilingGroup #7895
Conversation
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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.
Looks pretty solid. See minor comments.
/** | ||
* A name for the profiling group. | ||
*/ | ||
readonly profilingGroupName: 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.
Can this be omitted? Can we produce a sensible default 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've changed the approach to the following, not an expert by any means, let me know your thoughts.
physicalName: props.profilingGroupName || Lazy.stringValue({ produce: () => this.node.uniqueId })
@@ -9,8 +9,28 @@ | |||
--- | |||
<!--END STABILITY BANNER--> | |||
|
|||
This module is part of the [AWS Cloud Development Kit](https://github.com/aws/aws-cdk) project. | |||
Amazon CodeGuru Profiler collects runtime performance data from your live applications, and provides recommendations that can help you fine-tune your application performance. Using machine learning algorithms, CodeGuru Profiler can help you find your most expensive lines of code and suggest ways you can improve efficiency and remove CPU bottlenecks. Amazon CodeGuru Profiler provides different visualizations of profiling data to help you identify what code is running on the CPU, see how much time is consumed, and suggest ways to reduce CPU utilization. |
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.
limit to 100 chars
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.
reduced to 2 sentences, hope that's okay.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
packages/@aws-cdk/aws-codeguruprofiler/test/integ.profiler-group.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-codeguruprofiler/test/profiling-group.test.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-codeguruprofiler/lib/profiling-group-base.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-codeguruprofiler/lib/profiling-group-base.ts
Outdated
Show resolved
Hide resolved
|
||
constructor(scope: Construct, id: string, props: ProfilingGroupProps) { | ||
super(scope, id, { | ||
physicalName: props.profilingGroupName || Lazy.stringValue({ produce: () => this.node.uniqueId }), |
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.
physicalName: props.profilingGroupName || Lazy.stringValue({ produce: () => this.node.uniqueId }), | |
physicalName: props.profilingGroupName ?? Lazy.stringValue({ produce: () => this.node.uniqueId }), |
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.
Also - are there any constraints on the profile group name that we should take into account when using unqiueId?
packages/@aws-cdk/aws-codeguruprofiler/test/integ.profiler-group.ts
Outdated
Show resolved
Hide resolved
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
…nd setup functions to add publish/read policies to IGrantable
…nd setup functions to add publish/read policies to IGrantable
…nd setup functions to add publish/read policies to IGrantable
…nd setup functions to add publish/read policies to IGrantable
…nd setup functions to add publish/read policies to IGrantable
…nd setup functions to add publish/read policies to IGrantable
…nd setup functions to add publish/read policies to IGrantable
…nd setup functions to add publish/read policies to IGrantable
…nd setup functions to add publish/read policies to IGrantable
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). |
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). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
L2 constructs were introduced in #7895 about 5 months ago. We missed marking the module as experimental. Updated stability and updated the banner in the README.
) L2 constructs were introduced in #7895 about 5 months ago. We missed marking the module as experimental. Updated stability and updated the banner in the README. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
fixes #6984 by creating L2 construct and functions to allow for policies to be assigned to execution roles.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license