feat(applicationsignals-alpha): introduce Application Signals SLO L2 constructs #34413

TejeshreeDaliVenugopal · 2025-05-10T00:04:07Z

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.

Address the comments in RFC to update the keyAttributes.ts
Reference - feat(applicationsignals-alpha): introduce Application Signals SLO L2 constructs #33987

Description of how you validated changes

Added unit tests

Checklist

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

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

aws-cdk-automation

The pull request linter fails with the following errors:

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

…enablement-daemon.js.snapshot/ApplicationSignalsIntegrationECSDaemonDefaultTestDeployAssert0A2A622F.assets.json

…enablement-daemon.js.snapshot/ApplicationSignalsIntegrationECSDaemonDefaultTestDeployAssert0A2A622F.template.json

…enablement-daemon.js.snapshot/asset.a1acfc2b5f4f6b183fd2bb9863f486bc5edef6a357b355a070d9a0e502df418c/__entrypoint__.js

…enablement-daemon.js.snapshot/asset.a1acfc2b5f4f6b183fd2bb9863f486bc5edef6a357b355a070d9a0e502df418c/index.js

…enablement-daemon.js.snapshot/ecs-enablement-integration.assets.json

…enablement-daemon.js.snapshot/ecs-enablement-integration.template.json

…enablement-sidecar.js.snapshot/ApplicationSignalsIntegrationECSSidecarDefaultTestDeployAssert7F3E5290.assets.json

…enablement-sidecar.js.snapshot/ApplicationSignalsIntegrationECSSidecarDefaultTestDeployAssert7F3E5290.template.json

…enablement-sidecar.js.snapshot/ecs-enablement-integration.assets.json

…enablement-sidecar.js.snapshot/ecs-enablement-integration.template.json

aws-cdk-automation · 2025-05-10T02:12:57Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 5c18ee9
Result: FAILED
Build Logs (available for 30 days)

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

moelasmar

Thank you for adding the Application Signals SLO L2 constructs! I've reviewed the code and found a few issues that should be addressed

moelasmar · 2025-05-24T01:05:18Z

packages/@aws-cdk/aws-applicationsignals-alpha/lib/slo/slo-types.ts

+import { PeriodBasedAppSignalsMetricProps, PeriodBasedCloudWatchMetricProps, RequestBasedAppSignalsMetricProps, RequestBasedCloudWatchMetricProps } from './metric';
+import { Resource } from 'aws-cdk-lib';
+import { Construct } from 'constructs';
+import * as applicationsignals from 'aws-cdk-lib/aws-applicationsginals';


There's a typo in the import statement: aws-applicationsginals should be aws-applicationsignals

moelasmar · 2025-05-24T01:06:16Z

packages/@aws-cdk/aws-applicationsignals-alpha/lib/slo/keyAttributes.ts

+    constructor(props: KeyAttributesProps) {
+        this.validateProps(props);
+        this.props = props;
+    }


Consider adding JSDoc comments to explain what this validation is checking for.

moelasmar · 2025-05-24T01:06:47Z

packages/@aws-cdk/aws-applicationsignals-alpha/test/slo/interval.test.ts

@@ -0,0 +1,134 @@
+import {Interval} from '../../lib/slo/interval'


Consider adding spaces around the import braces for consistency: import { Interval } from '../../lib/slo/interval'

moelasmar

Thank you for adding the Application Signals SLO L2 constructs! I've reviewed the code and found several issues that should be addressed:

Naming conventions: Use IServiceLevelObjective for the interface and ServiceLevelObjectiveBase for the base class following CDK patterns
Avoid union types: Replace union types with proper inheritance hierarchies for better JSII compatibility
Documentation: Add JSDoc comments to all public methods and classes, especially mentioning which CFN resources they represent
Internal methods: Add @internal annotations to methods not intended for public use
Structure: Add an index.ts in the slo directory to better organize exports
Type safety: Define proper return types for methods instead of using any
Consistency: Make sure interface method names match implementation names (bind vs _bind)

Please address these issues to ensure the code follows CDK best practices and provides a good developer experience.

moelasmar · 2025-05-24T01:15:46Z

packages/@aws-cdk/aws-applicationsignals-alpha/lib/slo/constants.ts

+export const DEFAULT_GOAL_CONFIG = {
+    ATTAINMENT_GOAL: 99.9,
+    WARNING_THRESHOLD: 30,
+} as const;


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?

moelasmar · 2025-05-24T01:16:03Z

packages/@aws-cdk/aws-applicationsignals-alpha/lib/index.ts

 export * from './enablement/ecs-cloudwatch-agent';
 export * from './enablement/ecs-sdk-instrumentation';
+export * from './slo/slo';
+export * from './slo/slo-types';


Add an index.ts in the slo dir, and expose whatever you want there. Here, add only the directory.

moelasmar · 2025-05-24T01:18:26Z

packages/@aws-cdk/aws-applicationsignals-alpha/lib/slo/slo-types.ts

+    protected constructor(
+        scope: Construct,
+        id: string,
+        protected readonly props: PeriodBasedSloProps | RequestBasedSloProps


We do not recommend using union types. The Props should be of the base type of props, and in general the children classes of this abstract class will forward the concrete props classes. Also add docs.

moelasmar · 2025-05-24T01:19:00Z

packages/@aws-cdk/aws-applicationsignals-alpha/lib/slo/slo-types.ts

+/**
+ * Base class for all SLO implementations
+ */
+export abstract class ServiceLevelObjective extends Resource implements ISlo {


We usually follow the <<CFN Resource Name>>Base naming pattern. Name this as ServiceLevelObjectiveBase.

moelasmar · 2025-05-24T01:19:35Z

packages/@aws-cdk/aws-applicationsignals-alpha/lib/slo/slo-types.ts

+        props: RequestBasedSloProps
+    ): ServiceLevelObjective {
+        return new RequestBasedSlo(scope, id, props);
+    }


Add documentation, these are public functions and we should tell customers what are these functions for.

moelasmar · 2025-05-24T01:24:20Z

packages/@aws-cdk/aws-applicationsignals-alpha/lib/slo/metric.ts

+     * @default - Based on metric type
+     */
+    readonly comparisonOperator?: ComparisonOperator;
+} & (ApplicationSignalsMetricProps | CloudWatchMetricProps);


Create actual interfaces, we do not support this while translating these types to other languages using JSII.

moelasmar · 2025-05-24T01:24:55Z

packages/@aws-cdk/aws-applicationsignals-alpha/lib/slo/interval.ts

+        return new RollingInterval(props);
+    }
+
+    abstract bind(): any;


Make it _bind() and also annotated with @internal

moelasmar · 2025-05-24T01:25:33Z

packages/@aws-cdk/aws-applicationsignals-alpha/lib/slo/interval.ts

+        return {
+            attainmentGoal: this.props.attainmentGoal ?? DEFAULT_GOAL_CONFIG.ATTAINMENT_GOAL,
+            warningThreshold: this.props.warningThreshold ?? DEFAULT_GOAL_CONFIG.WARNING_THRESHOLD,
+            interval: this.props.interval._bind(),


I believe this is wrong, you defined in the interface as bind not _bind

moelasmar · 2025-05-24T01:25:49Z

packages/@aws-cdk/aws-applicationsignals-alpha/lib/slo/interval.ts

+        return {
+            attainmentGoal: this.props.attainmentGoal ?? DEFAULT_GOAL_CONFIG.ATTAINMENT_GOAL,
+            warningThreshold: this.props.warningThreshold ?? DEFAULT_GOAL_CONFIG.WARNING_THRESHOLD,
+            interval: this.props.interval._bind(),


This should be consistent with the interface method name

moelasmar · 2025-05-24T01:26:07Z

packages/@aws-cdk/aws-applicationsignals-alpha/lib/slo/interval.ts

+                startTime: this.startTime,
+            },
+        };
+    }


What is the returning type?

moelasmar · 2025-05-24T01:36:08Z

packages/@aws-cdk/aws-applicationsignals-alpha/lib/slo/slo-types.ts

+        }));
+    }
+
+    protected getMetricComparisonOperator(metric: PeriodBasedAppSignalsMetricProps | PeriodBasedCloudWatchMetricProps | RequestBasedAppSignalsMetricProps | RequestBasedCloudWatchMetricProps): ComparisonOperator {


if it will be internal function, so it is Ok to use union, but if the plan is to export it to function, so replace the union type with a parent type for all of these concrete types

moelasmar · 2025-05-24T01:37:30Z

packages/@aws-cdk/aws-applicationsignals-alpha/lib/slo/slo.ts

+/**
+ * Interface defining SLO behavior and runtime properties
+ */
+export interface ISlo {


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

moelasmar · 2025-05-24T01:37:57Z

packages/@aws-cdk/aws-applicationsignals-alpha/lib/slo/slo.ts

+     *
+     * @returns The L1 construct properties
+     */
+    _bind(): any;


I believe we do not need this. If this is the L2, we usually will create the L1 construct in the L2 class' constructor. 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.

moelasmar · 2025-05-24T01:38:33Z

packages/@aws-cdk/aws-applicationsignals-alpha/lib/slo/slo.ts

+     *
+     * @required
+     */
+    readonly name: string;


Is it a good idea to auto generate it somehow?

moelasmar

I am going to close the original PR

aws-cdk-automation · 2025-06-01T00:16:04Z

This PR has been in the BUILD FAILING 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:

Resume work on the PR
OR request an exemption by adding a comment containing 'Exemption Request' with justification e.x "Exemption Request: "
OR request clarification by adding a comment containing 'Clarification Request' with a question e.x "Clarification Request: "

This PR will automatically close in 14 days if no action is taken.

aws-cdk-automation · 2025-06-16T00:13:34Z

This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error.

github-actions · 2025-06-16T00:13:45Z

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.

aws-cdk-automation requested a review from a team May 10, 2025 00:04

github-actions bot added beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK p2 labels May 10, 2025

aws-cdk-automation requested changes May 10, 2025

View reviewed changes

bjrara and others added 7 commits May 9, 2025 17:29

Add application signals enablement L2

467d2a6

Address comments

9516a5b

Update daemon test

a314a29

Address additional comments

22fad49

feat: Add L2 CDK for application-signals-slo

4acf1ef

Improve interface definition and add more default values

3ffca9e

update keyAttributes and add unit tests

588dc02

TejeshreeDaliVenugopal force-pushed the dalivete-dev/application-signals-slo branch from 23d98a0 to 588dc02 Compare May 10, 2025 00:49

moelasmar added 11 commits May 9, 2025 18:24

Merge branch 'main' into dalivete-dev/application-signals-slo

015af7f

Delete packages/@aws-cdk/aws-applicationsignals-alpha/test/integ.ecs-…

60bec35

…enablement-daemon.js.snapshot/ApplicationSignalsIntegrationECSDaemonDefaultTestDeployAssert0A2A622F.assets.json

Delete packages/@aws-cdk/aws-applicationsignals-alpha/test/integ.ecs-…

c5904e9

…enablement-daemon.js.snapshot/ApplicationSignalsIntegrationECSDaemonDefaultTestDeployAssert0A2A622F.template.json

Delete packages/@aws-cdk/aws-applicationsignals-alpha/test/integ.ecs-…

95ff88e

…enablement-daemon.js.snapshot/asset.a1acfc2b5f4f6b183fd2bb9863f486bc5edef6a357b355a070d9a0e502df418c/__entrypoint__.js

Delete packages/@aws-cdk/aws-applicationsignals-alpha/test/integ.ecs-…

2c7cc85

…enablement-daemon.js.snapshot/asset.a1acfc2b5f4f6b183fd2bb9863f486bc5edef6a357b355a070d9a0e502df418c/index.js

Delete packages/@aws-cdk/aws-applicationsignals-alpha/test/integ.ecs-…

82f93d3

…enablement-daemon.js.snapshot/ecs-enablement-integration.assets.json

Delete packages/@aws-cdk/aws-applicationsignals-alpha/test/integ.ecs-…

c928a78

…enablement-daemon.js.snapshot/ecs-enablement-integration.template.json

Delete packages/@aws-cdk/aws-applicationsignals-alpha/test/integ.ecs-…

5c18ee9

…enablement-sidecar.js.snapshot/ApplicationSignalsIntegrationECSSidecarDefaultTestDeployAssert7F3E5290.assets.json

Delete packages/@aws-cdk/aws-applicationsignals-alpha/test/integ.ecs-…

27c51ef

…enablement-sidecar.js.snapshot/ApplicationSignalsIntegrationECSSidecarDefaultTestDeployAssert7F3E5290.template.json

Delete packages/@aws-cdk/aws-applicationsignals-alpha/test/integ.ecs-…

10f503a

…enablement-sidecar.js.snapshot/ecs-enablement-integration.assets.json

Delete packages/@aws-cdk/aws-applicationsignals-alpha/test/integ.ecs-…

6a59aff

…enablement-sidecar.js.snapshot/ecs-enablement-integration.template.json

moelasmar reviewed May 24, 2025

View reviewed changes

moelasmar suggested changes May 24, 2025

View reviewed changes

github-actions bot mentioned this pull request Jun 1, 2025

Monthly PR metrics report #34601

Closed

TejeshreeDaliVenugopal marked this pull request as draft June 13, 2025 17:59

aws-cdk-automation added the closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. label Jun 16, 2025

aws-cdk-automation closed this Jun 16, 2025

github-actions bot locked as resolved and limited conversation to collaborators Jun 16, 2025

		@@ -0,0 +1,134 @@
		import {Interval} from '../../lib/slo/interval'

feat(applicationsignals-alpha): introduce Application Signals SLO L2 constructs #34413

feat(applicationsignals-alpha): introduce Application Signals SLO L2 constructs #34413

Uh oh!

Conversation

TejeshreeDaliVenugopal commented May 10, 2025

Issue # (if applicable)

Description of changes

Description of how you validated changes

Checklist

Uh oh!

aws-cdk-automation left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aws-cdk-automation commented May 10, 2025

AWS CodeBuild CI Report

Uh oh!

moelasmar left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

moelasmar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

moelasmar left a comment

Choose a reason for hiding this comment

Uh oh!

aws-cdk-automation commented Jun 1, 2025

Uh oh!

aws-cdk-automation commented Jun 16, 2025

Uh oh!

github-actions bot commented Jun 16, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

aws-cdk-automation left a comment •

edited

Loading

moelasmar left a comment •

edited

Loading