feat(stepfunctions): support X-Ray tracing (#10371) #10374
Conversation
|
This PR required bumping the cfn spec... Likely you'd want to separate the cfn spec bump into a different commit but I wasn't sure what the best process to follow is here. Build logs below show some failures for cfn spec bump I think as I managed to build stepfunctions successfully so appreciate some input into how to best progress on this |
There was a problem hiding this comment.
I think there are a lot of unrelated changes here. can we clean up the PR before I can take a look?
@humanzz - the spec bump is currently being addressed in a separate PR by the cdk team. I'd say the best path forward would be to merge that in once it's resolved. can we simplify this PR int he meantime to exclude the spec bump changes (regardless of the PR build status) and that way I can focus the review on the code related to the feature we are introducing here.
|
@shivlaks that's because the first commit is to bump CloudFormation spec to get the changes needed for this feature (the second commit). I went over the commit logs and saw that in the most recent CloudFormation bump specs you guys tend to do it so I wonder if the right course of action here is to wait for you to do it and then rebase my changes? To be more concrete, I think
The question is: who will do the bump spec? Are you guys planning to do it soon? |
@humanzz - there have been some issues with importing the spec and they are actively being worked on in #10201 what do you think |
|
@shivlaks so, the second commit contains the changes for XRay. I wonder if that can be reviewed independently - if I click on it, I see the review options but not sure if you get the same view. Otherwise, given the dependency of the second commit on the first commit, I'm not sure how to best update this PR to exclude the first commit while still containing the necessary changes from it. Also, #10201 is for v18.2.0 (while the first commit in this PR is for v18.2.0) and on viewing its diff am not noticing anything related to stepfunctions (but am not sure if that means if the change necessary is part of it or not) What do you think? |
|
@humanzz - I can filter by module and review it, probably the simplest way forward. |
mergify
bot
dismissed
shivlaks’s stale review
Pull request has been modified.
|
@shivlaks so the cfn spec bump required was merged. I rebased and updated the PR so it should all be good for reviewing now |
extract buildLoggingConfiguration and buildTracingConfiguration into private methods use Role's addToPrincipalPolicy method instead of the deprecated addToPolicy
mergify
bot
dismissed
shivlaks’s stale review
Pull request has been modified.
| @@ -404,6 +387,49 @@ export class StateMachine extends StateMachineBase { | |||
| public metricTime(props?: cloudwatch.MetricOptions): cloudwatch.Metric { | |||
| return this.metric('ExecutionTime', props); | |||
| } | |||
|
|
|||
| private buildLoggingConfiguration(props: StateMachineProps) { | |||
There was a problem hiding this comment.
we should specify the return type for the method
| }; | ||
| } | ||
|
|
||
| private buildTracingConfiguration(props: StateMachineProps) { |
There was a problem hiding this comment.
we should specify the return type for the method
| return undefined; | ||
| } | ||
|
|
||
| this.addToRolePolicy(new iam.PolicyStatement({ |
There was a problem hiding this comment.
also worth linking to documentation as to why these cannot be scoped to resource level - i.e. point to https://docs.aws.amazon.com/xray/latest/devguide/security_iam_id-based-policy-examples.html#xray-permissions-resources
| destinations: [{ cloudWatchLogsLogGroup: { logGroupArn: props.logs.destination.logGroupArn } }], | ||
| includeExecutionData: props.logs.includeExecutionData, | ||
| level: props.logs.level || 'ERROR', | ||
| }; |
There was a problem hiding this comment.
nit: can we format this so it's a little more readable
| loggingConfiguration: this.buildLoggingConfiguration(props), | ||
| tracingConfiguration: this.buildTracingConfiguration(props), |
There was a problem hiding this comment.
do these really need to pass the entire props object?
I don't think the tracing configuration needs anything if checked here.
the logging configuration should really only need the LogOptions if logging has been specified
There was a problem hiding this comment.
I think it was a matter of style to similar methods in other packages... but maybe the style is not that consistent. Changing it
mergify
bot
dismissed
shivlaks’s stale review
Pull request has been modified.
|
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
closes #10371
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license