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(stepfunctions): support X-Ray tracing (#10371) #10374

Merged
merged 6 commits into from
Sep 18, 2020
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions packages/@aws-cdk/aws-stepfunctions/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -568,6 +568,22 @@ new stepfunctions.StateMachine(stack, 'MyStateMachine', {
});
```

## X-Ray tracing

Enable X-Ray tracing for StateMachine:

```ts
const logGroup = new logs.LogGroup(stack, 'MyLogGroup');

new stepfunctions.StateMachine(stack, 'MyStateMachine', {
definition: stepfunctions.Chain.start(new stepfunctions.Pass(stack, 'Pass')),
tracingEnabled: true
});
```

See [the AWS documentation](https://docs.aws.amazon.com/step-functions/latest/dg/concepts-xray-tracing.html)
to learn more about AWS Step Functions's X-Ray support.

## State Machine Permission Grants

IAM roles, users, or groups which need to be able to work with a State Machine should be granted IAM permissions.
Expand Down
80 changes: 53 additions & 27 deletions packages/@aws-cdk/aws-stepfunctions/lib/state-machine.ts
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,13 @@ export interface StateMachineProps {
* @default No logging
*/
readonly logs?: LogOptions;

/**
* Specifies whether Amazon X-Ray tracing is enabled for this state machine.
*
* @default false
*/
readonly tracingEnabled?: boolean;
shivlaks marked this conversation as resolved.
Show resolved Hide resolved
}

/**
Expand Down Expand Up @@ -272,37 +279,13 @@ export class StateMachine extends StateMachineBase {

this.stateMachineType = props.stateMachineType ? props.stateMachineType : StateMachineType.STANDARD;

let loggingConfiguration: CfnStateMachine.LoggingConfigurationProperty | undefined;
if (props.logs) {
const conf = props.logs;
loggingConfiguration = {
destinations: [{ cloudWatchLogsLogGroup: { logGroupArn: conf.destination.logGroupArn } }],
includeExecutionData: conf.includeExecutionData,
level: conf.level || 'ERROR',
};
// 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: ['*'],
}));
}

const resource = new CfnStateMachine(this, 'Resource', {
stateMachineName: this.physicalName,
stateMachineType: props.stateMachineType ? props.stateMachineType : undefined,
roleArn: this.role.roleArn,
definitionString: Stack.of(this).toJsonString(graph.toGraphJson()),
loggingConfiguration,
loggingConfiguration: this.buildLoggingConfiguration(props),
tracingConfiguration: this.buildTracingConfiguration(props),
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it was a matter of style to similar methods in other packages... but maybe the style is not that consistent. Changing it

});

resource.node.addDependency(this.role);
Expand All @@ -324,7 +307,7 @@ export class StateMachine extends StateMachineBase {
* Add the given statement to the role's policy
*/
public addToRolePolicy(statement: iam.PolicyStatement) {
this.role.addToPolicy(statement);
this.role.addToPrincipalPolicy(statement);
}

/**
Expand Down Expand Up @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

we should specify the return type for the method

if (props.logs === undefined) {
return undefined;
}

// 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: [{ cloudWatchLogsLogGroup: { logGroupArn: props.logs.destination.logGroupArn } }],
includeExecutionData: props.logs.includeExecutionData,
level: props.logs.level || 'ERROR',
};
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can we format this so it's a little more readable

}

private buildTracingConfiguration(props: StateMachineProps) {
Copy link
Contributor

Choose a reason for hiding this comment

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

we should specify the return type for the method

if (!props.tracingEnabled) {
return undefined;
}

this.addToRolePolicy(new iam.PolicyStatement({
Copy link
Contributor

Choose a reason for hiding this comment

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

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

actions: ['xray:PutTraceSegments', 'xray:PutTelemetryRecords'],
resources: ['*'],
}));

return {
enabled: true,
};
}
}

/**
Expand Down
40 changes: 39 additions & 1 deletion packages/@aws-cdk/aws-stepfunctions/test/state-machine.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -120,4 +120,42 @@ describe('State Machine', () => {
});
});

});
test('tracing configuration', () => {
// GIVEN
const stack = new cdk.Stack();

// WHEN
new stepfunctions.StateMachine(stack, 'MyStateMachine', {
definition: stepfunctions.Chain.start(new stepfunctions.Pass(stack, 'Pass')),
tracingEnabled: true,
});

// THEN
expect(stack).toHaveResource('AWS::StepFunctions::StateMachine', {
DefinitionString: '{"StartAt":"Pass","States":{"Pass":{"Type":"Pass","End":true}}}',
TracingConfiguration: {
Enabled: true,
},
});

expect(stack).toHaveResource('AWS::IAM::Policy', {
PolicyDocument: {
Statement: [{
Action: [
'xray:PutTraceSegments',
'xray:PutTelemetryRecords',
],
Effect: 'Allow',
Resource: '*',
}],
Version: '2012-10-17',
},
PolicyName: 'MyStateMachineRoleDefaultPolicyE468EB18',
Roles: [
{
Ref: 'MyStateMachineRoleD59FFEBC',
},
],
});
});
});