feat(applicationsignals-alpha): introduce Application Signals SLO L2 constructs #33987
Conversation
github-actions
bot
added
p2
beginning-contributor
There was a problem hiding this comment.
The pull request linter fails with the following errors:
❌ Features must contain a change to a README file.
❌ Features must contain a change to an integration test file and the resulting snapshot.
If you believe this pull request should receive an exemption, please comment and provide a justification. A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed, add Clarification Request to a comment.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. Note that PRs with failing linting check or builds are not reviewed, please ensure your build is passing To prevent automatic closure:
This PR will automatically close in 14 days if no action is taken. |
| export const DEFAULT_GOAL_CONFIG = { | ||
| ATTAINMENT_GOAL: 99.9, | ||
| WARNING_THRESHOLD: 30, | ||
| } as const; |
There was a problem hiding this comment.
either move these constants as static properties in some class and it is better to be the classes where these values be used, or make them private. Do we need to add these values in CDK Docs, and let the customers use them ?
| export * from './slo/slo'; | ||
| export * from './slo/slo-types'; |
There was a problem hiding this comment.
add an index.ts in the slo dir, and expose whatever you want there. Here, add only the directory
| /** | ||
| * Types of services that can be monitored | ||
| */ | ||
| export enum KeyAttributeType { |
There was a problem hiding this comment.
I see some difference between the implementation and the RFC. RFC suggested more values (RESOURCE, AWS::RESOURCE)
| /** | ||
| * Interface defining SLO behavior and runtime properties | ||
| */ | ||
| export interface ISlo { |
There was a problem hiding this comment.
I believe this is the L2 construct interface, so I prefer use the I<> format. so, it should be IServiceLevelObjective. Also, in comment, mention that this is the Interface for L2 construct of the resource AWS::ApplicationSignals::ServiceLevelObjective
| /** | ||
| * Binds the SLO configuration to L1 construct properties | ||
| * Used internally by CDK to generate CloudFormation | ||
| * | ||
| * @returns The L1 construct properties | ||
| */ | ||
| _bind(): any; |
There was a problem hiding this comment.
I believe we do not need this. if this is the L2, we usually will create the L1 construct in the L2 class' constructor
There was a problem hiding this comment.
if I am wrong, and this is needed. Please add @internal annotation to it, so we do not expose this function to external customers, and skip adding it in docs
|
|
||
| /** | ||
| * Period-based metric properties | ||
| */ | ||
| export type PeriodBasedMetricProps = PeriodBasedAppSignalsMetricProps | PeriodBasedCloudWatchMetricProps; | ||
|
|
||
| /** | ||
| * Request-based metric properties | ||
| */ | ||
| export type RequestBasedMetricProps = RequestBasedAppSignalsMetricProps | RequestBasedCloudWatchMetricProps; |
There was a problem hiding this comment.
we do not use union types. Use Inheritance for this pattern, also do you want that type to be exposed to customers, if no, use @internal annotation on it
| export type SliMetricBaseProps = { | ||
| /** | ||
| * The threshold value for the metric | ||
| * | ||
| * @required | ||
| */ | ||
| readonly metricThreshold: number; | ||
|
|
||
| /** | ||
| * The comparison operator | ||
| * | ||
| * @default - Based on metric type | ||
| */ | ||
| readonly comparisonOperator?: ComparisonOperator; | ||
| } & (ApplicationSignalsMetricProps | CloudWatchMetricProps); |
There was a problem hiding this comment.
create actual interfaces, we do not support this while translating these types to other languages using JSII
| return new RollingInterval(props); | ||
| } | ||
|
|
||
| abstract bind(): any; |
There was a problem hiding this comment.
make it _bind() and also annotated with @internal
| return { | ||
| attainmentGoal: this.props.attainmentGoal ?? DEFAULT_GOAL_CONFIG.ATTAINMENT_GOAL, | ||
| warningThreshold: this.props.warningThreshold ?? DEFAULT_GOAL_CONFIG.WARNING_THRESHOLD, | ||
| interval: this.props.interval._bind(), |
There was a problem hiding this comment.
I believe this is wrong, you defined in the interface as bind not _bind
| bind() { | ||
| return { | ||
| calendarInterval: { | ||
| duration: this.duration, | ||
| durationUnit: this.unit, | ||
| startTime: this.startTime, | ||
| }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
what is the returning type ?
|
Comments on closed issues and PRs are hard for our team to see. |
4 participants
Issue # (if applicable)
Description of changes
This PR adds L2 constructs to simplify the Application Signals slo process. See aws/aws-cdk-rfcs#673 for more details.
Describe any new or updated permissions being added
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license