-
Notifications
You must be signed in to change notification settings - Fork 1
CDK Seed/stepfunction event logger #11
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
base: main
Are you sure you want to change the base?
Conversation
… datastore and sqs messages
I'll wait until you assign reviewers before doing a full review, but I think this construct serves as a great example of how we can write the infrastructure in TS but still use something like Python to buildout the business logic of code. 💯 |
@alukach @markdboyd This PR is ready for review, TODO issue is open 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.
Looks great! Just a few minor comments. Also, I don't really feel qualified to review the Python, so I'm going to leave that for someone else
|
||
const stepfunctionEventLogger = require('..'); | ||
|
||
describe('@cdk-seed/stepfunction-event-logger', () => { |
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.
So, if implemented, the tests would demonstrate that CDK code creates the right infrastructure? This whole thing of using tests to valid your infra-as-code is wild to me.
If so, is there any reason we're not implementing that?
export interface EventLoggerStandardLambdaProps extends EventLoggerBaseProps { | ||
readonly eventLoggingLevel: EventLoggingLevel | ||
readonly datastore: Datastore | ||
// readonly dynamodbSettings?: DynamodbSettings |
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.
Maybe a personal preference, but I prefer to delete commented code that isn't needed yet
var stepfunctionArns = Array<string>(); | ||
|
||
props.stepfunctions.forEach(sf => { | ||
stepfunctionArns.push(sf.stateMachineArn) | ||
}); |
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'm nitpicking a bit here, but var
is pretty much obsolete with const
and let
in ES6:
https://blog.usejournal.com/awesome-javascript-no-more-var-working-title-999428999994
Also, I'd recommend using a functional style operator when building an array from another array, as you're doing here. You get the immutability benefits of FP and I think it's a bit more expressive. So in this case:
const stepfunctionArns: Array<string>() = props.stepfunctions.map(sf => sf.stateMachineArn);
constructor(scope: Construct, id: string, props: EventLoggerStandardLambdaProps | EventLoggerCustomLambdaProps) { | ||
super(scope, id); | ||
|
||
function isCustomLambda(props: EventLoggerStandardLambdaProps | EventLoggerCustomLambdaProps): props is EventLoggerCustomLambdaProps { |
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.
Very well done
@@ -0,0 +1,223 @@ | |||
import { Construct, Duration } from "@aws-cdk/core"; |
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 get this warning from a build of this package (both packages in fact)
lib/index.ts:32:1 - warning JSII5: The type "constructs.Construct" is exposed in the public API of this module. Therefore, the module "constructs" must also be defined under "peerDependencies". This will be auto-corrected unless --no-fix-peer-dependencies was specified.
It seems like it's just saying you need to add this module as a peer dependency: https://www.npmjs.com/package/constructs?activeTab=versions
@@ -0,0 +1,11 @@ | |||
# `@cdk-seed/stepfunction-event-logger` | |||
|
|||
> TODO: description |
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 would like to see this README fleshed out a bit more
What I did:
I implemented a CDK construct that takes a list of StepFunction arn's and stores their execution history in dynamodb, or postgres (coming soon) or allows the user to customize their error handling. The event logger is further described here. The event logger
How I did it:
A lot of blood, sweat and tears. Mostly tears because of TypeScript's type checking, that is clearly very powerful, but I have no idea how to properly use.
The seed itself contains a CDK construct that defines:
The seed also contains a lambda directory with the code to process the stepfunction execution event
How to test it:
Check out this very simple example CDK project that contains the CDK seed.
Run:
pipenv install --dev
cdk deploy stepfunction-logger-test
This will stand up the construct along with a very simple stepfunction that the construct will monitor. Invoke the stepfunction with any parameters for a successful execution, and with
{test_failure: true}
for a failed execution. Notice in both cases how new events are written to the dynamodb table. It's very easy to query failed events by querying theExecutionFailed-Timestamp-Index
fortype = "ExecutionFailed"
.To see the construct operating with a "custom" error handling lamdba, set
custom_lamdba
toTrue
instack/app.py
and re-deploy the stack using the above command. This will re-deploy the stack with a "custom" lambda (insrc/lambda
) that will instead store events in S3. This is supposed to show how easy it is for the user to supply their own lambda to process the events. This custom lambda can just as easily point to an existing error storage system, fire off emails to responsible people, etc).TODO: