fix(stepfunctions): disabling logging still requires LogGroup #30816

Tietew · 2024-07-10T10:20:43Z

Issue # (if applicable)

Reason for this change

To disable logging on a StateMachine (with logging enabled), we should specify LogLevel.OFF to LogOptions.level. But cannot remove the LogGroup because LogOptions.destination is required.

new sfn.StateMachine(this, 'StateMachine', {
  definitionBody: ...,
  logs: { level: sfn.LogLevel.OFF } // allow to disable logging
});

Description of changes

Make LogOptions.destination optional.
Validate LogOptions.destination is present when LogOptions.level is not OFF.

Description of how you validated changes

Unit and integ tests that verify LogOptions.destination is opitional when LogOptions.level is OFF and throw an exception otherwise.

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 has failed. See the aws-cdk-automation comment below for failure reasons. 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.

Tietew · 2024-07-10T10:25:13Z

~~Clarification Request~~
Existing integration tests do not include StateMachine with logging enabled.
Needed to add integ tests?
Added.

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

ChakshuGupta13 · 2024-07-15T16:22:18Z

packages/aws-cdk-lib/aws-stepfunctions/lib/state-machine.ts

+    if (logOptions.level !== LogLevel.OFF && !logOptions.destination) {
+      throw new Error('Logs destination is required when level is not OFF.');
+    }


shall this validation be done earlier (at line 427) with other validations instead?

if (!logs && !logs.level != LogLevel.OFF && !logs.destination) { ...

reasons for ask:

with current change, validations will be split across various places and as this file is big (and keep getting bigger), hence, it will become difficult to have complete picture of validation

ideally, caller shall not even call buildLoggingConfiguration if prerequisites are not met

added a separate validation method validateLogOptions()

ChakshuGupta13 · 2024-07-15T16:29:49Z

packages/aws-cdk-lib/aws-stepfunctions/lib/state-machine.ts

+    if (logOptions.destination) {
+      // https://docs.aws.amazon.com/step-functions/latest/dg/cw-logs.html#cloudwatch-iam-policy
+      this.addToRolePolicy(new iam.PolicyStatement({
+        effect: iam.Effect.ALLOW,
+        actions: [
+          'logs:CreateLogDelivery',
+          'logs:GetLogDelivery',
+          'logs:UpdateLogDelivery',
+          'logs:DeleteLogDelivery',
+          'logs:ListLogDeliveries',
+          'logs:PutResourcePolicy',
+          'logs:DescribeResourcePolicies',
+          'logs:DescribeLogGroups',
+        ],
+        resources: ['*'],
+      }));
+    }

    return {
-      destinations: [{
+      destinations: logOptions.destination ? [{
        cloudWatchLogsLogGroup: { logGroupArn: logOptions.destination.logGroupArn },
-      }],
+      }] : undefined,


if we do this like as follows - will it reduce need to check logOptions.destination twice and simplify a bit?

let destinations = undefined; if (logOptions.destination) { // Policy addition destinations = [{ cloudWatchLogsLogGroup: { logGroupArn: logOptions.destination.logGroupArn }, }]; } return { destinations, ..., level: logOptions.level || LogLevel.ERROR }

ChakshuGupta13 · 2024-07-15T16:42:01Z

packages/aws-cdk-lib/aws-stepfunctions/lib/state-machine.ts

   */
-  readonly destination: logs.ILogGroup;
+  readonly destination?: logs.ILogGroup;


will this not be a breaking change for existing usage? example:

const logOptions: LogOptions = { destination: <LogGroup> } const stateMachine: StateMachine = new StateMachine(parent, 'ID', { logs: logOptions, ... } ... stateMachine.logs.destination.logGroupName // Error stateMachine.logs.destination!.logGroupName // Change needed

Some user(s) might be utilising ILogGroup's public properties from logOptions which will now cause issues because now such users will need to make non-null assertions before using such properties (e.g. with !)

According to the jsii-diff documentation:

You are allowed to make inputs optional

So i think this change is not a breaking change.

ChakshuGupta13 · 2024-07-15T16:44:22Z

packages/aws-cdk-lib/aws-stepfunctions/test/state-machine.test.ts

+
+    expect(() => {
+      new sfn.StateMachine(stack, 'MyStateMachine', {
+        definitionBody: sfn.DefinitionBody.fromChainable(sfn.Chain.start(new sfn.Pass(stack, 'Pass'))),


can we not directly pass new sfn.Pass(stack, 'Pass') to DefintionBody.fromChainable as Pass implements IChainable?

it's just a copy & paste.
README also uses sfn.Chain.start() instead of bare sfn.Pass.

…-logging

codecov · 2024-11-20T04:18:07Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.18%. Comparing base (baa8561) to head (e0aacb9).

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #30816   +/-   ##
=======================================
  Coverage   77.18%   77.18%           
=======================================
  Files         105      105           
  Lines        7161     7161           
  Branches     1312     1312           
=======================================
  Hits         5527     5527           
  Misses       1454     1454           
  Partials      180      180

Flag	Coverage Δ
suite.unit	`77.18% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
packages/aws-cdk	`77.18% <ø> (ø)`

---- 🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests
JS Bundle Analysis - Avoid shipping oversized bundles

aws-cdk-automation · 2024-11-20T04:41:14Z

AWS CodeBuild CI Report

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

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

make LogOptions.destination optional

b62fe80

github-actions bot added bug This issue is a bug. p2 labels Jul 10, 2024

aws-cdk-automation requested a review from a team July 10, 2024 10:20

github-actions bot added the admired-contributor [Pilot] contributed between 13-24 PRs to the CDK label Jul 10, 2024

aws-cdk-automation previously requested changes Jul 10, 2024

View reviewed changes

aws-cdk-automation added the pr/reviewer-clarification-requested The contributor has requested clarification on feedback, a failing build, or a failing PR Linter run label Jul 10, 2024

add @default

3efbfba

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jul 11, 2024

add an intg test

3ca2073

github-actions bot mentioned this pull request Jul 15, 2024

Weekly PR metrics report #30858

Closed

ChakshuGupta13 suggested changes Jul 15, 2024

View reviewed changes

Tietew added 2 commits July 18, 2024 11:43

review

f5b6d2c

Merge remote-tracking branch 'origin/main' into stepfunctions-disable…

6a0fb67

…-logging

github-actions bot mentioned this pull request Aug 1, 2024

Monthly PR metrics report #30999

Closed

Tietew requested a review from ChakshuGupta13 August 14, 2024 02:54

Merge branch 'main' into stepfunctions-disable-logging

6dc606e

Tietew changed the title ~~fix(stepfunctions): make LogOptions.destination optional~~ fix(stepfunctions): disabling logging still requires LogGroup Aug 22, 2024

github-actions bot added the effort/small Small work item – less than a day of effort label Aug 22, 2024

Tietew added 2 commits August 27, 2024 14:11

Merge branch 'main' into stepfunctions-disable-logging

ef79c2c

Merge branch 'main' into stepfunctions-disable-logging

e0aacb9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(stepfunctions): disabling logging still requires LogGroup #30816

fix(stepfunctions): disabling logging still requires LogGroup #30816

Tietew commented Jul 10, 2024 •

edited

Loading

aws-cdk-automation left a comment

Tietew commented Jul 10, 2024 •

edited

Loading

ChakshuGupta13 Jul 15, 2024

Tietew Jul 18, 2024

ChakshuGupta13 Jul 15, 2024

Tietew Jul 18, 2024

ChakshuGupta13 Jul 15, 2024

Tietew Jul 16, 2024

ChakshuGupta13 Jul 15, 2024

Tietew Jul 18, 2024

codecov bot commented Nov 20, 2024 •

edited

Loading

aws-cdk-automation commented Nov 20, 2024

fix(stepfunctions): disabling logging still requires LogGroup #30816

Are you sure you want to change the base?

fix(stepfunctions): disabling logging still requires LogGroup #30816

Conversation

Tietew commented Jul 10, 2024 • edited Loading

Issue # (if applicable)

Reason for this change

Description of changes

Description of how you validated changes

Checklist

aws-cdk-automation left a comment

Choose a reason for hiding this comment

Tietew commented Jul 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 20, 2024 • edited Loading

Codecov Report

aws-cdk-automation commented Nov 20, 2024

AWS CodeBuild CI Report

Tietew commented Jul 10, 2024 •

edited

Loading

Tietew commented Jul 10, 2024 •

edited

Loading

codecov bot commented Nov 20, 2024 •

edited

Loading