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: add skeleton of step function class and example stack #290

Open
wants to merge 1 commit into
base: yiming.luo/step-function-refactor2
Choose a base branch
from

Conversation

lym953
Copy link
Contributor

@lym953 lym953 commented Aug 30, 2024

What does this PR do?

  1. Add a skeleton of DatadogStepFunctions class, which users will use to add Datadog monitoring for their Step Functions
  2. Add an example TypeScript stack showing how to use this class in a CDK
    • a large part of the stack was copied from the existing TypeScript stack for Lambda functions

Motivation

For SVLS-4394: Customers can instrument their Step Functions with CDK macro and SAM

Testing Guidelines

Followed the internal wiki for testing local changes to CDK Construct

After cdk deploy, a CloudFormation stack was deployed successfully, which includes a Lambda function and a Step Function, though Datadog monitoring is not set up for them.

Additional Notes

Please comment on naming. I'm using:

  1. DatadogStepFunctions as the class name
  2. addStateMachines() as the function name

Let me know if you prefer to use:

  1. the singular form (without s), and/or
  2. addStepFunctions() instead of addStateMachines()

Next steps:

  1. Implement DatadogStepFunctions class, make it do various work needed to set up Datadog monitoring. I'll do this in small steps, sending small PRs.
  2. After the TypeScript stack fully works, create an example for Step Functions in Go and Python.

Types of Changes

  • Bug fix
  • New feature
  • Breaking change
  • Misc (docs, refactoring, dependency upgrade, etc.)

Check all that apply

  • This PR's description is comprehensive
  • This PR contains breaking changes that are documented in the description
  • This PR introduces new APIs or parameters that are documented and unlikely to change in the foreseeable future
  • This PR impacts documentation, and it has been updated (or a ticket has been logged)
  • This PR's changes are covered by the automated tests
  • This PR collects user input/sensitive content into Datadog

@lym953 lym953 requested review from a team as code owners August 30, 2024 20:33
import { CdkStepFunctionsTypeScriptStack } from "../lib/cdk-step-functions-typescript-stack";

const app = new cdk.App();
new CdkStepFunctionsTypeScriptStack(app, "CdkStepFunctionsTypeScriptStack", {});

Choose a reason for hiding this comment

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

Code Quality Violation

Do not use 'new' for side effects. (...read more)

A lonely instance is almost always useless. Do not create objects without assigning them to a variable that you will use later.

View in Datadog  Leave us feedback  Documentation

@@ -91,3 +91,5 @@ export interface Node {
}

export type LambdaFunction = lambda.Function | lambda.SingletonFunction;

export interface DatadogStepFunctionsProps {}

Choose a reason for hiding this comment

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

Code Quality Violation

An empty interface is equivalent to `{}`. (...read more)

Do not use empty interfaces.

View in Datadog  Leave us feedback  Documentation

),
});

console.log("Instrumenting Step Functions in TypeScript stack with Datadog");

Choose a reason for hiding this comment

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

🟠 Code Quality Violation

Unexpected console statement. (...read more)

Debugging with console is not considered a bad practice, but it's easy to forget about console statements and leave them in production code. There is no need to pollute production builds with debugging statements.

View in Datadog  Leave us feedback  Documentation

constructor(scope: Construct, id: string, props?: StackProps) {
super(scope, id, props);

console.log("Creating Hello World TypeScript stack");

Choose a reason for hiding this comment

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

🟠 Code Quality Violation

Unexpected console statement. (...read more)

Debugging with console is not considered a bad practice, but it's easy to forget about console statements and leave them in production code. There is no need to pollute production builds with debugging statements.

View in Datadog  Leave us feedback  Documentation

@@ -0,0 +1,6 @@
*.ts
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copied from examples/typescript-stack/.npmignore

@@ -0,0 +1,3 @@
# Datadog CDK TypeScript Example
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Written from scratch

@@ -0,0 +1,7 @@
#!/usr/bin/env node
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Modified from examples/typescript-stack/bin/index.ts

@@ -0,0 +1,39 @@
{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copied from examples/typescript-stack/cdk.json

@@ -0,0 +1,40 @@
import * as lambda from "aws-cdk-lib/aws-lambda";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Written from scratch

@@ -0,0 +1,20 @@
{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copied from examples/typescript-stack/package.json

@@ -0,0 +1,23 @@
/*
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Written from scratch.

Copy link

@jhgilbert jhgilbert left a comment

Choose a reason for hiding this comment

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

Approved with some suggestions, thanks!

@@ -0,0 +1,3 @@
# Datadog CDK TypeScript Example

This example is used for testing the [datadog-cdk-constructs](https://github.com/DataDog/datadog-cdk-constructs) v2 library for Step Functions. The Step Functions support is still being built. Once it's ready, this example will be merged into the `typescript-stack` so that a single stack can be used for testing both Lambda functions and Step Functions, making it easier for CDK construct developers to test changes and ensure that their changes don't break things. The two examples are kept separate for now so that the incomplete Step Function support won't break the testing of Lambda functions.

Choose a reason for hiding this comment

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

Suggested change
This example is used for testing the [datadog-cdk-constructs](https://github.com/DataDog/datadog-cdk-constructs) v2 library for Step Functions. The Step Functions support is still being built. Once it's ready, this example will be merged into the `typescript-stack` so that a single stack can be used for testing both Lambda functions and Step Functions, making it easier for CDK construct developers to test changes and ensure that their changes don't break things. The two examples are kept separate for now so that the incomplete Step Function support won't break the testing of Lambda functions.
This example is used to test the [datadog-cdk-constructs](https://github.com/DataDog/datadog-cdk-constructs) v2 library for Step Functions. The Step Functions support is still in development. Once it's ready, this example will be merged into the `typescript-stack` so that a single stack can be used for testing both Lambda functions and Step Functions, making it easier for CDK construct developers to test their changes. The two examples are kept separate for now, so the incomplete Step Function support won't break the testing of Lambda functions.

I skipped the explanation of why someone would test, just because your audience likely already knows that. But if you wanted to keep it in, you can leave that part. I might say "easier for CDK construct developers to ensure that their changes don't cause regressions" or something along those lines, just for simplicity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Make sense! I prefer simplicity as well. Will remove the redundant part.

@lym953 lym953 force-pushed the yiming.luo/step-function-example-stack branch from 0807af8 to 14a7971 Compare September 4, 2024 20:07
@lym953 lym953 force-pushed the yiming.luo/step-function-refactor2 branch 2 times, most recently from 43990ce to 1fe6976 Compare October 16, 2024 18:17
@lym953 lym953 force-pushed the yiming.luo/step-function-example-stack branch from 14a7971 to 7ed97a7 Compare October 16, 2024 18:23
Comment on lines +8 to +40
export class CdkStepFunctionsTypeScriptStack extends Stack {
constructor(scope: Construct, id: string, props?: StackProps) {
super(scope, id, props);

console.log("Creating Hello World TypeScript stack");

const helloLambdaFunction = new lambda.Function(this, "hello-python", {
runtime: lambda.Runtime.PYTHON_3_12,
timeout: Duration.seconds(10),
memorySize: 256,
code: lambda.Code.fromAsset("../lambda/python", {
bundling: {
image: lambda.Runtime.PYTHON_3_12.bundlingImage,
command: ["bash", "-c", "pip install -r requirements.txt -t /asset-output && cp -aT . /asset-output"],
},
}),
handler: "hello.lambda_handler",
});

const stateMachine = new sfn.StateMachine(this, "MyStateMachine", {
definitionBody: sfn.DefinitionBody.fromChainable(
new tasks.LambdaInvoke(this, "MyLambdaTask", {
lambdaFunction: helloLambdaFunction,
}).next(new sfn.Succeed(this, "GreetedWorld")),
),
});

console.log("Instrumenting Step Functions in TypeScript stack with Datadog");

const datadogSfn = new DatadogStepFunctions(this, "Datadog", {});
datadogSfn.addStateMachines([stateMachine]);
}
}

Choose a reason for hiding this comment

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

Code Quality Violation

This class is unnecessary (...read more)

This rule advises against the unnecessary use of classes that contain only static members, or nothing. In JavaScript, classes are primarily used for object-oriented programming, where each instance of a class has its own state and behavior. Static members, on the other hand, belong to the class itself and not to any instance of the class.

When a class contains only static members, it does not make use of JavaScript's object-oriented capabilities, and it can be more difficult to understand, test, and maintain than necessary. In order to avoid this issue, consider using regular functions and variables instead of static class members. This makes your code easier to understand and maintain, and it allows you to make better use of JavaScript's features.

View in Datadog  Leave us feedback  Documentation

this.props = props;
}

public addStateMachines(_stateMachines: sfn.StateMachine[], _construct?: Construct): void {}

Choose a reason for hiding this comment

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

🔴 Code Quality Violation

Suggested change
public addStateMachines(_stateMachines: sfn.StateMachine[], _construct?: Construct): void {}
public addStateMachines(_stateMachines: sfn.StateMachine[], _construct?: Construct): void {/* empty */}
Empty statement. (...read more)

Empty or non-functional blocks in the code can be misleading and lead to maintenance difficulties. They can also lead to a false sense of security or functionality. While they may not directly introduce security issues, their presence can suggest that some logic or error handling is implemented when it is not.

View in Datadog  Leave us feedback  Documentation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants